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

[Alex Rousskov]

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?


Thank you,

Alex.