Repository navigation
obmalloc's 8-byte alignment causes undefined behavior #72174
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2019-05-25.18:25:09.602>
created_at = <Date 2016-09-07.00:17:09.669>
labels = ['interpreter-core', '3.8', 'type-bug', '3.7']
title = "obmalloc's 8-byte alignment causes undefined behavior"
updated_at = <Date 2019-06-03.02:02:46.814>
user = 'https://github.com/benjaminp'bugs.python.org fields:
activity = <Date 2019-06-03.02:02:46.814>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2019-05-25.18:25:09.602>
closer = 'gregory.p.smith'
components = ['Interpreter Core']
creation = <Date 2016-09-07.00:17:09.669>
creator = 'benjamin.peterson'
dependencies = []
files = ['44413']
hgrepos = []
issue_num = 27987
keywords = ['patch']
message_count = 41.0
messages = ['274688', '304873', '304874', '304912', '304917', '304918', '304919', '304922', '304925', '304959', '305847', '305939', '305985', '311323', '340120', '340179', '340254', '340264', '340265', '340266', '340330', '340331', '342440', '342441', '342443', '342445', '342447', '342575', '343133', '343134', '343218', '343219', '343471', '343489', '343492', '343493', '343495', '343498', '343499', '343500', '344364']
nosy_count = 11.0
nosy_names = ['twouters', 'nascheme', 'gregory.p.smith', 'pitrou', 'vstinner', 'benjamin.peterson', 'methane', 'skrah', 'fweimer', 'miss-islington', 'tgrigg']
pr_nums = ['12850', '13318', '13319', '13335', '13336', '13569', '13581']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27987'
versions = ['Python 3.7', 'Python 3.8']
Activity
benjaminp commentedon Sep 7, 2016
ubsan complains about unaligned access when structs include "long double". An example error:
runtime error: member access within misaligned address 0x7f77dbba9798 for type 'struct CDataObject', which requires 16 byte alignment
This is because (on x86 anyway), long double is 16-bytes long and requires that alignment, but obmalloc only gives a 8-byte alignment. (glibc malloc() gives 16-byte alignment.)
I'm attaching a POC patch. I don't know what the impact of increasing the alignment is on obmalloc's performance or memory usage. It's also unfortunate that this patch increases the size of PyGC_Head to 32 bytes from 24 bytes. One can imagine a more middle-ground solution to this by allowing types to specify their required alignment.
pitrou commentedon Oct 24, 2017
What do we do if at some point a C type requires a larger alignment (for example a vector type readable using AVX512 instructions)?
vstinner commentedon Oct 24, 2017
Can we use memcpy() to prevent such issue?
benjaminp commentedon Oct 24, 2017
My suggestion would be to pass alignof(type) into the allocator via macro. Then the allocator could at least assert it's providing good enough alignment if not provide the correct alignment.
I believe 16-byte alignment is special because it's glibc's malloc's default. So "normal" code shouldn't really be expecting anything better than 16-byte alignment. Code with higher alignment requirements will have to use APIs like the one proposed in bpo-18835.
pitrou commentedon Oct 24, 2017
Do you mean using some new PyMem_ function? Or as as new tp_ field on the type declaration?
vstinner commentedon Oct 24, 2017
What matters when a Python object is allocated? The start of the PyObject structure, or the start of the PyGC_Head structure? Would it be possible to align the PyObject start?
The simplest option is to store data which needs to be aligned in a second memory block allocated by PyMem_AlignedAlloc().
vstinner commentedon Oct 24, 2017
Change by Antoine Pitrou: "versions: -Python 2.7, Python 3.3, Python 3.4, Python 3.5, Python 3.6"
The undefined behaviour exists and should be fixed in Python 2.7 and 3.6, no? Can we use memcpy()?
pitrou commentedon Oct 24, 2017
Hmm, perhaps. Do you want to try it out (and measure any performance degradation)?
skrah commentedon Oct 24, 2017
Since we have "#define PYMEM_FUNCS PYOBJ_FUNCS", I think extensions that
use PyMem_Malloc() also won't get the glibc max_align_t alignment.
But guess technically they should.
benjaminp commentedon Oct 25, 2017
Yes, we could memcpy things around to obtain the desired alignment. It would be nicer to have a builtin solution, though.
vstinner commentedon Nov 8, 2017
alignment.patch: + long double dummy; /* force worst-case alignment */
Would it be possible to use max_align_t mentioned by Stefan, at least when this type is available?
What is the impact of the patch on objects size?
benjaminp commentedon Nov 9, 2017
On Wed, Nov 8, 2017, at 06:14, STINNER Victor wrote:
Yes, that would be the correct thing to do. I was looking for the quick
hack.
On 64-bit platforms, I believe it wastes a word for GC objects.
25 remaining items
gpshead commentedon May 25, 2019
commit reverted in 2156fec
miss-islington commentedon May 25, 2019
New changeset 1b85f4e by Miss Islington (bot) in branch '3.7':
bpo-27987: pymalloc: align by 16bytes on 64bit platform (GH-12850)
1b85f4e
gpshead commentedon May 25, 2019
I'm not marking this bug as "Fixed" as the original complaint about obmalloc'd structs with a long double not being aligned is still going to be true on 32-bit platforms for 2.7 - 3.7. We've merely increased the obmalloc alignment to 16-bytes on 64-bit platforms.
So the problem should only remain for 32-bit users which at this point are likely only arm (rpi and similar low end friends not using a 64-bit OS).
pitrou commentedon May 25, 2019
Here is what I've found for (32-bit) ARM:
(see http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0274b/index.html)
(see http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042d/IHI0042D_aapcs.pdf)
And on (32-bit) x86, it looks like the standard alignment for "long double" is 4 bytes:
https://www.codesynthesis.com/~boris/blog/2009/04/06/cxx-data-alignment-portability/
So I don't think there's anything to change on 32-bit Python builds *if* we only really care about ARM and x86 (which is restrictive, but using "long double" in C extension types is a bit of an exotic issue).
pitrou commentedon May 25, 2019
And of course, someone who has this issue can at worse recompile Python without pymalloc.
gpshead commentedon May 25, 2019
if someone runs into an actual need for this on 32-bit builds, please provide details and feel free to reopen the issue. closing as i don't believe there is any more for us to do.
gpshead commentedon Jun 3, 2019
New changeset 8766cb7 by Gregory P. Smith (Inada Naoki) in branch '3.7':
[3.7] bpo-27987: align PyGC_Head to alignof(long double) (GH-13335) (GH-13581)
8766cb7