Reject a too large block in ZSTD_compressSequencesAndLiterals()

[nileshpatil6]

Summary

ZSTD_compressSequencesAndLiterals() accepts sequences that describe a block larger than cctx->blockSizeMax and returns success, while its sibling ZSTD_compressSequences() rejects the same input.

Detail

ZSTD_compressSequences_internal() routes every block through determine_blockSize(), which in explicit-delimiter mode enforces:

if (explicitBlockSize > blockSize)
    RETURN_ERROR(externalSequences_invalid, "sequences incorrectly define a too large block");

ZSTD_compressSequencesAndLiterals_internal() supports only explicit delimiters, so it is exactly the mode that guard was written for, but its block loop validates just the literals:

BlockSummary const block = ZSTD_get1BlockSummary(inSeqs, nbSequences);
FORWARD_IF_ERROR(block.nbSequences, "...");
RETURN_ERROR_IF(block.litSize > litSize, externalSequences_invalid, "discrepancy: ...");
ZSTD_resetSeqStore(&cctx->seqStore);

so block.blockSize is never checked against cctx->blockSizeMax.

Reproduction

One sequence plus a block delimiter, describing a single block of ZSTD_BLOCKSIZE_MAX + 1 bytes, given to both APIs:

block regenerated size = 131073, ZSTD_BLOCKSIZE_MAX = 131072
ZSTD_compressSequences            -> External sequences are not valid
ZSTD_compressSequencesAndLiterals -> SUCCESS (cSize = 36)

The caller gets a success return for a frame containing a block that exceeds the maximum block size the format permits. How bad the result is depends on how far over the limit the block goes. At ZSTD_BLOCKSIZE_MAX + 1 the frame still happens to round-trip through ZSTD_decompress(); at a larger size it does not:

block regenerated size = 200016, ZSTD_BLOCKSIZE_MAX = 131072
ZSTD_compressSequencesAndLiterals -> SUCCESS (cSize = 36)
  round-trip ZSTD_decompress      -> Data corruption detected

So for a sufficiently over-sized block the API reports success for output that cannot be read back. ZSTD_compressSequencesAndLiterals() is aimed at callers that hold literals and sequences separately and compute block boundaries themselves, which is precisely where a mis-sized block is plausible.

How it happened

The guard was introduced for the sibling path in 87dcd33, "fix sequence compression API in Explicit Delimiter mode" (2022-01-22), and later moved into determine_blockSize(). The AndLiterals pipeline was split out afterwards in 14a21e4, "produced ZSTD_compressSequencesAndLiterals() as a separate pipeline" (2024-12-11), without carrying the check across.

Change

One RETURN_ERROR_IF, using the same error code and message as the sibling, so both APIs now reject identical input identically.

I deliberately kept this to the single check I can demonstrate. determine_blockSize() also guards explicitBlockSize > remaining; I tested that case here and it is already rejected on this path, so I have not added a second line for it.

Testing

New unit test in tests/fuzzer.c next to the existing ZSTD_compressSequencesAndLiterals tests.

Without the fix:

test321 : ZSTD_compressSequencesAndLiterals : reject a too large block : ZSTD_compressSequencesAndLiterals() should have failed: sequences define a block larger than ZSTD_BLOCKSIZE_MAX
Error detected in Unit tests !

tests/fuzzer -v -i0 exits 1.

With the fix:

test321 : ZSTD_compressSequencesAndLiterals : reject a too large block : OK

tests/fuzzer -v -i0 exits 0.

No regressions: tests/fuzzer -i3000 completes 3000 rounds and tests/zstreamtest -i2000 completes 2001 rounds, both exit 0.

[xhon-pelushi]

tests/fuzzer.c:4382 declares a new inner cSize, but basicUnitTests() already has size_t cSize at line 988. The default fuzzer build only warns, but the warning-as-error path fails locally:

$ make -C tests clean
$ make -j$(nproc) -C tests fuzzer MOREFLAGS=-Werror
fuzzer.c:4382:16: error: declaration of ‘cSize’ shadows a previous local [-Werror=shadow]

Renaming this local test variable, or reusing the outer one, should keep the PR passing the -Werror jobs.

I verified the functional fix as well: on the parent commit with only the new test transplanted, ./tests/fuzzer -v -i0 fails at test321; on this head, make -C tests fuzzer, ./tests/fuzzer -v -i0, and ./tests/fuzzer -i3000 pass. I also checked the sibling ZSTD_compressSequences_internal() path: explicit delimiter block size is already bounded by determine_blockSize(), and this PR's guard runs before the AndLiterals path subtracts block.blockSize from remaining.

[nileshpatil6]

Good catch, thank you, and thanks for independently reproducing the functional part.

Fixed in bf7504b: the inner local is now cSizeTooLarge, so it no longer shadows the cSize declared at the top of basicUnitTests(). I reproduced your failure first and confirmed the fix:

$ make -C tests clean && make -j$(nproc) -C tests fuzzer MOREFLAGS=-Werror
fuzzer.c:4382:16: error: declaration of 'cSize' shadows a previous local [-Werror=shadow]
fuzzer.c:988:12: note: shadowed declaration is here

After the rename that build is clean, and the test still discriminates: ./tests/fuzzer -v -i0 exits 0 on this head and exits 1 with Error detected in Unit tests ! when the guard in zstd_compress.c is removed.

[xhon-pelushi]

Re-checked bf7504bf1a65e48faa14f0293b1a961a81534b8e. The shadowing issue is fixed: the new local is cSizeTooLarge, and the warning-as-error build now passes for the fuzzer target:

make -C tests clean
make -j$(nproc) -C tests fuzzer MOREFLAGS=-Werror
./tests/fuzzer -v -i0

test321 still passes on the updated head, so the oversized-block regression coverage remains intact. I do not see anything else blocking from my side.