This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] malloc: Fix for infinite loop in memalign/posix_memalign.
- From: Will Newton <will dot newton at linaro dot org>
- To: libc-alpha <libc-alpha at sourceware dot org>
- Cc: Patch Tracking <patches at linaro dot org>
- Date: Tue, 29 Oct 2013 15:59:21 -0700
- Subject: Re: [PATCH] malloc: Fix for infinite loop in memalign/posix_memalign.
- Authentication-results: sourceware.org; auth=none
- References: <52569CED dot 3060700 at linaro dot org> <CANu=DmjvNT8j7_WJMGZhEeXOVcS7io+1wenDv0ei4PJZ6Xvd7A at mail dot gmail dot com>
On 17 October 2013 00:42, Will Newton <will.newton@linaro.org> wrote:
> On 10 October 2013 13:26, Will Newton <will.newton@linaro.org> 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.
>>
>> 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;
>> + }
>> +
>> /* 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;
>> + }
>> +
>> /* 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");
>> +
>> + free (p);
>> +
>> /* 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;
>> +
>> /* 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);
>> --
>> 1.8.1.4
>>
>
> Ping?
Ping? Any further comments on this or should I go ahead and commit?
Thanks,
--
Will Newton
Toolchain Working Group, Linaro