[squid-dev] [PATCH] SBuf Locker friend class

[Alex Rousskov]

On 10/29/2015 01:23 PM, Amos Jeffries wrote:

> Adds the Locker friend class to SBuf to provide safety ref-count locks
> when dealing with dangerous char* or SBuf& inputs.

Thank you for working on this.


> +    Locker prevent_raw_memory_madness(this, S, n);

If you can make it constant, make it const.


> +            // lock if Q intersects the parents buffer area
> +            const MemBlob *P = parent->store_.getRaw();
> +            if ( (Q+len) >= P->mem && Q <= (P->mem + P->capacity) )


I do not think "len" is important here. We are not trying to protect
ourselves against invalid pointers that start somewhere before the blob
but end inside the blob. If I am right, please remove the len argument
and simplify.

I recommend using "min <= x <= max" pattern.

Also, a Q pointing at (P->mem + P->capacity) does not belong to P!

Please avoid single-character names. They may the code more difficult to
understand, search, and refactor.

Please reserve capitalized names for Globals.

In summary, consider:

    if (blob->mem <= mem && mem < blob->mem + blob->capacity)


However, I would actually make it an inlined MemBlob method:

    if (blob->contains(mem))


I like your refcounting Pointer trick. Much better than manual
lock/unlock work!


> I have gone through and added it to all the non-const methods that
> appear to be taking char* directly, or using a char* taken from SBuf&.
> That review added the appendf/printf methods to the set we identified on
> IRC.

What about SBuf::scanf()? Unlikely, but, the format parameter might be
pointing to the same blob that gets deleted when we call c_str()...



>  SBuf &
>  SBuf::append(const char * S, size_type Ssize)
>  {
>      if (S == NULL)
>          return *this;
>      if (Ssize == SBuf::npos)
>          Ssize = strlen(S);
>      debugs(24, 7, "from c-string to id " << id);
> +    Locker prevent_raw_memory_madness(this, S, Ssize);

and

> SBuf::vappendf(const char *fmt, va_list vargs)
>  {
>      Must(fmt != NULL);
>      int sz = 0;
>      //reserve twice the format-string size, it's a likely heuristic
>      size_type requiredSpaceEstimate = strlen(fmt)*2;
>  
> +    // with appendf() an arg might be a dangerous char*
> +    Locker prevent_raw_memory_madness(this, buf(), length());
> +

If possible, let's always start dangerous methods with allocating a
Locker object. This clarifies the lock scope and minimizes the chance
that somebody adds dangerous code _above_ the lock.


> +    // with printf() an arg might be a dangerous char*

> +    // with appendf() an arg might be a dangerous char*


s/an arg/fmt or arg/ or s/an arg/any parameter/

That is, the "fmt" parameter itself is not excluded from danger.


> IMO the trim() method mentioned on IRC does not need it since the scope
> lifetime of the pointer taken by toRemove.buf() does not cross any
> alterations of the this objects members.

Agreed.


I also suggest renaming "prevent_raw_memory_madness" to "blobKeeper" to
clarify and for consistency with other camelCase names.



> I've not added documentation for the Locker class. Alex could you
> provide a blurb to describe the memory issue and requirements this solves ?


How about something like the following?

/// Keeps SBuf's MemBlob alive in a blob-destroying context where
/// a seemingly unrelated memory pointer may belong to the same blob.
/// For [an extreme] example, consider: a.append(a).
/// Compared to an SBuf temporary, this class is optimized to
/// preserve blobs only if needed and to reduce debugging noise.
class Locker ...


HTH,

Alex.