solving flexArray issue #3785 in fse

[Cyan4973]

the flexArray in structure FSE_DecompressWksp is a bit different from the ones in ZSTDMT_.

Its primary purpose is to divide a flat buffer workspace into different regions,
and provide a way to derive a pointer to dtable easily,
with less risk/complexity than calculating it manually.
Here there is no way to update dtable to use a separate malloc/free, since the entire point of workspace is to avoid any allocation (it helps tremendously to deploy the code in constrained environments such as an OS kernel).

In this PR, we just do no longer use struct->dtable directly, and instead derive a pointer dtable* from it,
which is then used in following invocations.

Not sure if this change is good enough to circumvent ubsan warnings, since inlining optimizations with constant propagation might believe that the pointed area only has a size of 1.

Btw, it shows that having a way to reproduce the issue would be great, especially in CI.

[ebiggers]

This won't be enough, unfortunately. Assigning the array to a pointer doesn't change the fact that it is declared to have length 1.

BTW, the UBSAN error in zstdmt_compress.c reproduces with the following command: CFLAGS="-fsanitize=undefined -fno-sanitize-recover=undefined -fstrict-flex-arrays=3" make -j8 check. It works with both CC=gcc and CC=clang (using gcc 13.2.1 and clang 16.0.6). However, the UBSAN error in fse_decompress.c doesn't reproduce that way. It does reproduce when booting the Linux kernel with the crypto self-tests enabled (causing the copy of Zstandard in the kernel to be tested), if the kernel was built with clang and the UBSAN bounds checker enabled.

[terrelln]

It does reproduce when booting the Linux kernel with the crypto self-tests enabled (causing the copy of Zstandard in the kernel to be tested), if the kernel was built with clang and the UBSAN bounds checker enabled.

I haven't been able to reproduce it in the Linux Kernel myself. I compiled with clang 16.0.6, UBSAN, and crypto self checks enabled. Do you have a config fragment for the relevant configs I should set?

[terrelln]

I think a reasonable path forward would be just removing the variable size array and adding logic like:

typedef struct {
    short ncount[FSE_MAX_SYMBOL_VALUE + 1];
- FSE_DTable dtable[1]; /* Dynamically sized */
} FSE_DecompressWksp;

...
if (FSE_DECOMPRESS_WKSP_SIZE(tableLog, maxSymbolValue) > wkspSize) return ERROR(tableLog_tooLarge);
assert(sizeof(*wksp) + FSE_DTABLE_SIZE(tableLog) <= wkspSize);

+ ZSTD_STATIC_ASSERT(sizeof(FSE_DecompressWksp) % sizeof(FSE_DTable) == 0);
+ FSE_DTable* dtable = (BYTE*)wksp + sizeof(*wksp);

workSpace = (BYTE*)workSpace + sizeof(*wksp) + FSE_DTABLE_SIZE(tableLog);
wkspSize -= sizeof(*wksp) + FSE_DTABLE_SIZE(tableLog);

ZSTD_STATIC_ASSERT(sizeof(FSE_DecompressWksp) % sizeof(FSE_DTable) == 0);
FSE_DTable* dtable = (BYTE*)wksp + sizeof(*wksp);

[Cyan4973]

I think a reasonable path forward would be just removing the variable size array and adding logic like:

Yes,
basically back to determining memory regions manually.
This is the natural fallback.

[Cyan4973]

Changed to manually determine dtable start position,
so that array size is no longer confused as 1.