[squid-dev] [RFC] Support concurrent SBuf::c_str() calls

[Amos Jeffries]

On 30/09/2016 5:03 a.m., Alex Rousskov wrote:
> Hello,
> 
>     The current trunk code contains at least two serious bugs caused by
> SBuf::c_str() misuse. Both known bugs looks similar:
> 
>> storeCreateEntry(storeUri.c_str(), storeUri.c_str(), ...);
> 
> and
> 
>> storeCreateEntry(uri.c_str(), uri.c_str(), ...);
> 
> 
> Both use cases violate safe c_str() use conditions documented in the
> long and occasionally YELLING description of that method. As you have
> probably guessed by now, the c_str() method itself is not constant, so
> calling c_str() twice often destroys the first c_str() result.
> 
> It is trivial to fix both violations, but it is very difficult to say
> how many other similar violations are hidden in (or will be added to)
> the code. I am sure we will continue to miss them during code reviews.
> 
> This particular problem is caused by the "used storage size" increment
> in the SBuf::c_str() implementation:
> 
>>     *rawSpace(1) = '\0'; // terminate the buffer
>>     ++store_->size; // protect the terminator character
> 
> The increment forces the second call to c_str() to re-allocate the
> store, which usually destroys the still-in-use storage pointed to by the
> first c_str() result.
> 
> If we remove the increment, then Squid will work as expected when using
> two concurrent c_str() results.
> 
> With or without the increment, Squid may crash or misbehave when buggy
> code appends to SBuf while a previous c_str() result is still in use:
> With an increment, appending may destruct the previous c_str() buffer.
> Without an increment, appending may overwrite the 0-terminator used by
> the previous c_str() call.
> 
> Valgrind is able to detect the "concurrent c_str()s" bugs in the current
> trunk. My Valgrind tests did not expose other related bugs when I
> removed the increment, but that does not mean those other bugs do not
> exist, of course.
> 
> Should we remove the increment to make concurrent c_str() calls safe?

The reason it exists remember is to prevent other SBuf sharing that
storage MemBuf from thinking they can append to the storage oer top of
the terminator. Given that we have no easy knowledge of how the c_str()
is going to be used by the recipient code we cannot guarantee lack of
terminator is safe.
 If we could then rawContent() should have been used instead of c_str().

Time to push SBuf usage forward and aggressively remove the need for
c_str()?

Cheers
Amos