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

On 1/08/2013 12:58 a.m., Kinkie wrote:
> 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

minSpace is controlled completely by the unknown caller code. It may be
UINT_MAX or something equally capable of overflowing when you add to it.

I think the right check for here is:
   if (maxSize - length() < minSpace || minSpace < 0) abort().

The math being done on the two values we are confident about already and
ensuring that the leftover space is enough for the caller.
The extra 0 check disappearing when we move to unsigned of course.

Amos
Received on Wed Jul 31 2013 - 14:17:21 MDT