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. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev