Skip to content

zdict: check allocation sizes in ZDICT_trainBuffer_legacy() - #4889

Open
itsgaldoron wants to merge 1 commit into
meta:devfrom
itsgaldoron:zdict-trainbuffer-alloc-overflow
Open

itsgaldoron wants to merge 1 commit into
meta:devfrom
itsgaldoron:zdict-trainbuffer-alloc-overflow

Conversation

@itsgaldoron

Copy link
Copy Markdown

Defect. ZDICT_trainBuffer_legacy() (lib/dictBuilder/zdict.c) sizes its working buffers straight from bufferSize and nbFiles with unchecked arithmetic:

unsigned* const suffix0 = (unsigned*)malloc((bufferSize+2)*sizeof(*suffix0));
unsigned* const suffix = suffix0+1;
U32* reverseSuffix = (U32*)malloc((bufferSize)*sizeof(*reverseSuffix));
BYTE* doneMarks = (BYTE*)malloc((bufferSize+16)*sizeof(*doneMarks));   /* +16 for overflow security */
U32* filePos = (U32*)malloc(nbFiles * sizeof(*filePos));

On 32-bit targets (bufferSize+2)*sizeof(*suffix0) and bufferSize*sizeof(*reverseSuffix) wrap for bufferSize >= 0x3FFFFFFE (~1 GB of samples, accepted through the public ZDICT_trainFromBuffer_legacy() entry), so the suffix arrays under-allocate (to 8 and 0 bytes) and divsufsort() writes out of bounds through the too-small suffix block. nbFiles * sizeof(*filePos) wraps the same way once nbFiles > SIZE_MAX / sizeof(*filePos).

Measured at dev (49cf517) on a 32-bit build with bufferSize = 0x40000000 (1 sample × 1 GB; wrap arithmetic printed by the driver itself):

sizeof(size_t)=4
bufferSize=0x40000000 (1073741824)
suffix0       : (bufferSize+2)*4 = 0x100000008 -> uint32 0x00000008
reverseSuffix : bufferSize*4     = 0x100000000 -> uint32 0x00000000
doneMarks     : bufferSize+16    = 0x40000010 (no wrap)
filePos       : nbFiles*4        = 0x4
calling ZDICT_trainBuffer_legacy(bufferSize=0x40000000, nbFiles=1)
build entry result
i386 -fsanitize=address ZDICT_trainBuffer_legacy() direct heap-buffer-overflow WRITE of size 4 in sort_typeBstar() (divsufsort.c:1469), 4 bytes left of the 8-byte region allocated at zdict.c:474 — the wrapped (bufferSize+2)*4 allocation
i386 plain ZDICT_trainFromBuffer_legacy() (public) SIGSEGV (exit 139)

(The first write through the wrapped block is divsufsort's suffix-array fill; with a constant input it is instead the reverseSuffix[suffix[pos]] = pos fill at zdict.c:518 — same under-allocation, measured as an ASan WRITE of size 4 into the 0-byte reverseSuffix region.)

Fix. Reject sizes whose byte counts cannot be represented instead of allocating from an overflowed size. Each product is computed and divided back (a / sizeof(*p) != count ⇒ the multiply wrapped), the bufferSize+2 / bufferSize+16 additions are pre-checked against SIZE_MAX, and the four pointers are allocated only after the checks pass. Unrepresentable sizes return ERROR(memory_allocation) before any allocation work.

Regression tests (tests/fuzzer.c, dictBuilder section): ordinary samples must still train through the legacy entry (control against over-rejection), and the wrapping size must fail cleanly. The wrap cell is crash-isolated via fork() (in-process skip on Windows) and gated to 32-bit targets — that is where the wrap is reachable.

Dual-tree evidence (same test transplanted into both trees, 32-bit builds):

unpatched: test223 : ZDICT_trainFromBuffer_legacy : OK, created dictionary of size 16384
unpatched: test224 : ZDICT_trainFromBuffer_legacy rejects wrapping allocation sizes : Error detected in Unit tests !   (exit 1)
patched:   test224 : ZDICT_trainFromBuffer_legacy rejects wrapping allocation sizes : OK
patched:   1 fuzzer tests completed   (exit 0)

On 64-bit builds the cell reports skipped (allocation sizes do not wrap on 64-bit) and the suites are identical modulo the new cells (pre-existing cells unchanged, zero build warnings). make -C tests test-zstd test-legacy test-invalidDictionaries passes on both trees (exit 0), outputs identical modulo timings. Witness output on the patched 32-bit build: returned: 4294967232 (Allocation error : not enough memory) — clean error, no crash.

What this does not claim. This is dictionary-builder input validation (lib/dictBuilder, the training path); no decompression-side impact is claimed. On 64-bit targets the products are representable and a huge malloc fails gracefully — this patch does not change 64-bit behaviour. The public ZDICT_trainFromBuffer() no longer reaches this code (it routes through fastCover); the reachable surface is the ZDICT_trainFromBuffer_legacy() family, where samplesBuffer/samplesSizes are caller-supplied. Note the caller ZDICT_trainFromBuffer_unsafe_legacy() discards ZDICT_trainBuffer_legacy()'s return code (pre-existing behaviour): after the fix the API reports dictionaryCreation_failed instead of crashing; error propagation is deliberately not changed here.

Supersede / neighbour disclosure. This supersedes #4616 ("Add allocation size overflow checks in ZDICT_trainBuffer_legacy()"), which guards the same four products with the same rejection semantics but ships no test and has been inactive since it was opened (2026-03-09); I will happily drop this PR if the original author plans to continue. Same-adjacent-area PRs with different root causes, disclosed for the record: #4724 (ZDICT_tryMerge stale slot index), #4869 (ZDICT_addEntropyTablesFromBuffer_advanced buffer capacities — different function), #4649 (shared checked-allocation helpers in lib/common/allocations.h — complementary, not required by this patch), and my own #4887 (COVER_map_init dmer-map sizes — different function, same "unrepresentable size → reject" shape).

🤖 Generated with Claude Code

ZDICT_trainBuffer_legacy() sized its working buffers straight from
bufferSize and nbFiles with unchecked arithmetic:

    malloc((bufferSize+2)*sizeof(*suffix0))
    malloc(bufferSize*sizeof(*reverseSuffix))
    malloc((bufferSize+16)*sizeof(*doneMarks))
    malloc(nbFiles*sizeof(*filePos))

On 32-bit targets (bufferSize+2)*4 and bufferSize*4 wrap for
bufferSize >= 0x3FFFFFFE (~1 GB of samples, accepted through the public
ZDICT_trainFromBuffer_legacy() entry), so the suffix arrays
under-allocate (to 8 and 0 bytes) and divsufsort() writes out of bounds
through them: measured as an ASan heap-buffer-overflow WRITE inside
sort_typeBstar() and a SIGSEGV through the public API on 32-bit builds.

Reject sizes whose byte counts cannot be represented instead of
allocating from an overflowed size.

Add regression tests to tests/fuzzer.c: ordinary samples must still
train through the legacy entry, and the wrapping size must fail cleanly
(run crash-isolated via fork(), and gated to 32-bit targets, where the
wrap is reachable; on 64-bit the products are representable and a huge
allocation fails gracefully).
@meta-cla

meta-cla Bot commented Nov 4, 2026

Copy link
Copy Markdown

Hi @itsgaldoron!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@itsgaldoron itsgaldoron closed this Nov 4, 2026
@itsgaldoron itsgaldoron reopened this Nov 4, 2026
@meta-cla

meta-cla Bot commented Nov 4, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant