Re: [PATCH] locale: align _nl_C_LC_CTYPE_class and _nl_C_LC_CTYPE_class32 arrays to uint16_t and uint32_t respectively

[Lirong Yuan <yuanzi@google.com>]

On Mon, Mar 15, 2021 at 6:45 PM Carlos O'Donell <carlos@redhat.com> wrote:

> My expectation is that normally aarch64 simply handles the unaligned load
> without any problems,
> but that it would be "better" if it were 16-bit aligned?
> Is this the *only* case of misaligned pointers?


Yes, this is the only case reported by UBSan.

> Signed-off-by: Lirong Yuan <yuanzi@google.com>
> We don't use DSOs in glibc, we assign copyright to the FSF, so this line
> would
> be normally removed, and you as the git author remains.


Thanks for the explanation! I will send an updated patch without
"Signed-off-by" if the current approach looks good. :)

On Tue, Mar 16, 2021 at 7:28 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:

> The 03/15/2021 21:44, Carlos O'Donell wrote:
> > On 3/15/21 2:42 PM, Lirong Yuan via Libc-alpha wrote:
> > > steps to reproduce the problem: compile a program that uses ctype
> functions such as “isspace” for aarch64 with UBSan flag
> “-fsanitize=undefined” and run it on x86_64 machines with qemu user mode
> emulation.
> >
> > Szabolcs,
> >
> > Do you have any input on this?
> >
> > > observed behavior: UndefinedBehaviorSanitizer reports
> misaligned-pointer-use in the program.
> >
> > Yes, the char array could be misaligned with respect to a 16-bit value,
> > and should be aligned to the type that is expected from the interface
> e.g.
>
> using char[] as uint16_t[] is aliasing violation. and in principle
> alignas on the definition does not fix this, but in practice that's
> the only abi visible aspect of the wrong type.
>

Alternatively, we can define _nl_C_LC_CTYPE_class and
_nl_C_LC_CTYPE_class32 arrays directly as uint16_t and uint32_t arrays,
like _nl_C_LC_CTYPE_toupper array:
https://code.woboq.org/userspace/glibc/locale/C-ctype.c.html#_nl_C_LC_CTYPE_toupper
Though the conversion may be error-prune and require more test cases...
It would seem that using alignas is an approach that's both technically
correct and less likely to cause havoc.


> i'm not sure why ubsanitizer cares about alignment specifically on
> aarch64, unaligned load should work.
>

Yes, the code works fine in practice on aarch64. The ubsan alignment is a
check for misaligned rather than unaligned. It's almost always worth fixing
since this can cause subtle and hard to track down failures that more often
manifest on other architectures.

Thanks,
Lirong