Re: [PATCH] Allocation buffers for NSS result construction

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 21/06/2017 17:21, Florian Weimer wrote:
> On 06/21/2017 09:44 PM, Adhemerval Zanella wrote:
> 
>> Some functions like 'alloc_buffer_add_byte' does have a check to mark
>> the buffer failed, but 'alloc_buffer_size' will return a bogus value if
>> 'start' plus 'size' overflows.  It might not get caught in
>> 'alloc_buffer_alloc_bytes' for instance, where the 'length' can be
>> potentially lower than the returned value of 'alloc_buffer_size'
>> (since a signed ptrdiff_t will be converted to size_t).  Maybe to add
>> the same check from 'alloc_buffer_add_byte' might be suffice.
> 
> I added out-of-line process termination in the attached version.  I
> assume that's what you had in mind.
> 
>> Indeed, but check_add_wrapv_size_t (or check_add_overflow_size_t as you
>> suggested) might use a GCC builtin for newer compiler (which might use
>> more optimized instructions).
> 
> GCC recognizes the idiom and should optimize it just like the built-in.
> The value of the built-in primarily lies in the accurate, type-generic
> check (you get an overflow if the result does not match the result over
> the integers, irrespective of the types involved).
> 
>>> +/* Allocation buffers are used to carve out sub-allocations from a
>>> +   larger allocation.  Their primary application is in writing NSS
>>> +   modules, which recieve a caller-allocated buffer in which they are
>>
>> s/recieve/receive
> 
> Thanks, fixed.
> 
>>> +     void *heap_ptr;
>>> +     struct alloc_buffer buf = alloc_buffer_allocate
>>> +       (double_array_size * sizeof (double) + int_array_size * sizeof (int),
>>> +        &heap_ptr);
>>
>> I am not very found of this example because it does not check for
>> potentially overflow the size calculation.
> 
> I expanded the comment and explained why the initial overflow check is
> not needed.
> 
> Thanks,
> Florian
> 

LGTM, thanks.