Re: [PATCH 07/17] malloc: Add specialized dynarray for C strings

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

On 14/06/2017 11:05, Florian Weimer wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> On 13/06/2017 11:24, Adhemerval Zanella wrote:
>>
>>> Here is an updated patch for the specialized dynarray.  I have incorporated all
>>> your suggestion but the skeleton name change (which I am not sure if it should
>>> follow the idea since it should be a complete 'api').  I also added some nonull
>>> attribute as for dynarray implementation.
>>
>> I rebase against the new begin/end additions and fixed some issues
>> regarding the make check (which I forgot to actually run before patch
>> submission...).
> 
> I don't see any begin/end references in the attached patch.

My mistake, I did not commit the two smalls changes that replace char_array_at (0)
with char_array_begin.

> 
>> +/* Replace the contents starting of position 'pos' of char_array 'array'
>> +   with the contents of string 'str' up to 'len' bytes.  A final '\0'
>> +   is appended in the string.  */
>> +__attribute__ ((nonnull (1, 3)))
>> +static bool
>> +char_array_replace_str_pos (struct char_array *array, size_t pos,
>> +                            const char *str, size_t len)
>> +{
>> +  if (pos > array->dynarray_header.used)
>> +    return false;
>> +
>> +  size_t newsize;
>> +  if (check_add_wrapv_size_t (pos, len, &newsize))
>> +    __libc_dynarray_overflow_failure (pos, len);
>> +  if (check_add_wrapv_size_t (newsize, 1, &newsize))
>> +    __libc_dynarray_overflow_failure (newsize, 1);
> 
> This is the opposite of what I expect: pos > array->dynarray_header.used
> appears to be a usage error, so this could result in __libc_fatal.
> Integer overflow while computing sizes for memory allocation is usually
> treated as a memory allocation failure, so it would expect a false
> return (and no __libc_fatal) for that.

So I think a better approach would just to use:

  if (pos > array->dynarray_header.used)
    __libc_dynarray_at_failure (char_array_size (array), pos);

  if (check_add_wrapv_size_t (pos, len, &newsize))
      ||check_add_wrapv_size_t (newsize, 1, &newsize))
    return false;

> 
> If you want to prevent access to the underlying char_array_* functions
> generated by dynarray, you could use #pragma GCC poison.
It could be a nice idea, although there is no direct usage on the glob
patchset.  I will check this out for a future enhancement.