Skip to content

cover: reject unrepresentable dmer map sizes in COVER_map_init - #4887

Open
itsgaldoron wants to merge 1 commit into
meta:devfrom
itsgaldoron:cover-map-init-unrepresentable-size
Open

itsgaldoron wants to merge 1 commit into
meta:devfrom
itsgaldoron:cover-map-init-unrepresentable-size

Conversation

@itsgaldoron

Copy link
Copy Markdown

Defect. COVER_map_init() (lib/dictBuilder/cover.c) rounds the dmer-map capacity up to the next power of two in a U32 and sizes the pair array with an unchecked multiply:

map->sizeLog = ZSTD_highbit32(size) + 2;
map->size = (U32)1 << map->sizeLog;
map->sizeMask = map->size - 1;
map->data = (COVER_map_pair_t *)malloc(map->size * sizeof(COVER_map_pair_t));

size is parameters.k - parameters.d + 1 from both call sites (:842, :1165). COVER_checkParameters() only bounds k by maxDictSize, so k = 2^30, d = 1 with maxDictSize >= 2^30 is accepted by the public ZDICT_trainFromBuffer_cover() and gives sizeLog = 32 — a shift of 32 in (U32)1 << sizeLog, which is undefined for a 32-bit type. On 32-bit targets the subsequent mapSize * sizeof(COVER_map_pair_t) wraps to zero once mapSize >= 2^27, so the map under-allocates and the first COVER_map_insert() writes out of bounds.

Measured on dev (49cf517) with ZDICT_trainFromBuffer_cover(dictBuffer, maxDictSize, 8 x 256 B samples, nbSamples=8, {k, d=1, steps=1, splitPoint=0.5}):

build k result
x86-64 plain 2^30 SIGSEGV (exit 139)
x86-64 -fsanitize=undefined 2^30 cover.c:158:22: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int'
i386 (-m32) plain 2^29 mapSize * 16 wraps to 0, under-allocated map, heap write OOB via COVER_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 guards ZSTD_highbit32(0), itself undefined);
  • sizeLog >= 32 (capacity not representable in the U32);
  • a pair-array byte size that wraps size_t (divide-back check — meaningful on 32-bit, where the OOB cell above lives).

map->sizeLog/size/sizeMask/data are committed only after the allocation succeeds. The same k-d+1 < 2^30 bound is surfaced in COVER_checkParameters(), so out-of-range k/d now fail as parameter_out_of_bound before any allocation work instead of reaching the map at all.

Regression tests (tests/fuzzer.c, dictBuilder section): ordinary k=64, d=8 must still train (control against over-rejection), and k = 2^30 must fail cleanly. The oversized-k cell runs crash-isolated via fork() (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):

unpatched: test175 : COVER accepts ordinary k/d : OK, created dictionary of size 16384
unpatched: test176 : COVER rejects unrepresentable k-d+1 : Error detected in Unit tests !   (exit 1)
patched:   test176 : COVER rejects unrepresentable k-d+1 : OK
patched:   1 fuzzer tests completed   (exit 0)

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 the ZDICT_trainFromBuffer* family); no decompression-side impact is claimed. k >= 2^30 is 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 large maxDictSize to 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_init size-MAX(d,8)+1 underflow), #4769 and #4821 (COVER_selectDict stale customDictContentEnd), #4724 (ZDICT_tryMerge stale slot index), #4616 (ZDICT_trainBuffer_legacy allocation sizes — different function, not touched here), #4649 (shared checked-allocation helpers in lib/common/allocations.h — complementary, not required by this patch).

🤖 Generated with Claude Code

@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!

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).
@itsgaldoron
itsgaldoron force-pushed the cover-map-init-unrepresentable-size branch from 4ad70fe to ae3732f Compare November 4, 2026 12:31
@meta-cla meta-cla Bot added the CLA Signed label Nov 4, 2026

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