[1/2] malloc/malloc.c: Validate SIZE passed to aligned_alloc.

Message ID 527BD0C3.4010607@linaro.org
State Rejected
Headers show

Commit Message

Will Newton Nov. 7, 2013, 5:41 p.m.
The ISO C11 standard specifies that a SIZE passed to aligned_alloc
must be a multiple of ALIGNMENT. Aliasing aligned_alloc to memalign
does not enforce this restriction, so create a new function that
does this validation.

ChangeLog:

2013-11-07  Will Newton  <will.newton@linaro.org>

	* malloc/malloc.c (__aligned_alloc): New function.
	(__libc_memalign): Move main body of the code to
	_int_aligned_alloc and call that function.
	(_int_aligned_alloc): New function.
---
 malloc/malloc.c | 97 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 60 insertions(+), 37 deletions(-)

Comments

Paul Eggert Nov. 7, 2013, 5:48 p.m. | #1
On 11/07/2013 09:41 AM, Will Newton wrote:
> The ISO C11 standard specifies that a SIZE passed to aligned_alloc
> must be a multiple of ALIGNMENT. Aliasing aligned_alloc to memalign
> does not enforce this restriction, so create a new function that
> does this validation.

This doesn't look right.  See the NEWS file's entry for glibc 2.16, which says:

  + aligned_alloc.  NB: The code is deliberately allows the size parameter
    to not be a multiple of the alignment.  This is a moronic requirement
    in the standard but it is only a requirement on the caller, not the
    implementation.
Will Newton Nov. 7, 2013, 8:09 p.m. | #2
On 7 November 2013 17:48, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 11/07/2013 09:41 AM, Will Newton wrote:
>> The ISO C11 standard specifies that a SIZE passed to aligned_alloc
>> must be a multiple of ALIGNMENT. Aliasing aligned_alloc to memalign
>> does not enforce this restriction, so create a new function that
>> does this validation.
>
> This doesn't look right.  See the NEWS file's entry for glibc 2.16, which says:
>
>   + aligned_alloc.  NB: The code is deliberately allows the size parameter
>     to not be a multiple of the alignment.  This is a moronic requirement
>     in the standard but it is only a requirement on the caller, not the
>     implementation.

I disagree with Drepper on this point. If we don't enforce the
contract on callers then it becomes possible for callers to write
non-portable code with glibc aligned_alloc. Admittedly the spec of
aligned_alloc isn't amazingly rigid so writing non-portable code is
possible anyway, but I still think it is worth glibc validating what
is actually written in the spec. If we want to write a function that
implements "almost aligned_alloc" it should really be called something
else IMO.
Rich Felker Nov. 8, 2013, 4:20 a.m. | #3
On Thu, Nov 07, 2013 at 08:09:24PM +0000, Will Newton wrote:
> On 7 November 2013 17:48, Paul Eggert <eggert@cs.ucla.edu> wrote:
> > On 11/07/2013 09:41 AM, Will Newton wrote:
> >> The ISO C11 standard specifies that a SIZE passed to aligned_alloc
> >> must be a multiple of ALIGNMENT. Aliasing aligned_alloc to memalign
> >> does not enforce this restriction, so create a new function that
> >> does this validation.
> >
> > This doesn't look right.  See the NEWS file's entry for glibc 2.16, which says:
> >
> >   + aligned_alloc.  NB: The code is deliberately allows the size parameter
> >     to not be a multiple of the alignment.  This is a moronic requirement
> >     in the standard but it is only a requirement on the caller, not the
> >     implementation.
> 
> I disagree with Drepper on this point. If we don't enforce the
> contract on callers then it becomes possible for callers to write
> non-portable code with glibc aligned_alloc. Admittedly the spec of
> aligned_alloc isn't amazingly rigid so writing non-portable code is
> possible anyway, but I still think it is worth glibc validating what
> is actually written in the spec. If we want to write a function that
> implements "almost aligned_alloc" it should really be called something
> else IMO.

I'm against unnecessary and (mildly) expensive validation of a
condition that the implementation is not required to validate and for
which the check has no purpose except for intentionally breaking
non-portable code.

Rich
Will Newton Nov. 8, 2013, 10:20 a.m. | #4
On 8 November 2013 04:20, Rich Felker <dalias@aerifal.cx> wrote:
> On Thu, Nov 07, 2013 at 08:09:24PM +0000, Will Newton wrote:
>> On 7 November 2013 17:48, Paul Eggert <eggert@cs.ucla.edu> wrote:
>> > On 11/07/2013 09:41 AM, Will Newton wrote:
>> >> The ISO C11 standard specifies that a SIZE passed to aligned_alloc
>> >> must be a multiple of ALIGNMENT. Aliasing aligned_alloc to memalign
>> >> does not enforce this restriction, so create a new function that
>> >> does this validation.
>> >
>> > This doesn't look right.  See the NEWS file's entry for glibc 2.16, which says:
>> >
>> >   + aligned_alloc.  NB: The code is deliberately allows the size parameter
>> >     to not be a multiple of the alignment.  This is a moronic requirement
>> >     in the standard but it is only a requirement on the caller, not the
>> >     implementation.
>>
>> I disagree with Drepper on this point. If we don't enforce the
>> contract on callers then it becomes possible for callers to write
>> non-portable code with glibc aligned_alloc. Admittedly the spec of
>> aligned_alloc isn't amazingly rigid so writing non-portable code is
>> possible anyway, but I still think it is worth glibc validating what
>> is actually written in the spec. If we want to write a function that
>> implements "almost aligned_alloc" it should really be called something
>> else IMO.
>
> I'm against unnecessary and (mildly) expensive validation of a
> condition that the implementation is not required to validate and for
> which the check has no purpose except for intentionally breaking
> non-portable code.

My initial interest in this came from documenting the aligned_alloc
interface. So should we document this non-standard behaviour?
Ondřej Bílka Nov. 8, 2013, 10:30 a.m. | #5
On Fri, Nov 08, 2013 at 10:20:29AM +0000, Will Newton wrote:
> On 8 November 2013 04:20, Rich Felker <dalias@aerifal.cx> wrote:
> > On Thu, Nov 07, 2013 at 08:09:24PM +0000, Will Newton wrote:
> >> On 7 November 2013 17:48, Paul Eggert <eggert@cs.ucla.edu> wrote:
> >> > On 11/07/2013 09:41 AM, Will Newton wrote:
> >> >> The ISO C11 standard specifies that a SIZE passed to aligned_alloc
> >> >> must be a multiple of ALIGNMENT. Aliasing aligned_alloc to memalign
> >> >> does not enforce this restriction, so create a new function that
> >> >> does this validation.
> >> >
> >> > This doesn't look right.  See the NEWS file's entry for glibc 2.16, which says:
> >> >
> >> >   + aligned_alloc.  NB: The code is deliberately allows the size parameter
> >> >     to not be a multiple of the alignment.  This is a moronic requirement
> >> >     in the standard but it is only a requirement on the caller, not the
> >> >     implementation.
> >>
> >> I disagree with Drepper on this point. If we don't enforce the
> >> contract on callers then it becomes possible for callers to write
> >> non-portable code with glibc aligned_alloc. Admittedly the spec of
> >> aligned_alloc isn't amazingly rigid so writing non-portable code is
> >> possible anyway, but I still think it is worth glibc validating what
> >> is actually written in the spec. If we want to write a function that
> >> implements "almost aligned_alloc" it should really be called something
> >> else IMO.
> >
> > I'm against unnecessary and (mildly) expensive validation of a
> > condition that the implementation is not required to validate and for
> > which the check has no purpose except for intentionally breaking
> > non-portable code.
> 
> My initial interest in this came from documenting the aligned_alloc
> interface. So should we document this non-standard behaviour?
> 
I would call this implementation detail, so adding comment in
aligned_alloc code is appropriate, in user documentation you risk that
users will start using this behaviour.

I would be also ok if we changed behaviour to rounding size up to nearest
multiple.
Joseph Myers Nov. 8, 2013, 1:42 p.m. | #6
On Fri, 8 Nov 2013, Will Newton wrote:

> > I'm against unnecessary and (mildly) expensive validation of a
> > condition that the implementation is not required to validate and for
> > which the check has no purpose except for intentionally breaking
> > non-portable code.
> 
> My initial interest in this came from documenting the aligned_alloc
> interface. So should we document this non-standard behaviour?

I say document only what the standard requires, but don't add extra 
validation - the general practice in glibc is not to make glibc slower or 
bigger for valid code by checking for conditions that can only arise when 
the user's call to a glibc function means undefined behavior (this is much 
the same as not checking for NULL arguments when a function's interface 
doesn't allow them, for example).
Will Newton Nov. 8, 2013, 2 p.m. | #7
On 8 November 2013 13:42, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Fri, 8 Nov 2013, Will Newton wrote:
>
>> > I'm against unnecessary and (mildly) expensive validation of a
>> > condition that the implementation is not required to validate and for
>> > which the check has no purpose except for intentionally breaking
>> > non-portable code.
>>
>> My initial interest in this came from documenting the aligned_alloc
>> interface. So should we document this non-standard behaviour?
>
> I say document only what the standard requires, but don't add extra
> validation - the general practice in glibc is not to make glibc slower or
> bigger for valid code by checking for conditions that can only arise when
> the user's call to a glibc function means undefined behavior (this is much
> the same as not checking for NULL arguments when a function's interface
> doesn't allow them, for example).

At the moment we check for non-power-of-two alignments which are user
error. aligned_alloc will return NULL with EINVAL in this case, but
not in the case of size not being a multiple of alignment. It seems
awkward to document that aligned_alloc returns error and EINVAL for
one case without explicitly pointing out that it doesn't for the other
(after specifying that aligned_alloc has this requirement).

If the docs are ok as is then I'll drop this patch and respin a test
patch without the requirement to test for this case.
Rich Felker Nov. 8, 2013, 6:03 p.m. | #8
On Fri, Nov 08, 2013 at 10:20:29AM +0000, Will Newton wrote:
> On 8 November 2013 04:20, Rich Felker <dalias@aerifal.cx> wrote:
> > On Thu, Nov 07, 2013 at 08:09:24PM +0000, Will Newton wrote:
> >> On 7 November 2013 17:48, Paul Eggert <eggert@cs.ucla.edu> wrote:
> >> > On 11/07/2013 09:41 AM, Will Newton wrote:
> >> >> The ISO C11 standard specifies that a SIZE passed to aligned_alloc
> >> >> must be a multiple of ALIGNMENT. Aliasing aligned_alloc to memalign
> >> >> does not enforce this restriction, so create a new function that
> >> >> does this validation.
> >> >
> >> > This doesn't look right.  See the NEWS file's entry for glibc 2.16, which says:
> >> >
> >> >   + aligned_alloc.  NB: The code is deliberately allows the size parameter
> >> >     to not be a multiple of the alignment.  This is a moronic requirement
> >> >     in the standard but it is only a requirement on the caller, not the
> >> >     implementation.
> >>
> >> I disagree with Drepper on this point. If we don't enforce the
> >> contract on callers then it becomes possible for callers to write
> >> non-portable code with glibc aligned_alloc. Admittedly the spec of
> >> aligned_alloc isn't amazingly rigid so writing non-portable code is
> >> possible anyway, but I still think it is worth glibc validating what
> >> is actually written in the spec. If we want to write a function that
> >> implements "almost aligned_alloc" it should really be called something
> >> else IMO.
> >
> > I'm against unnecessary and (mildly) expensive validation of a
> > condition that the implementation is not required to validate and for
> > which the check has no purpose except for intentionally breaking
> > non-portable code.
> 
> My initial interest in this came from documenting the aligned_alloc
> interface. So should we document this non-standard behaviour?

No. It's not non-standard behavior. It's undefined behavior on the
part of the caller.

Rich
Rich Felker Nov. 8, 2013, 6:09 p.m. | #9
On Fri, Nov 08, 2013 at 02:00:10PM +0000, Will Newton wrote:
> On 8 November 2013 13:42, Joseph S. Myers <joseph@codesourcery.com> wrote:
> > On Fri, 8 Nov 2013, Will Newton wrote:
> >
> >> > I'm against unnecessary and (mildly) expensive validation of a
> >> > condition that the implementation is not required to validate and for
> >> > which the check has no purpose except for intentionally breaking
> >> > non-portable code.
> >>
> >> My initial interest in this came from documenting the aligned_alloc
> >> interface. So should we document this non-standard behaviour?
> >
> > I say document only what the standard requires, but don't add extra
> > validation - the general practice in glibc is not to make glibc slower or
> > bigger for valid code by checking for conditions that can only arise when
> > the user's call to a glibc function means undefined behavior (this is much
> > the same as not checking for NULL arguments when a function's interface
> > doesn't allow them, for example).
> 
> At the moment we check for non-power-of-two alignments which are user
> error.

This code is required for posix_memalign anyway. It's not formally
required for memalign (which has no standard dictating its behavior),
but if glibc's memalign wants to accept non-power-of-two alignments,
the subsequent code would have to handle them correctly, and it
doesn't. I suspect historical implementations either reject or support
non-power-of-two alignments rather than blowing up, so from a
compatibility standpoint, the current behavior should be preserved
anyway.

Rich

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 897c43a..67ad141 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1054,6 +1054,7 @@  static void     _int_free(mstate, mchunkptr, int);
 static void*  _int_realloc(mstate, mchunkptr, INTERNAL_SIZE_T,
 			   INTERNAL_SIZE_T);
 static void*  _int_memalign(mstate, size_t, size_t);
+static void*  _int_aligned_alloc(size_t, size_t);
 static void*  _int_valloc(mstate, size_t);
 static void*  _int_pvalloc(mstate, size_t);
 static void malloc_printerr(int action, const char *str, void *ptr);
@@ -3000,56 +3001,34 @@  __libc_realloc(void* oldmem, size_t bytes)
 libc_hidden_def (__libc_realloc)

 void*
-__libc_memalign(size_t alignment, size_t bytes)
+__aligned_alloc(size_t alignment, size_t bytes)
 {
-  mstate ar_ptr;
-  void *p;
-
   void *(*hook) (size_t, size_t, const void *) =
     force_reg (__memalign_hook);
   if (__builtin_expect (hook != NULL, 0))
     return (*hook)(alignment, bytes, RETURN_ADDRESS (0));

-  /* If need less alignment than we give anyway, just relay to malloc */
-  if (alignment <= MALLOC_ALIGNMENT) return __libc_malloc(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)
+  /* Check size is integral multiple of alignment.  */
+  if (bytes % alignment != 0)
     {
       __set_errno (EINVAL);
       return 0;
     }

-  /* Check for overflow.  */
-  if (bytes > SIZE_MAX - alignment - MINSIZE)
-    {
-      __set_errno (ENOMEM);
-      return 0;
-    }
+  return _int_aligned_alloc(alignment, bytes);
+}
+weak_alias (__aligned_alloc, aligned_alloc)

-  arena_get(ar_ptr, bytes + alignment + MINSIZE);
-  if(!ar_ptr)
-    return 0;
-  p = _int_memalign(ar_ptr, alignment, bytes);
-  if(!p) {
-    LIBC_PROBE (memory_memalign_retry, 2, bytes, alignment);
-    ar_ptr = arena_get_retry (ar_ptr, bytes);
-    if (__builtin_expect(ar_ptr != NULL, 1)) {
-      p = _int_memalign(ar_ptr, alignment, bytes);
-      (void)mutex_unlock(&ar_ptr->mutex);
-    }
-  } else
-    (void)mutex_unlock(&ar_ptr->mutex);
-  assert(!p || chunk_is_mmapped(mem2chunk(p)) ||
-	 ar_ptr == arena_for_chunk(mem2chunk(p)));
-  return p;
+void*
+__libc_memalign(size_t alignment, size_t bytes)
+{
+  void *(*hook) (size_t, size_t, const void *) =
+    force_reg (__memalign_hook);
+  if (__builtin_expect (hook != NULL, 0))
+    return (*hook)(alignment, bytes, RETURN_ADDRESS (0));
+
+  return _int_aligned_alloc(alignment, bytes);
 }
-/* For ISO C11.  */
-weak_alias (__libc_memalign, aligned_alloc)
 libc_hidden_def (__libc_memalign)

 void*
@@ -4404,6 +4383,50 @@  _int_memalign(mstate av, size_t alignment, size_t bytes)
   return chunk2mem(p);
 }

+static void *
+_int_aligned_alloc(size_t alignment, size_t bytes)
+{
+  mstate ar_ptr;
+  void *p;
+
+  /* If need less alignment than we give anyway, just relay to malloc */
+  if (alignment <= MALLOC_ALIGNMENT) return __libc_malloc(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)
+    {
+      __set_errno (ENOMEM);
+      return 0;
+    }
+
+  arena_get(ar_ptr, bytes + alignment + MINSIZE);
+  if(!ar_ptr)
+    return 0;
+  p = _int_memalign(ar_ptr, alignment, bytes);
+  if(!p) {
+    LIBC_PROBE (memory_memalign_retry, 2, bytes, alignment);
+    ar_ptr = arena_get_retry (ar_ptr, bytes);
+    if (__builtin_expect(ar_ptr != NULL, 1)) {
+      p = _int_memalign(ar_ptr, alignment, bytes);
+      (void)mutex_unlock(&ar_ptr->mutex);
+    }
+  } else
+    (void)mutex_unlock(&ar_ptr->mutex);
+  assert(!p || chunk_is_mmapped(mem2chunk(p)) ||
+	 ar_ptr == arena_for_chunk(mem2chunk(p)));
+  return p;
+}

 /*
   ------------------------------ valloc ------------------------------