Re: [RFC] Time to talk about StringNG merge again?

On Wed, Jul 31, 2013 at 2:46 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 1/08/2013 12:18 a.m., Kinkie wrote:
>>
>> On Wed, Jul 31, 2013 at 2:53 AM, Alex Rousskov
>> <rousskov_at_measurement-factory.com> wrote:
>>>
>>> On 07/30/2013 03:56 AM, Kinkie wrote:
>>>
>>>> lp:~squid/squid/stringng-cherrypick has the most uptodate code.
>>>
>>>
>>> I did not finish the review before running out of time, but I found one
>>> or two bugs in r12761 (in the code I have not looked at before). There
>>> are also a few polishing issues:
>>
>> Thanks.
>>
>>>> typedef int32_t size_type;
>>>
>>> I think we should make it unsigned. I agree with your earlier
>>> observation that it is better to do that now rather than later or never.
>>
>> Yes, it is agreed. It's however something I'd prefer to do as a
>> separate change, after everything else has been defined in order to
>> have a clear impact.
>
>
> Yes and no. It will probably cause a lot of little changes all over the
> classes involved. So doing it at a point of stability as a separate commit
> to stringng is good but not a reason to

How about doing it after this round of reviews is done?
That should be a reasonable compromise.

>>>> void reserveSpace(size_type minSpace)
>>>> {reserveCapacity(length()+minSpace);}
>>>
>>> As I mentioned before, this lacks overflow control if the sum of
>>> length() and minSpace exceeds size_type maximum. Please add a check.
>>
>> reserve* calls cow() which - if a resizing is needed - calls reAlloc
>> which throws SBufTooBigException if the requested size is too big.
>> Isn't that good enough?
>
>
> Not if the math overflowed down to a smaller value before it even got passed
> to reserveCapacity().

Ok. I'm going to check minSpace. maxSize+minSpace is definitely not
enough to overflow size_type

>>>> cow(minSpace+length());
>>>
>>> Same here, in rawSpace().
>
>
>>
>>> BTW, please add SBuf::spaceSize() as a public method so that
>>> optimization like the above will work. Otherwise, non-privileged users
>>> will have to write
>>>
>>> foobar(buf.rawSpace(minSize), minSize);
>>>
>>> instead of getting an advantage of the actual space size being larger
>>> than the requested minimum.
>>
>> Done, but I'm quite wary of this: it implies a knowledge about
>> internal store positioning which I would prefer to hide from clients.
>
>
> I think we may find a reason in future to extend this futther to allow
> caller handling of the cases where realloc is not possible to the new
> minSize.
> But for now that should be enough.

Ok.

>
>
>>
>>>> if (sz >= static_cast<int>(store_->spaceSize()))
>>>> rawSpace(sz*2); // TODO: tune heuristics
>>>
>>> I would use reserveSpace() here instead of ignoring rawSpace() result
>>> because we know the buffer needs to be reallocated at this point, right?
>>
>>
>> Hand-unrolled code doesn't ignore that return value anymore (see below)
>>
>>>> /* snprintf on Linux returns -1 on overflows */
>>>
>>> ...
>>>>
>>>> if (sz >= static_cast<int>(store_->spaceSize()))
>>>> rawSpace(sz*2); // TODO: tune heuristics
>>>> else if (sz < 0) // output error in vsnprintf
>>>> throw TextException("output error in vsnprintf",__FILE__,
>>>> __LINE__);
>>>
>>> If snprintf on Linux returns -1 on overflows, we should not throw when
>>> sz is -1. We should grow the buffer instead, right?
>>
>> Man snprintf(3) says
>> If an output error is encountered, a negative value is returned.
>>
>> I guess the comment is broken.
>> vsnprintf is standard as of C99; it should be therefore quite
>> consistent in our build environment.
>
>
> What other types of outt error than buffer overflow can occur?
> Might be worth a little test to see what overflow produces exactly. And I
> agree with Alex that growing is the better way to handle it, on the proviso
> that we can reliably detect that overflow.

Overflow is signaled by returning a size greater than the buffer. The
freebsd man page mentions possible errors in write(2) ( does not apply
here) but also ENOMEM or EILSEQ (illegal wide character).

  Kinkie
Received on Wed Jul 31 2013 - 12:58:13 MDT