Repository navigation
cover: reject unrepresentable dmer map sizes in COVER_map_init - #4887
itsgaldoron wants to merge 1 commit into
Conversation
|
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! |
COVER_map_init() rounded the map capacity up to the next power of two in a U32 and sized the pair array with an unchecked multiply. For size = k-d+1 >= 2^30 (accepted by COVER_checkParameters(), which only bounds k by maxDictSize) the shift in `map->size = (U32)1 << map->sizeLog` is by 32 or more, which is undefined for a 32-bit type; measured SIGSEGV on x86-64 and a UBSan shift-exponent abort. On 32-bit targets mapSize * sizeof(COVER_map_pair_t) wraps to zero for mapSize >= 2^27, so the map under-allocates and the first COVER_map_insert() writes out of bounds (measured heap corruption, SIGABRT in free()). Reject sizes that cannot be represented (size == 0, sizeLog >= 32, or a pair-array byte size that wraps size_t) instead of shifting/multiplying into undefined behaviour, and surface the same bound in COVER_checkParameters() so out-of-range k/d fail as a parameter error. Add regression tests to tests/fuzzer.c: ordinary k/d must still train, and k = 2^30 must fail cleanly (run crash-isolated via fork() so an unfixed tree reports the failure instead of killing the test runner).
4ad70fe to
ae3732f
Compare
Defect.
COVER_map_init()(lib/dictBuilder/cover.c) rounds the dmer-map capacity up to the next power of two in aU32and sizes the pair array with an unchecked multiply:sizeisparameters.k - parameters.d + 1from both call sites (:842,:1165).COVER_checkParameters()only boundskbymaxDictSize, sok = 2^30, d = 1withmaxDictSize >= 2^30is accepted by the publicZDICT_trainFromBuffer_cover()and givessizeLog = 32— a shift of 32 in(U32)1 << sizeLog, which is undefined for a 32-bit type. On 32-bit targets the subsequentmapSize * sizeof(COVER_map_pair_t)wraps to zero oncemapSize >= 2^27, so the map under-allocates and the firstCOVER_map_insert()writes out of bounds.Measured on
dev(49cf517) withZDICT_trainFromBuffer_cover(dictBuffer, maxDictSize, 8 x 256 B samples, nbSamples=8, {k, d=1, steps=1, splitPoint=0.5}):2^30-fsanitize=undefined2^30cover.c:158:22: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int'-m32) plain2^29mapSize * 16wraps to 0, under-allocated map, heap write OOB viaCOVER_map_insert()→munmap_chunk(): invalid pointer, SIGABRT (exit 134)Fix. Reject map sizes that cannot be represented instead of computing with undefined behaviour:
size == 0(also guardsZSTD_highbit32(0), itself undefined);sizeLog >= 32(capacity not representable in theU32);size_t(divide-back check — meaningful on 32-bit, where the OOB cell above lives).map->sizeLog/size/sizeMask/dataare committed only after the allocation succeeds. The samek-d+1 < 2^30bound is surfaced inCOVER_checkParameters(), so out-of-rangek/dnow fail asparameter_out_of_boundbefore any allocation work instead of reaching the map at all.Regression tests (
tests/fuzzer.c, dictBuilder section): ordinaryk=64, d=8must still train (control against over-rejection), andk = 2^30must fail cleanly. The oversized-k cell runs crash-isolated viafork()(in-process on Windows) so an unfixed tree reports the failure instead of killing the test runner.Dual-tree evidence (this branch vs
dev, same test in both trees):Pre-existing cells are identical on both trees (
test169–test175,test177+); the only delta is the new cell. Zero build warnings on the patched tree.What this does not claim. This is dictionary-builder input validation (
lib/dictBuilder, the training path and theZDICT_trainFromBuffer*family); no decompression-side impact is claimed.k >= 2^30is outside any documented usage range — the defect is that such parameters are accepted and turn into undefined behaviour / a heap overflow instead of an error. The reachable surface is a caller passing attacker-influenced COVER parameters (k,d) together with a largemaxDictSizeto the public training APIs.Supersede / neighbour disclosure. This supersedes #4627 ("Add checked shift and allocation size validation in COVER_map_init()"), which fixes the same root cause and has been inactive since 2026-03-18; I have posted a takeover comment there and will happily drop this PR if the original author plans to continue. Same-file / same-adjacent-area PRs with different root causes, disclosed for the record: #4690 and #4693 (
COVER_ctx_initsize-MAX(d,8)+1underflow), #4769 and #4821 (COVER_selectDictstalecustomDictContentEnd), #4724 (ZDICT_tryMergestale slot index), #4616 (ZDICT_trainBuffer_legacyallocation sizes — different function, not touched here), #4649 (shared checked-allocation helpers inlib/common/allocations.h— complementary, not required by this patch).🤖 Generated with Claude Code