[Bug gdb/23768] New: ubsan runtime error due to misaligned struct member access after XCNEW

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23768

Bug ID: 23768
Summary: ubsan runtime error due to misaligned struct member
access after XCNEW
Product: gdb
Version: HEAD
Status: NEW
Severity: normal
Priority: P2
Component: gdb
Assignee: unassigned at sourceware dot org
Reporter: brobecker at gnat dot com
Target Milestone: ---

on ppc-linux, a GDB built with --enable-ubsan=yes crashes immediately as soon
as one tries to enter anything at the prompt:

$ /path/to/gdb
GNU gdb (GDB) 8.2.50.20181012-git (with AdaCore local changes)
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
See your support agreement for details of warranty and support.
If you do not have a current support agreement, then there is absolutely
no warranty for this version of GDB.
Type "show copying" and "show warranty" for details.
This GDB was configured as "powerpc-generic-linux-gnu".
Type "show configuration" for configuration details.
For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) /homes/brobecke/act/gdb/gdb-head/gdb/common/common-exceptions.c:84:26:
runtime error: member access within misaligned address 0x124af518 for type
'struct catcher', which requires 16 byte alignment

The code that triggers the error is fairly straightforward:

| jmp_buf *
| exceptions_state_mc_init (void)
| {
| struct catcher *new_catcher = XCNEW (struct catcher);
|
| /* Start with no exception. */
| new_catcher->exception = exception_none;

The problem happens because "struct catcher" is a structure which
requires a 16 bytes alignment, due to one of its members (the jmpbuf)
having itself a 16bytes alignment:

From bits/setjmp.h on ppc-linux:

| /* The current powerpc 32-bit Altivec ABI specifies for SVR4 ABI and EABI
| the vrsave must be at byte 248 & v20 at byte 256. So we must pad this
| correctly on 32 bit. It also insists that vecregs are only gauranteed
| 4 byte alignment so we need to use vperm in the setjmp/longjmp routines.
| We have to version the code because members like int __mask_was_saved
| in the jmp_buf will move as jmp_buf is now larger than 248 bytes. We
| cannot keep the altivec jmp_buf backward compatible with the jmp_buf. */
| #ifndef _ASM
| # if __WORDSIZE == 64
| typedef long int __jmp_buf[64] __attribute__ ((__aligned__ (16)));
| # else
| /* The alignment is not essential, i.e.the buffer can be copied to a 4 byte
| aligned buffer as per the ABI it is just added for performance reasons.
*/
| typedef long int __jmp_buf[64 + (12 * 4)] __attribute__ ((__aligned__ (16)));
| # endif

XCNEW performs calls to malloc without worrying about alignment,
so there is indeed no guaranty that the returned address is
aligned on 16 bytes. We looked at the GNU libc documentation,
for instance, and they guaranty 8 bytes only on 32bit machines

| https://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html
|
| 3.2.3.6 Allocating Aligned Memory Blocks
|
| The address of a block returned by malloc or realloc in GNU systems is
| always a multiple of eight (or sixteen on 64-bit systems). If you need a
| block whose address is a multiple of a higher power of two than that,
| use aligned_alloc or posix_memalign. aligned_alloc and posix_memalign
| are declared in stdlib.h.

We can certainly start by plugging the hole in this particular case,
and swap the call to XCNEW with a call to an aligned malloc. But
this made me realize we have a general issue, because potentially,
all struct allocations are potentially problematic, if they happen
to have alignment requirements that are stronger than what malloc
itself follows. I hope that, in practice, aligment requirements
beyond 4 or 8 bytes are rare.

Perhaps one possible idea was to have XNEW/XCNEW et all take the
alignment into account when performing the memory allocation
(we pass the type, so it could use alignof); however, that is
a major change affecting everyone. And looking closer at those
macros, one notices the following important comment...

/* Scalar allocators. */

... which tells me the macros may not have been meant to be used
in the context of allocating structures.

It's unclear to me what we would want to do...

[simon.marchi at ericsson dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23768

Simon Marchi <simon.marchi at ericsson dot com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |simon.marchi at ericsson dot com

--- Comment #1 from Simon Marchi <simon.marchi at ericsson dot com> ---
I think what it means by "scalar allocators" is the fact they allocate a single
object, if you compare that to the following "Array allocators.". The comment
above that makes me think that they should take the alignment into
consideration in order to be correct:

/* These macros provide a K&R/C89/C++-friendly way of allocating structures
with nice encapsulation. The XDELETE*() macros are technically
superfluous, but provided here for symmetry. Using them consistently
makes it easier to update client code to use different allocators such
as new/delete and new[]/delete[]. */

So I think modifying XNEW and friends would make sense. If we really don't
want to change the code generated for current call sites, we could do something
like:

#define XNEW ((alignof (T) > 8) ? (some_aligned_malloc (sizeof (T), alignof
(T))) : (malloc (sizeof(T))))

where some_aligned_malloc uses posix_memalign or similar. The condition is
evaluated at compile time, so the generated code shouldn't change. But if
there's no big difference, we could always call some_aligned_malloc, for all
types.

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23768

--- Comment #2 from Joel Brobecker <brobecker at gnat dot com> ---
Hi Simon; I am certainly open to the idea of enhancing XNEW et al, so we could
contact the libiberty folks about this to see if they remember what the intent
was when they created these macros. The concern I have with this approach is
that it pessimizes the case of plain scalars, and for tools that are
performance-sensitive (such as compilation toolchains), this may not be
acceptable, even with the approach you suggest of checking the alignment first.

[simon.marchi at ericsson dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23768

--- Comment #3 from Simon Marchi <simon.marchi at ericsson dot com> ---

The concern I have with this approach is that it pessimizes the case of plain scalars, and for tools that are performance-sensitive (such as compilation toolchains), this may not be acceptable, even with the approach you suggest of checking the alignment first.

What do you mean here with pessimization? In this case:

#define XNEW ((alignof (T) > 8) ? (some_aligned_malloc (sizeof (T), alignof
(T))) : (malloc (sizeof(T))))

scalars will be allocated exactly the same way as before, and the generated
machine code should be the same as before.

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23768

--- Comment #4 from Joel Brobecker <brobecker at gnat dot com> ---
I was referring to the extra alignment check which is used to decide which
allocation policy to use. But perhaps the compiler is able to statically
evaluate that check, and thus eliminate the overhead in practice? (didn't think
of that)

[simon.marchi at ericsson dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23768

--- Comment #5 from Simon Marchi <simon.marchi at ericsson dot com> ---
I think this is a piece of cake for the compiler to optimize. My test program

---
#include <malloc.h>

#define XNEW(T) (T *)((_Alignof (T) > 8) ? memalign (_Alignof (T), sizeof (T))
: malloc (sizeof (T)))

struct a_struct { int n; };
struct an_aligned_struct { int __attribute__((aligned(32))) n; };

struct a_struct*
allocate_a_struct ()
{ return XNEW(struct a_struct); }

struct an_aligned_struct*
allocate_an_aligned_struct ()
{ return XNEW(struct an_aligned_struct); }

int main()
{
allocate_a_struct();
allocate_an_aligned_struct();
}
---

$ gcc -std=gnu11 test.c -g3 -O0
$ objdump -d a.out

0000000000400576 <allocate_a_struct>:
400576: 55 push %rbp
400577: 48 89 e5 mov %rsp,%rbp
40057a: bf 04 00 00 00 mov $0x4,%edi
40057f: e8 dc fe ff ff callq 400460 <***@plt>
400584: 5d pop %rbp
400585: c3 retq

0000000000400586 <allocate_an_aligned_struct>:
400586: 55 push %rbp
400587: 48 89 e5 mov %rsp,%rbp
40058a: be 20 00 00 00 mov $0x20,%esi
40058f: bf 20 00 00 00 mov $0x20,%edi
400594: e8 b7 fe ff ff callq 400450 <***@plt>
400599: 5d pop %rbp
40059a: c3 retq

And this is with O0!