Re: [PATCH v2 00/10] Undefined Behavior Sanitizer, this time with docs

[Joel Brobecker <brobecker@adacore.com>]

> This also revealed PR 23743:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=23743

For the record, I've also discovered something I didn't realize
about struct memory allocations, which is a bit disturbing, because
I think we have a lot of areas where we malloc struct objects...
I reported it under https://sourceware.org/bugzilla/show_bug.cgi?id=23768

For now, it seems like the lessons learned is that it seems we should
be very careful when using XNEW/XCNEW with struct types?

----------------------------------------------------------------------

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...

-- 
Joel