Re: [PATCH] String API re-implementation using SBuf

On 23/11/2013 5:49 a.m., Alex Rousskov wrote:
> On 11/22/2013 08:25 AM, Amos Jeffries wrote:
>>>>>> char const *
>>>>>> String::termedBuf() const
>>>>>> {
>>>>>> - return buf_;
>>>>>> + if (!defined()) return NULL;
>>>>
>>>> Since an empty String could have been defined() in the unpatched code, I
>>>> suggest relaxing this to always return c_str(), even for undefined
>>>> Strings. I think it will be safer this way, but you should double check
>>>> that I did not miss any cases where it would break something.
>>>>
>> I've gone through the non-String code use of String::defined() and tried
>> to find other ways of avoiding the defined()/NULL testing for ambiguous
>> cases of String/SBuf differences.
>>
>> If you can check this attached patch is okay for merge to trunk we can
>> drop the nasty defined_ tracking the conversion patch has to do.
>
>
> Done, but please note that removing external defined() callers will not
> solve the termedBuf() problem. There are three cases from termedBuf()
> caller point of view:
>
> 1. Caller expects defined() String to return a not-NULL c-string.
> 2. Caller expects an empty "" String to return a not-NULL c-string.
> 3. Caller expects !defined() String to return a NULL c-string.
>
> Case #1 is handled the same before and after your changes.
> Case #2 will be broken if you do not return buf_.c_str().
> Case #3 will be broken if you return buf_.c_str().
>
> We may not know for sure whether cases #2 and #3 exist, but we need to
> pick between breaking case #2 or case #3. Due to termedBuf()
> look-and-fill, and after quickly scanning termedBuf() callers, I suggest
> that we break #3 and return buf_.c_str() after you double check those
> callers and adjust those that need adjustments (if any).
>

Noted.

I am going through the list of direct callers now (and extending it with
indirect callers as found). Of the first 75 many of them have no visible
NULL protection except assert() or will just pass it to some syscall
which expects a 0-terminated string.

The one border case so far is ACL StringData::match using NULL to avoid
walking the splay tree on non-"" empty strings. But there is no
protection against explicit "" strings being sent anyway so this seems
like a useless optimization.

Amos
Received on Sat Nov 23 2013 - 11:36:06 MST