| Message ID | 527BD0C3.4010607@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
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.
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.
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
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?
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.
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).
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.
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
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
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 ------------------------------