Re: [PATCH] malloc: Fix for infinite loop in memalign/posix_memalign.

["Carlos O'Donell" <carlos at redhat dot com>]

On 10/10/2013 08:26 AM, Will Newton wrote:
> A very large alignment argument passed to mealign/posix_memalign
> causes _int_memalign to enter an infinite loop. Limit the maximum
> alignment value to the maximum representable power of two to
> prevent this from happening.

OK to checkin.

This looks good to me, adds tests with comments, and detects an
invalid alignment which is required given the linux kernel man pages
documentation.

We still lack updated manual info for memalign and posix_memalign
where we should be covering what is returned as valid errno values
for these functions e.g. EINVAL and ENOMEM (that is to say that
manual/memory.texi doesn't say that memalign and posix_memalign
return any useful errors).
 
> Changelog:
> 
> 2013-10-10  Will Newton  <will.newton@linaro.org>
> 
> 	[BZ #16038]
> 	* malloc/hooks.c (memalign_check): Limit alignment to the
> 	maximum representable power of two.
> 	* malloc/malloc.c (__libc_memalign): Likewise.
> 	* malloc/tst-memalign.c (do_test): Add test for very
> 	large alignment values.
> 	* malloc/tst-posix_memalign.c (do_test): Likewise.
> ---
>  malloc/hooks.c              |  8 ++++++++
>  malloc/malloc.c             |  8 ++++++++
>  malloc/tst-memalign.c       | 15 +++++++++++++++
>  malloc/tst-posix_memalign.c | 10 ++++++++++
>  4 files changed, 41 insertions(+)
> 
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 3f663bb..1dbe93f 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -361,6 +361,14 @@ memalign_check(size_t alignment, size_t bytes, const void *caller)
>    if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL);
>    if (alignment <  MINSIZE) alignment = MINSIZE;
> 
> +  /* If the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a
> +     power of 2 and will cause overflow in the check below.  */
> +  if (alignment > SIZE_MAX / 2 + 1)
> +    {
> +      __set_errno (EINVAL);
> +      return 0;
> +    }
> +

OK.

>    /* Check for overflow.  */
>    if (bytes > SIZE_MAX - alignment - MINSIZE)
>      {
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 2938234..f1949f0 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3026,6 +3026,14 @@ __libc_memalign(size_t alignment, size_t bytes)
>    /* Otherwise, ensure that it is at least a minimum chunk size */
>    if (alignment <  MINSIZE) alignment = MINSIZE;
> 
> +  /* If the alignment is greater than SIZE_MAX / 2 + 1 it cannot be a
> +     power of 2 and will cause overflow in the check below.  */
> +  if (alignment > SIZE_MAX / 2 + 1)
> +    {
> +      __set_errno (EINVAL);
> +      return 0;
> +    }
> +

OK.

>    /* Check for overflow.  */
>    if (bytes > SIZE_MAX - alignment - MINSIZE)
>      {
> diff --git a/malloc/tst-memalign.c b/malloc/tst-memalign.c
> index 1c59752..b8eec9e 100644
> --- a/malloc/tst-memalign.c
> +++ b/malloc/tst-memalign.c
> @@ -70,6 +70,21 @@ do_test (void)
> 
>    free (p);
> 
> +  errno = 0;
> +
> +  /* Test to expose integer overflow in malloc internals from BZ #16038.  */
> +  p = memalign (-1, pagesize);
> +
> +  save = errno;
> +
> +  if (p != NULL)
> +    merror ("memalign (-1, pagesize) succeeded.");
> +
> +  if (p == NULL && save != EINVAL)
> +    merror ("memalign (-1, -pagesize) errno is not set correctly");

As Ondrej pointed out that should be "pagesize".

> +
> +  free (p);
> +

OK.

>    /* A zero-sized allocation should succeed with glibc, returning a
>       non-NULL value.  */
>    p = memalign (sizeof (void *), 0);
> diff --git a/malloc/tst-posix_memalign.c b/malloc/tst-posix_memalign.c
> index 27c0dd2..7f34e37 100644
> --- a/malloc/tst-posix_memalign.c
> +++ b/malloc/tst-posix_memalign.c
> @@ -65,6 +65,16 @@ do_test (void)
> 
>    p = NULL;
> 
> +  /* Test to expose integer overflow in malloc internals from BZ #16038.  */
> +  ret = posix_memalign (&p, -1, pagesize);
> +
> +  if (ret != EINVAL)
> +    merror ("posix_memalign (&p, -1, pagesize) succeeded.");
> +
> +  free (p);
> +
> +  p = NULL;
> +

OK.

>    /* A zero-sized allocation should succeed with glibc, returning zero
>       and setting p to a non-NULL value.  */
>    ret = posix_memalign (&p, sizeof (void *), 0);
> 

Cheers,
Carlos.