From da806fde701068ba467826e1eff25a3aa0c9b228 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 10 Nov 2026 11:12:03 -0800 Subject: [PATCH] Fix block splitter partition bounds Valid external sequence streams can exhaust the recursive block splitter's budget even within the 128 KiB block-size limit. The budget was checked only on function entry, so a left-child recursion could fill the partition array before its ancestors wrote their own split positions. The terminating block boundary also had no dedicated slot. These writes could silently overwrite adjacent compression-context state. Recheck the split budget after the left recursion, reserve one extra partition entry for the terminating boundary, and assert the final split count. Update the comment that incorrectly described the limit as unreachable. Add a deterministic regression using 38,400 valid external sequences with sequence validation enabled. It verifies the 196-split limit, the in-bounds terminator, and a successful round trip. The unmodified baseline emitted 200 blocks; the fixed implementation emits 197. --- lib/compress/zstd_compress.c | 9 ++- lib/compress/zstd_compress_internal.h | 2 +- tests/fuzzer.c | 79 +++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index c51fd1853ff..712c5eca346 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -4232,8 +4232,8 @@ typedef struct { * In practice, recursion depth usually doesn't go beyond 4. * * Furthermore, the number of splits is capped by ZSTD_MAX_NB_BLOCK_SPLITS. - * At ZSTD_MAX_NB_BLOCK_SPLITS == 196 with the current existing blockSize - * maximum of 128 KB, this value is actually impossible to reach. + * A 128 KiB block containing many 3-byte matches can have enough sequences + * for more splits than this limit allows. */ static void ZSTD_deriveBlockSplitsHelper(seqStoreSplits* splits, size_t startIdx, size_t endIdx, @@ -4267,6 +4267,8 @@ ZSTD_deriveBlockSplitsHelper(seqStoreSplits* splits, size_t startIdx, size_t end if (estimatedFirstHalfSize + estimatedSecondHalfSize < estimatedOriginalSize) { DEBUGLOG(5, "split decided at seqNb:%zu", midIdx); ZSTD_deriveBlockSplitsHelper(splits, startIdx, midIdx, zc, origSeqStore); + if (splits->idx >= ZSTD_MAX_NB_BLOCK_SPLITS) + return; splits->splitLocations[splits->idx] = (U32)midIdx; splits->idx++; ZSTD_deriveBlockSplitsHelper(splits, midIdx, endIdx, zc, origSeqStore); @@ -4289,6 +4291,7 @@ static size_t ZSTD_deriveBlockSplits(ZSTD_CCtx* zc, U32 partitions[], U32 nbSeq) return 0; } ZSTD_deriveBlockSplitsHelper(&splits, 0, nbSeq, zc, &zc->seqStore); + assert(splits.idx <= ZSTD_MAX_NB_BLOCK_SPLITS); splits.splitLocations[splits.idx] = nbSeq; DEBUGLOG(5, "ZSTD_deriveBlockSplits: final nb partitions: %zu", splits.idx+1); return splits.idx; @@ -4310,7 +4313,7 @@ ZSTD_compressBlock_splitBlock_internal(ZSTD_CCtx* zc, BYTE* op = (BYTE*)dst; size_t i = 0; size_t srcBytesTotal = 0; - U32* const partitions = zc->blockSplitCtx.partitions; /* size == ZSTD_MAX_NB_BLOCK_SPLITS */ + U32* const partitions = zc->blockSplitCtx.partitions; /* includes the final block boundary */ SeqStore_t* const nextSeqStore = &zc->blockSplitCtx.nextSeqStore; SeqStore_t* const currSeqStore = &zc->blockSplitCtx.currSeqStore; size_t const numSplits = ZSTD_deriveBlockSplits(zc, partitions, nbSeq); diff --git a/lib/compress/zstd_compress_internal.h b/lib/compress/zstd_compress_internal.h index c15a94da3b5..afebbdae33d 100644 --- a/lib/compress/zstd_compress_internal.h +++ b/lib/compress/zstd_compress_internal.h @@ -465,7 +465,7 @@ typedef struct { SeqStore_t currSeqStore; SeqStore_t nextSeqStore; - U32 partitions[ZSTD_MAX_NB_BLOCK_SPLITS]; + U32 partitions[ZSTD_MAX_NB_BLOCK_SPLITS + 1]; /* splits, followed by the end of the block */ ZSTD_entropyCTablesMetadata_t entropyMetadata; } ZSTD_blockSplitCtx; diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 4a2f0bac8f8..06e9830bfa4 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -1040,6 +1040,84 @@ static unsigned test_convertSequences_noRepcodes(unsigned seed, unsigned testNb) return testNb; } +static U32 FUZ_blockSplitHash(U32 x) +{ + x ^= x >> 16; + x *= 0x7feb352dU; + x ^= x >> 15; + x *= 0x846ca68bU; + return x ^ (x >> 16); +} + +/* The input is all zeroes, so all these offsets are valid matches. + * Groups of 150 sequences have distinct offset-code distributions, making + * enough dyadic splits beneficial to exhaust the block splitter's budget. */ +static size_t FUZ_blockSplitSequenceProducer(void* state, + ZSTD_Sequence* out, size_t capacity, + const void* src, size_t srcSize, const void* dict, size_t dictSize, + int compressionLevel, size_t windowSize) +{ + size_t i; + (void)state; (void)src; (void)dict; (void)dictSize; + (void)compressionLevel; (void)windowSize; + CHECK_EQ(srcSize, 128000); + CHECK(capacity >= 38401); + for (i = 0; i < 38400; ++i) { + U32 const code = 3 + FUZ_blockSplitHash((U32)(i / 150)) % 10; + out[i].offset = (1U << code) + (U32)(i % 4); + out[i].litLength = i == 0 ? 12800 : 0; + out[i].matchLength = 3; + out[i].rep = 0; + } + memset(out + i, 0, sizeof(*out)); + return i + 1; +} + +static unsigned test_blockSplitLimit(unsigned testNb) +{ + size_t const srcSize = 128000; + size_t const dstCapacity = ZSTD_compressBound(srcSize); + BYTE* const src = (BYTE*)calloc(1, srcSize); + BYTE* const dst = (BYTE*)malloc(dstCapacity); + BYTE* const decoded = (BYTE*)malloc(srcSize); + ZSTD_CCtx* const cctx = ZSTD_createCCtx(); + ZSTD_FrameHeader header; + size_t cSize, pos, nbBlocks = 0; + + DISPLAYLEVEL(3, "test%3u : external sequences exhaust the block split budget : ", testNb++); + CHECK(src != NULL && dst != NULL && decoded != NULL && cctx != NULL); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, 19)); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_minMatch, 3)); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_splitAfterSequences, ZSTD_ps_enable)); + CHECK_Z(ZSTD_CCtx_setParameter(cctx, ZSTD_c_validateSequences, 1)); + ZSTD_registerSequenceProducer(cctx, NULL, FUZ_blockSplitSequenceProducer); + cSize = ZSTD_compress2(cctx, dst, dstCapacity, src, srcSize); + CHECK_Z(cSize); + CHECK_EQ(ZSTD_getFrameHeader(&header, dst, cSize), 0); + pos = header.headerSize; + for (;;) { + U32 blockHeader; + CHECK(pos + ZSTD_blockHeaderSize <= cSize); + blockHeader = MEM_readLE24(dst + pos); + pos += ZSTD_blockHeaderSize; + pos += ((blockHeader >> 1) & 3) == bt_rle ? 1 : blockHeader >> 3; + CHECK(pos <= cSize); + ++nbBlocks; + if (blockHeader & 1) break; + } + CHECK_EQ(pos, cSize); + CHECK_EQ(nbBlocks, ZSTD_MAX_NB_BLOCK_SPLITS + 1); + CHECK_EQ(cctx->blockSplitCtx.partitions[ZSTD_MAX_NB_BLOCK_SPLITS], 38400); + CHECK_EQ(ZSTD_decompress(decoded, srcSize, dst, cSize), srcSize); + CHECK_EQ(memcmp(src, decoded, srcSize), 0); + ZSTD_freeCCtx(cctx); + free(decoded); + free(dst); + free(src); + DISPLAYLEVEL(3, "OK \n"); + return testNb; +} + static unsigned test_get1BlockSummary(unsigned testNb) { static const ZSTD_Sequence nseqs[] = { @@ -4729,6 +4807,7 @@ static int basicUnitTests(U32 const seed, double compressibility) testNb = test_convertSequences_noRepcodes(seed, testNb); testNb = test_get1BlockSummary(testNb); + testNb = test_blockSplitLimit(testNb); DISPLAYLEVEL(3, "test%3i : ZSTD_compressSequencesAndLiterals : ", testNb++); {