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.


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?

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)

> 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 ;)

> +#include <malloc/malloc-internal.h>

There doesn't seem to be anything in malloc-internal.h that this header
needs...

> +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.

> +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?

> +  /* 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.

> +  /* 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...

> +  /* 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().

> +  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 ;)

> +@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?


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