Repository navigation
zdict: check allocation sizes in ZDICT_trainBuffer_legacy() - #4889
itsgaldoron wants to merge 1 commit into
Conversation
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).
|
Hi @itsgaldoron! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Defect.
ZDICT_trainBuffer_legacy()(lib/dictBuilder/zdict.c) sizes its working buffers straight frombufferSizeandnbFileswith unchecked arithmetic:On 32-bit targets
(bufferSize+2)*sizeof(*suffix0)andbufferSize*sizeof(*reverseSuffix)wrap forbufferSize >= 0x3FFFFFFE(~1 GB of samples, accepted through the publicZDICT_trainFromBuffer_legacy()entry), so the suffix arrays under-allocate (to 8 and 0 bytes) anddivsufsort()writes out of bounds through the too-smallsuffixblock.nbFiles * sizeof(*filePos)wraps the same way oncenbFiles > SIZE_MAX / sizeof(*filePos).Measured at
dev(49cf517) on a 32-bit build withbufferSize = 0x40000000(1 sample × 1 GB; wrap arithmetic printed by the driver itself):-fsanitize=addressZDICT_trainBuffer_legacy()directsort_typeBstar()(divsufsort.c:1469), 4 bytes left of the 8-byte region allocated atzdict.c:474— the wrapped(bufferSize+2)*4allocationZDICT_trainFromBuffer_legacy()(public)(The first write through the wrapped block is divsufsort's suffix-array fill; with a constant input it is instead the
reverseSuffix[suffix[pos]] = posfill atzdict.c:518— same under-allocation, measured as an ASan WRITE of size 4 into the 0-bytereverseSuffixregion.)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), thebufferSize+2/bufferSize+16additions are pre-checked againstSIZE_MAX, and the four pointers are allocated only after the checks pass. Unrepresentable sizes returnERROR(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 viafork()(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):
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-invalidDictionariespasses 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 hugemallocfails gracefully — this patch does not change 64-bit behaviour. The publicZDICT_trainFromBuffer()no longer reaches this code (it routes through fastCover); the reachable surface is theZDICT_trainFromBuffer_legacy()family, wheresamplesBuffer/samplesSizesare caller-supplied. Note the callerZDICT_trainFromBuffer_unsafe_legacy()discardsZDICT_trainBuffer_legacy()'s return code (pre-existing behaviour): after the fix the API reportsdictionaryCreation_failedinstead 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_tryMergestale slot index), #4869 (ZDICT_addEntropyTablesFromBuffer_advancedbuffer capacities — different function), #4649 (shared checked-allocation helpers inlib/common/allocations.h— complementary, not required by this patch), and my own #4887 (COVER_map_initdmer-map sizes — different function, same "unrepresentable size → reject" shape).🤖 Generated with Claude Code