[PATCH] manual: remove an obsolete requirement on aligned_alloc() usage

[PATCH] manual: remove an obsolete requirement on aligned_alloc() usage

[John Scott]

[-- Attachment #1.1: Type: text/plain, Size: 35 bytes --]

I'm not subscribed; please CC me.

[-- Attachment #1.2: 0001-manual-remove-an-obsolete-requirement-on-aligned_all.patch --]
[-- Type: text/x-patch, Size: 1579 bytes --]

From 315c821421a2279d77e846bf927b7c4107e32d43 Mon Sep 17 00:00:00 2001
From: John Scott <jscott@posteo.net>
Date: Thu, 21 Oct 2021 05:20:55 -0400
Subject: [PATCH] manual: remove an obsolete requirement on aligned_alloc()
 usage

The C11 standard made it undefined behavior if the size of
the allocation was not a multiple of the page size. As discussed
at BZ #20137, changes to the standard were proposed and
subsequently adopted in Defect Report 460.

In particular, the following sentence
"The value of alignment shall be a valid alignment supported by
the implementation and the value of size shall be an integral
multiple of alignment."
was changed to
"If the value of alignment is not a valid alignment supported by
the implementation the function shall fail by returning a null pointer."

The DR is at
http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_460
---
 manual/memory.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/manual/memory.texi b/manual/memory.texi
index 0b2b9c87..5c16f7ae 100644
--- a/manual/memory.texi
+++ b/manual/memory.texi
@@ -995,7 +995,7 @@ power of two than that, use @code{aligned_alloc} or @code{posix_memalign}.
 @c Alias to memalign.
 The @code{aligned_alloc} function allocates a block of @var{size} bytes whose
 address is a multiple of @var{alignment}.  The @var{alignment} must be a
-power of two and @var{size} must be a multiple of @var{alignment}.
+power of two.
 
 The @code{aligned_alloc} function returns a null pointer on error and sets
 @code{errno} to one of the following values:
-- 
2.33.0


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH] manual: remove an obsolete requirement on aligned_alloc() usage

[Alejandro Colomar (man-pages)]

Hi John,

On 10/21/21 11:28 AM, John Scott via Libc-alpha wrote:
> 0001-manual-remove-an-obsolete-requirement-on-aligned_all.patch
> 
>  From 315c821421a2279d77e846bf927b7c4107e32d43 Mon Sep 17 00:00:00 2001
> From: John Scott<jscott@posteo.net>
> Date: Thu, 21 Oct 2021 05:20:55 -0400
> Subject: [PATCH] manual: remove an obsolete requirement on aligned_alloc()
>   usage
> 
> The C11 standard made it undefined behavior if the size of
> the allocation was not a multiple of the page size. As discussed
> at BZ #20137, changes to the standard were proposed and
> subsequently adopted in Defect Report 460.

Was that fixed in C17?  Or in (yet unreleased) C2X?

> 
> In particular, the following sentence
> "The value of alignment shall be a valid alignment supported by
> the implementation and the value of size shall be an integral
> multiple of alignment."
> was changed to
> "If the value of alignment is not a valid alignment supported by
> the implementation the function shall fail by returning a null pointer."

Did any implementation make use of that Undefined Behavior prior to its 
fix?  If so, at least in the man-pages, we should keep a historical note 
to warn about it.

Thanks,

Alex

> 
> The DR is at
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_460
> ---
>   manual/memory.texi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/manual/memory.texi b/manual/memory.texi
> index 0b2b9c87..5c16f7ae 100644
> --- a/manual/memory.texi
> +++ b/manual/memory.texi
> @@ -995,7 +995,7 @@ power of two than that, use @code{aligned_alloc} or @code{posix_memalign}.
>   @c Alias to memalign.
>   The @code{aligned_alloc} function allocates a block of @var{size} bytes whose
>   address is a multiple of @var{alignment}.  The @var{alignment} must be a
> -power of two and @var{size} must be a multiple of @var{alignment}.
> +power of two.
>   
>   The @code{aligned_alloc} function returns a null pointer on error and sets
>   @code{errno} to one of the following values:
> -- 2.33.0


-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH] manual: remove an obsolete requirement on aligned_alloc() usage

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

On 10/21/21 3:28 AM, John Scott via Libc-alpha wrote:
> I'm not subscribed; please CC me.
> 

Carlos asked me offline to comment on this change.  I'm not
a Glibc expert so I'll do my best but I defer to others here
who know the allocator much better.

diff --git a/manual/memory.texi b/manual/memory.texi
index 0b2b9c87..5c16f7ae 100644
--- a/manual/memory.texi
+++ b/manual/memory.texi
@@ -995,7 +995,7 @@ power of two than that, use @code{aligned_alloc} or 
@code{posix_memalign}.
  @c Alias to memalign.
  The @code{aligned_alloc} function allocates a block of @var{size} 
bytes whose
  address is a multiple of @var{alignment}.  The @var{alignment} must be a
-power of two and @var{size} must be a multiple of @var{alignment}.
+power of two.

Based on my code inspection of _mid_memalign in malloc.c I think
a change that would more accurately reflect the implementation
is one that described that when alignment is not a power of two
it's bumped up to next larger power of two, and adjusted
the EINVAL condition appropriately.  Attached is a diff along
these lines.  Please feel free to use it as you see fit.

Martin

[-- Attachment #2: glibc-doc-20137.diff --]
[-- Type: text/x-patch, Size: 1293 bytes --]

diff --git a/manual/memory.texi b/manual/memory.texi
index 0b2b9c8795..9e407cd44a 100644
--- a/manual/memory.texi
+++ b/manual/memory.texi
@@ -994,8 +994,8 @@ power of two than that, use @code{aligned_alloc} or @code{posix_memalign}.
 @safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@aculock{} @acsfd{} @acsmem{}}}
 @c Alias to memalign.
 The @code{aligned_alloc} function allocates a block of @var{size} bytes whose
-address is a multiple of @var{alignment}.  The @var{alignment} must be a
-power of two and @var{size} must be a multiple of @var{alignment}.
+address is a multiple of @var{adjusted-alignment}.  The value of @var{adjusted-alignment}
+is the smallest power of two that's greater than or equal to @var{alignment}.
 
 The @code{aligned_alloc} function returns a null pointer on error and sets
 @code{errno} to one of the following values:
@@ -1005,7 +1005,7 @@ The @code{aligned_alloc} function returns a null pointer on error and sets
 There was insufficient memory available to satisfy the request.
 
 @item EINVAL
-@var{alignment} is not a power of two.
+The value of @var{alignment} is greater than @code{SIZE_MAX / 2 + 1}.
 
 This function was introduced in @w{ISO C11} and hence may have better
 portability to modern non-POSIX systems than @code{posix_memalign}.

Re: [PATCH] manual: remove an obsolete requirement on aligned_alloc() usage

[Martin Sebor]

On 10/25/21 9:45 AM, Martin Sebor wrote:
> On 10/21/21 3:28 AM, John Scott via Libc-alpha wrote:
>> I'm not subscribed; please CC me.
>>
> 
> Carlos asked me offline to comment on this change.  I'm not
> a Glibc expert so I'll do my best but I defer to others here
> who know the allocator much better.
> 
> diff --git a/manual/memory.texi b/manual/memory.texi
> index 0b2b9c87..5c16f7ae 100644
> --- a/manual/memory.texi
> +++ b/manual/memory.texi
> @@ -995,7 +995,7 @@ power of two than that, use @code{aligned_alloc} or 
> @code{posix_memalign}.
>   @c Alias to memalign.
>   The @code{aligned_alloc} function allocates a block of @var{size} 
> bytes whose
>   address is a multiple of @var{alignment}.  The @var{alignment} must be a
> -power of two and @var{size} must be a multiple of @var{alignment}.
> +power of two.
> 
> Based on my code inspection of _mid_memalign in malloc.c I think
> a change that would more accurately reflect the implementation
> is one that described that when alignment is not a power of two
> it's bumped up to next larger power of two, and adjusted
> the EINVAL condition appropriately.  Attached is a diff along
> these lines.  Please feel free to use it as you see fit.

Also, to answer the follow-up question in Patchwork (I can't find
it in my mailbox):

   Was that fixed in C17?  Or in (yet unreleased) C2X?

I believe the DR460 change went into C17.  Here's an early C2x
draft from 2018 publicly available on the WG14 site that should
still be very close to C17:

   http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf

> 
> Martin

Re: [PATCH] manual: remove an obsolete requirement on aligned_alloc() usage

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

On 10/25/21 08:45, Martin Sebor via Libc-alpha wrote:
> Based on my code inspection of _mid_memalign in malloc.c I think
> a change that would more accurately reflect the implementation
> is one that described that when alignment is not a power of two
> it's bumped up to next larger power of two, and adjusted
> the EINVAL condition appropriately.

Let's not document this implementation detail, as it might tie us down 
unnecessarily in the future.

> I believe the DR460 change went into C17.  Here's an early C2x
> draft from 2018 publicly available on the WG14 site that should
> still be very close to C17:
> 
>   http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf

This says that aligned_alloc "shall fail" if the alignment is not a 
valid alignment supported by the implementation. This means the current 
glibc alloc_aligned behavior does not conform to C17 and should be fixed 
so that it does. Something like the attached (untested) patch, say.

[-- Attachment #2: align.patch --]
[-- Type: text/x-patch, Size: 1155 bytes --]

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 2ba1fee144..386fb5a4cb 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3416,6 +3416,12 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
   mstate ar_ptr;
   void *p;
 
+  if (!powerof2 (alignment))
+    {
+      __set_errno (EINVAL);
+      return 0;
+    }
+
   /* If we need less alignment than we give anyway, just relay to malloc.  */
   if (alignment <= MALLOC_ALIGNMENT)
     return __libc_malloc (bytes);
@@ -3424,24 +3430,6 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
   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;
-    }
-
-
-  /* Make sure alignment is power of 2.  */
-  if (!powerof2 (alignment))
-    {
-      size_t a = MALLOC_ALIGNMENT * 2;
-      while (a < alignment)
-        a <<= 1;
-      alignment = a;
-    }
-
   if (SINGLE_THREAD_P)
     {
       p = _int_memalign (&main_arena, alignment, bytes);

Re: [PATCH] manual: remove an obsolete requirement on aligned_alloc() usage

[Martin Sebor]

On 10/25/21 7:50 PM, Paul Eggert wrote:
> On 10/25/21 08:45, Martin Sebor via Libc-alpha wrote:
>> Based on my code inspection of _mid_memalign in malloc.c I think
>> a change that would more accurately reflect the implementation
>> is one that described that when alignment is not a power of two
>> it's bumped up to next larger power of two, and adjusted
>> the EINVAL condition appropriately.
> 
> Let's not document this implementation detail, as it might tie us down 
> unnecessarily in the future.
> 
>> I believe the DR460 change went into C17.  Here's an early C2x
>> draft from 2018 publicly available on the WG14 site that should
>> still be very close to C17:
>>
>>   http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf
> 
> This says that aligned_alloc "shall fail" if the alignment is not a 
> valid alignment supported by the implementation. This means the current 
> glibc alloc_aligned behavior does not conform to C17 and should be fixed 
> so that it does. Something like the attached (untested) patch, say.

I agree.  I took the term "valid alignment" here to mean one
specifically accepted by aligned_alloc() but 6.2.8 is explicit
that

   "Every valid alignment value shall be a nonnegative integral
   power of two."

so although implementations get to decide on the set of valid
extended alignments they're not allowed to be accept ones that
are in conflict with the blanket requirement above.

Martin