Bug 20137 - aligned_alloc should fail for bad size / alignment [DR#460]
Summary: aligned_alloc should fail for bad size / alignment [DR#460]
Status: NEW
Alias: None
Product: glibc
Classification: Unclassified
Component: malloc (show other bugs)
Version: 2.23
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-24 12:02 UTC by Joseph Myers
Modified: 2016-05-27 08:21 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Myers 2016-05-24 12:02:50 UTC
The resolution to C11 DR#460 requires aligned_alloc to return NULL if the alignment is not valid (not a power of 2, in our case) or the size is not a multiple of the alignment.  This needs to be implemented.

For compatibility with existing binaries, I think we should have a compat symbol for aligned_alloc that remains as an alias to memalign (whose semantics should remain unchanged), and implement the stricter semantics only at a new symbol version.
Comment 1 Florian Weimer 2016-05-25 10:07:23 UTC
(In reply to Joseph Myers from comment #0)
> The resolution to C11 DR#460 requires aligned_alloc to return NULL if the
> alignment is not valid (not a power of 2, in our case) or the size is not a
> multiple of the alignment.  This needs to be implemented.

Do you have link to the rationale for the second requirement?  It does not make sense to me.

For example, when allocating space for a struct dirent object, you can end up with an allocation size which is not a multiple of the required alignment.  glibc itself uses a similar allocations in many other places.


I understand this bug is just about aligned_alloc (and not about lowering the alignment of pointers returned by malloc if the size happens to be odd, for example).  But this additional requirement for aligned_alloc seems to be rather strange.
Comment 2 jsm-csl@polyomino.org.uk 2016-05-25 10:59:45 UTC
On Wed, 25 May 2016, fweimer at redhat dot com wrote:

> --- Comment #1 from Florian Weimer <fweimer at redhat dot com> ---
> (In reply to Joseph Myers from comment #0)
> > The resolution to C11 DR#460 requires aligned_alloc to return NULL if the
> > alignment is not valid (not a power of 2, in our case) or the size is not a
> > multiple of the alignment.  This needs to be implemented.
> 
> Do you have link to the rationale for the second requirement?  It does not make
> sense to me.

I think requiring failure, given that this case is not defined, is about 
avoiding unnecessary undefined behavior.

I don't see anything in N1397 (the original addition of aligned_alloc) 
about why the size was required to be a multiple of the alignment in the 
first place; I'd guess it's general principles about sizes of types always 
being multiples of their alignment.  aligned_alloc would be expected to be 
used with extensions such as vector types, given that it's not needed for 
any standard type.
Comment 3 Florian Weimer 2016-05-25 11:11:41 UTC
To be clear, the use case I'm thinking about is allocating storage for an object of this type:

  struct flexible
  {
     _Decimal128 value;
     char formatted[];
  };

Doing something like this (with error checking) looks reasonable to me:

  size_t length = strlen (formatted);
  struct flexible *p = aligned_alloc (_Alignof (struct flexible),
                                      offsetof (struct flexible, formatted)
                                      + length + 1);
  p->value = value;
  memcpy (p->formatted, formatted, length + 1);

I'm surprised this isn't recognized as a useful coding pattern.
Comment 4 Joseph Myers 2016-05-25 13:46:16 UTC
Martin, any comments on the rationale for requiring size to be a multiple of alignment?

(_Decimal128 is a basic type - both TS 18661-2 and the older TR 24732 define it in a way that adds it to the basic types - so should just need malloc, not aligned_alloc.)
Comment 5 Martin Sebor 2016-05-25 14:58:29 UTC
Let me see if Clark remembers why he made the behavior undefined in N1397.  The intent of DR 460 was only to make the function more usable in portable programs by removing the undefined behavior.  I did look at APIs like posix_memalign and memalign but it didn't occur to me to relax the size constraint even further to match theirs.  I can't think of a reason why the size should be required to be a multiple of alignment (greater than or equal to it should suffice).

Making this change would also be in line with Clark's C++ overaligned memory allocation proposal (P0035R1) where the overaligned operator new accepts any such size, and explicitly calls out using aligned_alloc as a possible (though not required) implementation.
Comment 6 Martin Sebor 2016-05-26 19:38:46 UTC
I discussed the issue with Clark.  He hadn't considered the flexible array use case when working on N1397 and agrees that relaxing the size requirement imposed in DR 460 would be useful and sees no technical problem with doing so.  I'll submit an issue to WG14 and propose to make this change (i.e., require aligned_alloc to fail only when size < alignment).  Clark is including a suggestion in his upcoming D0035R3 to make a corresponding change to the C++ overaligned operator new (ironically, at the last WG21 meeting in Jacksonville he was asked to make the same condition undefined in his updated proposal, which is already reflected in D0035R2, the followup on the P0035R1 paper I referred to in comment #5).
Comment 7 jsm-csl@polyomino.org.uk 2016-05-26 20:13:20 UTC
We still need to make aligned_alloc fail for bad alignment values, though 
it's less clear there that a new symbol version is needed as it would be 
for restricting size values.
Comment 8 Florian Weimer 2016-05-27 08:21:05 UTC
(In reply to joseph@codesourcery.com from comment #7)
> We still need to make aligned_alloc fail for bad alignment values, though 
> it's less clear there that a new symbol version is needed as it would be 
> for restricting size values.

Agreed.

Martin and I also had an off-bug discussion about the size < alignment check.  I think it is incorrect because it assumes a particular implementation strategy for malloc, and having aligned small objects is sometimes useful for obtaining free bits for pointer tagging.

Furthermore, for malloc, the standard already requires that all allocations (even the smallest ones) have fundamental alignment.