Undefined behavior in ZydisGetOperandSizeFromElementSize (memory alignment)

[wareya]

I'm using Zydis as an instruction encoder. I'm using the amalgamated version. When I compile my application with ubsan (i.e. clang and -fsanitize=undefined) I get a memory-alignment-related UB error:

src/x86/../../thirdparty/zydis/Zydis.c:15653:24: runtime error: load of misaligned address 0x55d8e70361db for type 'const ZyanU16 *' (aka 'const unsigned short *'), which requires 2 byte alignment
0x55d8e70361db: note: pointer points here

(Line number differs because of unrelated changes I had to make to get it to compile as both C and C++)

The responsible call is the one that looks like:

                match->eosz = ZydisGetOperandSizeFromElementSize(match, def_op->size,
                    user_op->mem.size, ZYAN_TRUE);

I don't know why this is happening; I think it might have something to do with ZydisOperandDefinition having bitfields.

I don't know how to keep the array aligned to a multiple of 2, but doing this works:

static ZyanU8 ZydisGetOperandSizeFromElementSize(ZydisEncoderInstructionMatch *match,
    const ZyanU16 *_size_table, ZyanU16 desired_size, ZyanBool exact_match_mode)
{
    ZyanU16 size_table[3];
    ZYAN_MEMCPY(size_table, (void*)_size_table, sizeof(ZyanU16) * 3);
    // ...

For some reason the explicit (void*) cast is necessary to keep ubsan from complaining.

(This is all while compiling as C, like intended)

[athre0z]

Hmm. Did you break or remove the ZYAN_BITFIELD macro by any chance? The size field in question seems to be correctly aligned:

$ pahole ./ZydisInfo
...
struct ZydisOperandDefinition_ {
        ZyanU8                     type:6;               /*     0: 0  1 */
        ZyanU8                     visibility:2;         /*     0: 6  1 */
        ZyanU8                     actions:4;            /*     1: 0  1 */

        /* XXX 4 bits hole, try to pack */

        ZyanU16                    size[3];              /*     2     6 */
        ZyanU8                     element_type:5;       /*     8: 0  1 */

        /* XXX 3 bits hole, try to pack */

        ...
};
...

I also cannot reproduce this with clang + ubsan locally.

[wareya]

I can reproduce it even with the original unmodified v4.1.0 amalgamation. I'm on WSL2. Nope, didn't change the definitions of any macros.

wareya@Toriaezu:/mnt/c/users/wareya/dev/bbae$ md5sum thirdparty/zydis/Zydis.c
fdeaa628eb311f4b09179b01e0a8ebae  thirdparty/zydis/Zydis.c

wareya@Toriaezu:/mnt/c/users/wareya/dev/bbae$ md5sum thirdparty/zydis/Zydis.h
a2af56beb4d6a252e407f5a24fc2918c  thirdparty/zydis/Zydis.h

wareya@Toriaezu:/mnt/c/users/wareya/dev/bbae$ cat thirdparty/zydis/Zydis.c | sed 's/include <Zydis.h>/include "Zydis.h"/' | sponge thirdparty/zydis/Zydis.c # don't require zydis dir to be a syslib dir

wareya@Toriaezu:/mnt/c/users/wareya/dev/bbae$ clang --version
Ubuntu clang version 18.1.3 (1ubuntu1)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

wareya@Toriaezu:/mnt/c/users/wareya/dev/bbae$ cat run_tests.sh | grep undefined
  *)        asan="-fsanitize=undefined -fno-sanitize=function" ;;

wareya@Toriaezu:/mnt/c/users/wareya/dev/bbae$ cat run_tests.sh | grep clang
clang --std=gnu99 src/tests.c -Wall -Wextra -pedantic -O3 -g -ggdb $asan -Wno-unused-function -o $f || exit 1
# note: happens with gnu11 and gnu23 as well

wareya@Toriaezu:/mnt/c/users/wareya/dev/bbae$ ./run_tests.sh
tests/retsanitytest.bbae: 0.00000000000000000000 -- pass!
tests/retsanitytest_int.bbae: 0 -- pass!
tests/optsanitytest.bbae: 0.00000000000000000000 -- pass!
examples/gravity.bbae: 4899999.99992822110652923584 -- pass!
tests/gravtest.bbae: 4899999.99992822110652923584 -- pass!
tests/loopsanity.bbae: 0.00000000000000000000 -- pass!
examples/too_simple.bbae: 3.14159265258805042720 -- pass!
examples/fib.bbae: 433494437 -- pass!
src/x86/../../thirdparty/zydis/Zydis.c:15690:39: runtime error: load of misaligned address 0x55a6ee9238dd for type 'const ZyanU16' (aka 'const unsigned short'), which requires 2 byte alignment
0x55a6ee9238dd: note: pointer points here
 57 02 01 00 01 00 01  00 06 02 00 00 00 44 01  00 00 00 00 00 00 00 01  00 00 00 48 02 00 00 00  00
             ^
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/x86/../../thirdparty/zydis/Zydis.c:15690:39
examples/global.bbae: (finished running) -- pass!
Tests finished!

Clang seems to decide that the alignment of the struct is 1 (so the big array of them could have every other member misaligned if it's an odd size):

fprintf(stderr, "alignof %d\n", _Alignof(ZydisOperandDefinition));

Output:

alignof 1

Which is why I think it might be related to the struct having bitfields in it.

[flobernd]

It might be right. We are using:

#pragma pack(push, 1)

for these structs.

[athre0z]

Hmm, yeah. This makes sense: the struct is 13 bytes long, and in the array it won't be padded due to the #pragma pack. I also tried with clang16 on a Debian x86_64 machine and still can't reproduce the ubsan crash locally with any of our test-cases, and we'd totally expect these to also read from the operand array. It also seems quite unlikely that all of our tests only read from even indices.

Either way, I think this should be considered a bug. We can probably fix this by moving the last two boolean fields to the front, shrinking the struct to 12 bytes. It's kind of brittle, though. Perhaps we should remove the packing in general. I remember that the effect on binary size is quite substantial. We should probably manually reorder fields to achieve equivalent packing without the ugly pragma. We use field-order struct init in the .inc files, so this implies also changing the generator.

@flobernd got permission from his employer to work on Zydis this week and is making an attempt to rewrite our old generator in C# (previously Delphi). Let's wait whether that works out in time, and if so we should be able to make such a change more easily. :)

[flobernd]

As a workaround we could as well "unpack" the array and add width16 width32 and width64 as separate fields.

As long as there is no pointer access, the unaligned fields shouldn't be an issue.