This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v4] Add reallocarray function.


On 10.05.2017 23:29, DJ Delorie wrote:
> 
> Dennis Wlfing <denniswoelfing@gmx.de> writes:
>> The reallocarray function is an extension from OpenBSD.
> 
> And seems to be being adopted by many other OSs, too.
> 
>> Tested on x86_64-linux.
> 
> Could you test on a 32-bit system also?

I will test it on a i686 system.

> This declaration:
> 
>> diff --git a/include/stdlib.h b/include/stdlib.h
>> +extern void *__libc_reallocarray (void *__ptr, size_t __nmemb, size_t __size)
>> +     __attribute_warn_unused_result__;
> 
> Doesn't match this one:
> 
>> diff --git a/malloc/malloc.h b/malloc/malloc.h
>> +extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
>> +__THROW __attribute_warn_unused_result__;
> 
> or this one:
> 
>> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
>>  extern void *realloc (void *__ptr, size_t __size)
>>       __THROW __attribute_warn_unused_result__;
> 
> which don't match the definition:
> 
>> diff --git a/malloc/reallocarray.c b/malloc/reallocarray.c
>> +void *
>> +__libc_reallocarray (void *optr, size_t nmemb, size_t elem_size)

The header include/stdlib.h (not to be confused with stdlib/stdlib.h) is
an internal glibc header. It declares the function prefixed with __libc_
so that other glibc functions can call it without causing namespace
problems. In the public headers only reallocarray is declared which is a
weak alias for __libc_reallocarray.

>> diff --git a/malloc/malloc-private.h b/malloc/malloc-private.h
> 
> I'm confused; we have a malloc-internal.h which doesn't look like it
> needs to be included for malloc.h to work; why can't we fix that and use
> it, instead of adding a second "internal" header?
> 
> (traditionally, we put private things in malloc.c, which includes other
> .c files, which is IMHO wrong, but that's a problem for another day ;)

The check_mul_overflow_size_t function was put in the new
malloc-private.h header because malloc-internal.h is also used in other
parts of glibc.
The reason why it is in a header and not just in reallocarray.c is that
this function can also be useful in other functions. Florian Weimer
currently has two pending patches that also use an equivalent function
and then these patches could just include that header:
https://sourceware.org/ml/libc-alpha/2017-04/msg00498.html
https://sourceware.org/ml/libc-alpha/2017-04/msg00501.html

>> +#include <malloc/malloc-internal.h>
> 
> There doesn't seem to be anything in malloc-internal.h that this header
> needs...

Right, previous versions of this patch used INTERNAL_SIZE_T from
malloc-internal.h but this is not used anymore. I will remove this include.

>> +static inline bool
>> +check_mul_overflow_size_t (size_t l, size_t r, size_t *result)
> 
> A short comment here that documents the function (returns true/false
> when, sets results when) is appropriate.

I will add one.

>> +void *
>> +__libc_reallocarray (void *optr, size_t nmemb, size_t elem_size)
>> +{
>> +  size_t bytes;
>> +  if (check_mul_overflow_size_t (nmemb, elem_size, &bytes))
> 
> Do we want a __glibc_unlikely() here?

Since check_mul_overflow_size_t is an inline function, I guess the
compiler should be smart enough to get the branch prediction right.
But I could probably add another __glibc_unlikely() around the
r != 0 && *result / r != l condition in check_mul_overflow_size_t.

>> +  /* Test realloc-like behavior.  */
>> +  /* Allocate memory like malloc.  */
>> +  ptr = reallocarray (NULL, 10, 2);
>> +  TEST_VERIFY_EXIT (ptr);
> 
> We should also call __malloc_usable_size() to make sure it's at least
> what we expect.

I will add this.

>> +  /* Enlarge buffer.   */
>> +  ptr2 = reallocarray (ptr, 20, 2);
>> +  TEST_VERIFY (ptr2);
>> +  if (ptr2)
>> +    ptr = ptr2;
> 
> Likewise, etc.
> 
>> +  /* Overflow should leave buffer untouched.  */
> 
> This requirement isn't documented anywhere...

The reallocarray documentation says that reallocarray is equivalent to
realloc(ptr, nmemb*size) and the realloc documentation says it does not
modify the memory block on failure.
But this could probably be documented more explicitly.

>> +  /* Free buffer (glibc).  */
> 
> That is not what the BSD version does... realloc of size zero returns a
> pointer, and the Austin Group has decided realloc(ptr,0) is not free().
> While this might be what we do, do we want to enshrine that in a test
> case?
> 
> Sine the original definition of this function comes from BSD and we have
> no historical reason to violate C99 or the Austin Group, we should have
> reallocarray (ptr,0,0) act like those instead of our own historic
> realloc().

I think reallocarray should just remain a wrapper for realloc that
checks for overflow. So reallocarray(ptr,0,0) should behave like
realloc(ptr,0*0).
I also don't like the realloc(ptr,0) behavior in glibc but it seems like
the C committee wants to allow the glibc behavior by making behavior
implementation defined when size is 0.
http://open-std.org/JTC1/SC22/WG14/www/docs/dr_400.htm
When realloc(ptr,0) is allowed to free ptr reallocarray(ptr,0,0) should
also be.

However I will remove this test because freeing with reallocarray is not
a portable.

>> +  errno = 0;
>> +  ptr2 = reallocarray (ptr, 0, 0);
>> +  TEST_VERIFY (!ptr2);
>> +
>> +  free (ptr2);
> 
> Intentional double-free here?  (if so, add a comment... if not, add a
> comment ;)

Since I will remove the test above this will no longer be a double free.

>> +@comment malloc.h stdlib.h
>> +@comment BSD
>> +@deftypefun {void *} reallocarray (void *@var{ptr}, size_t @var{nmemb}, size_t @var{size})
>> +@safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@aculock{} @acsfd{} @acsmem{}}}
>> +
>> +The @code{reallocarray} function changes the size of the block whose address
>> +is @var{ptr} to be long enough to contain a vector of @var{nmemb} elements,
>> +each of size @var{size}.  It is equivalent to @samp{realloc (@var{ptr},
>> +@var{nmemb} * @var{size})}, except that @code{reallocarray} fails if the
>> +multiplication overflows.
>> +
>> +@code{reallocarray} should be used instead of @code{realloc} when the new size
>> +of the allocated block is the result of a multiplication that might overflow.
>> +@end deftypefun
> 
> This chunk appears right before a paragraph about realloc:
> 
>>  Like @code{malloc}, @code{realloc} may return a null pointer if no
>>  memory space is available to make the block bigger.  When this happens,
>>  the original block is untouched; it has not been modified or relocated.
> 
> Is there a better way to add that chunk of text?  Or mention
> reallocarray in the surrounding text so it integrates better?

I guess I could change most occurrences of @code{realloc} in that
section by @code{realloc} and @code{reallocarray}.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]