Repository navigation
CI validation: COVER multi-key quicksort, rebased (do not merge) - #5
Closed
jaypatrickhoward wants to merge 52 commits into
Closed
jaypatrickhoward wants to merge 52 commits into
jaypatrickhoward wants to merge 52 commits into
Conversation
ZSTD_compressSequences() validates every block through
determine_blockSize(), which in explicit-delimiter mode rejects a block
whose regenerated size exceeds cctx->blockSizeMax:
if (explicitBlockSize > blockSize)
RETURN_ERROR(externalSequences_invalid,
"sequences incorrectly define a too large block");
ZSTD_compressSequencesAndLiterals_internal() only supports explicit
delimiters, but does not perform that check. It calls
ZSTD_get1BlockSummary() and validates block.litSize against litSize
only, so sequences describing an over-sized block are accepted and the
function returns success while emitting a block that exceeds the
maximum block size the format allows.
The two APIs disagree on identical input:
ZSTD_compressSequences -> External sequences are not valid
ZSTD_compressSequencesAndLiterals -> SUCCESS (cSize = 36)
For a large enough block the resulting frame is not decodable, so the
caller receives a success return for bytes that cannot be read back.
The guard was added for the sibling path in 87dcd33 (2022). The
AndLiterals pipeline was split out later, in 14a21e4 (2024), without
carrying it over.
basicUnitTests() already declares size_t cSize, so the inner declaration tripped -Wshadow and failed builds using MOREFLAGS=-Werror.
… not enabled Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
…efDDict(NULL) When ZSTD_d_refMultipleDDicts is used, the DCtx keeps a hashset of DDict pointers. ZSTD_DCtx_reset(session_and_parameters) and ZSTD_DCtx_refDDict(dctx, NULL) cleared the dictionary but left the set populated, so later references were added to a set that still held pointers to DDicts the caller may have freed. A frame asking for the dictID of one of those then made ZSTD_DCtx_selectFrameDDict() read the freed DDict. Free the set in both places, and add a test that decodes a frame after the referenced DDict has been freed.
… use With nbWorkers >= 1, a job of an abandoned frame can still be running when the context frees memory it uses: - ZSTD_freeCCtx() frees the dictionary before ZSTDMT_freeCCtx() joins the workers; - ZSTD_CCtx_reset(session_and_parameters), or a session reset followed by loading another dictionary, frees it while the job runs; - with ZSTD_CCtx_refThreadPool(), ZSTDMT_freeCCtx() does not stop the shared pool and frees the jobs table and CCtx pool under the worker; - after a session reset, raising nbWorkers resizes the CCtx pool before the abandoned jobs are waited for. Move the "previous compression not correctly finished" wait of ZSTDMT_initCStream_internal() into ZSTDMT_waitForPendingJobs(), and call it on session reset, at the start of ZSTDMT_freeCCtx(), and before ZSTDMT_resize(). Free the MT context before the dictionaries. Fixes meta#4862 Signed-off-by: Eshan05 <nahareshan05@gmail.com>
The macos-14 runner image is no longer available: the job was cancelled before starting, with "The configuration 'macos-14-arm64-us-default' is not supported". macos-latest is an ARM64 (Apple Silicon) runner.
ci: run the macOS ARM64 CMake job on macos-latest
--memory (128 MiB by default) only limited the decompression of zstd frames: liblzma was initialized without any limit. Pass it to liblzma as well. When a file needs more, report how much memory it needs and suggest a --memory= value, as for zstd frames. .xz and .lzma files that need more than 128 MiB to decode now require --memory=, like zstd frames with a large window.
Apply --memory to .xz and .lzma decompression
reject out-of-range numFrames in seekable seek table loader
…rflow programs: fix `zstd -l` infinite loop on a large skippable frame
The CLI does not follow symbolic links by default (only -f does), but on Windows UTIL_isLink() always returned 0, and the recursive directory walk entered symbolic links and junctions like ordinary directories. -r could then compress, and with --rm delete, files outside the given directory. Treat name surrogate reparse points, i.e. symbolic links and junctions, as links, like lstat() does on POSIX. Other reparse points, such as OneDrive placeholders or deduplicated files, remain regular files and directories. Fixes meta#4853. The test comes from meta#4860. Co-authored-by: Nmd-sketch <revusarlic@gmail.com>
programs: don't follow symbolic links and junctions in -r on Windows
ZSTD_seekable_decompress() limited len with offset + len > eos. An offset well past the end made eos - offset underflow into a huge byte count that ZSTD_isError() did not flag. An offset + len that wrapped around skipped the limit, and the loop never exited. Return parameter_outOfBound for an offset past the end, and compare len against eos - offset. Reading at the end still returns 0.
…ocks When a block's literals are split between dst and litExtraBuffer, the first part of the literals is stored at the end of the block's area in dst. A block that decodes to more than that area could overwrite these literals before reading them, and decode to wrong output: - ZSTD_safecopyDstBeforeSrc() used ZSTD_wildcopy() when src was more than 16 bytes ahead of dst, but ZSTD_wildcopy() can write up to 31 bytes past its length, onto source bytes not yet copied. Only use it when src is at least WILDCOPY_OVERLENGTH bytes ahead. - ZSTD_execSequenceEndSplitLitBuffer() only detected op starting inside the literals being copied. Reject any sequence whose output runs past the end of the literals it consumes, which also covers matches overwriting the literals of the next sequences. - ZSTD_decompressFrame() didn't check that a block decodes to at most blockSizeMax, unlike ZSTD_decompressContinue(). One-shot decompression accepted such blocks, including raw and RLE blocks larger than ZSTD_d_maxBlockSize. Valid blocks never decode to more than MIN(blockSizeMax, dstCapacity), so the output stays at least 64 KB - 32 bytes behind the literals in dst, and isn't affected. All writes were within dst, so this was not a memory safety issue. Frames with blocks larger than the window are now rejected by the one-shot API, as they already were by streaming. Add tests with a crafted block for the block and frame APIs, and for ZSTD_d_maxBlockSize with raw and RLE blocks.
…e data The return value is smaller than dstSize when the data ends first, 0 when offset is the end of the data, and an error past it.
…st-end seekable_format: Reject offsets past the end in decompress
The output file was opened with O_CREAT|O_TRUNC, which follows a symbolic link. stat() fails on a dangling link, so it was not seen as an existing file: zstd created its target and wrote the output there, even without -f. A new output file is now created with O_EXCL, which fails on any symbolic link, even a dangling one, including a link that appears after the existence check or after an existing file was removed. A dangling link now counts as an existing file: it is not overwritten without -f, and -f replaces the link, as it already did for a link to an existing file. Existing files that are not regular files, such as /dev/null, /dev/stdout or pipes, are still opened as they are. They are no longer truncated, and they are refused if they turn out to be regular files once opened. A regular file that could not be removed is still truncated in place.
programs: don't write the output through a symbolic link
Use the shared CCtx and the fixed dictionary for ddictB, compress the frame with ZSTD_compress_usingDict(), and run the reset and the refDDict(NULL) cases in one loop.
…ctor-and-reset decompress: drop the DDict hashset on ZSTD_DCtx_reset and ZSTD_DCtx_refDDict(NULL)
ZSTDMT_waitForAllJobsCompleted() waited while consumed < src.size. An empty job, the only job of an empty frame, meets that condition before its worker starts, so the wait returned at once. The previous commit calls ZSTDMT_waitForPendingJobs() at the start of ZSTDMT_freeCCtx(), before POOL_free() joins the workers. When an empty frame was abandoned (ZSTD_e_end returned because the output buffer was full), freeing the context cleared the job while its worker started, and the worker dereferenced a NULL pool. Before that commit, POOL_free() joined the worker first. The worker now sets a completed flag when it no longer uses the job, and the wait uses this flag. The test gets a scenario for this case.
ZSTDMT_releaseAllJobResources() clears every job descriptor, including the job prepared while no worker was free (jobReady), but it left jobReady set. After a frame was abandoned in that state, for example with ZSTD_CCtx_reset(), the first ZSTDMT_createCompressionJob() of the next frame posted the cleared descriptor, and the worker dereferenced a NULL pool. The test gets a scenario for this case.
Remove the scenario that raises nbWorkers after a reset. Since ZSTD_CCtx_reset() waits for the jobs, this scenario can only fail when the one that loads a new dictionary after a reset fails too. The new frames are compressed, but no longer decompressed: the bugs show as memory errors in the worker. Each of the five scenarios still fails when its library change is removed.
compress: wait for an abandoned frame's jobs before freeing what they use
ZSTD_CCtx_refThreadPool() only changed cctx->pool. Once created, the MT context kept the pool it was created with, so the next frames still ran on the previous pool, including after ZSTD_CCtx_refThreadPool(cctx, NULL). If the caller then freed the previous pool, the next frame used freed memory. ZSTD_CCtx_refThreadPool() now frees the MT context when the pool changes, and the next frame creates one with the new pool. The pool can only change between frames, and ZSTDMT_freeCCtx() waits for the jobs of an abandoned frame.
When ZSTD_c_nbWorkers changed between two frames, ZSTDMT_resize() called POOL_resize() on the context's pool, even on a pool given with ZSTD_CCtx_refThreadPool(). One context could then change the number of threads of a pool shared with other contexts, although such a pool is meant to limit them. ZSTDMT_createCCtx_advanced() already leaves a provided pool unchanged. ZSTDMT_resize() now does the same.
compress: fix ZSTD_CCtx_refThreadPool() rebinding and shared pool resizing
) * Fix out-of-bounds read in FASTCOVER on small training samples When every training sample is smaller than the dmer size, splitPoint < 1 can shrink the training set below MAX(d, 8) after the total samples size check passes, so nbDmers underflows and FASTCOVER reads past the sample buffer (meta#4499). It now returns srcSize_wrong. Adds a unit test in basicUnitTests. The same underflow in COVER_ctx_init is addressed separately in meta#4690. * Name the FASTCOVER dmer read length
…4690) * Fix size_t underflow in COVER_ctx_init for small training sets When splitPoint < 1.0, the COVER dictionary builder trains on a subset of the corpus, so trainingSamplesSize can be smaller than the total even after the existing totalSamplesSize check. The partial suffix array size is computed as `trainingSamplesSize - MAX(d, sizeof(U64)) + 1`, which underflows size_t when the training set is too small, producing an enormous allocation request instead of a clean error. Guard against an undersized training set before the subtraction and return srcSize_wrong, and add a regression test in fuzzer.c. Fixes meta#4682. * Factor out COVER read length calculation --------- Co-authored-by: Kevin Zhang <kevz8@fb.com>
Same case in 12 lines instead of 43: the sequences are initialized in their declaration, the literals come from CNBuffer, and CHECK() replaces the error branch.
…ls() The caller delimits the blocks, and a block larger than the maximum block size now generates an error.
…4765) * dictBuilder: break COVER sort ties by position, not by address stableSort() is documented to leave each dmer group ordered by position in the input, and COVER_group() depends on it: it counts how many samples a dmer occurs in using a forward-only cursor, so a group whose positions are not ascending has occurrences silently dropped. COVER_strict_cmp() tried to provide that ordering by breaking ties on `lp < rp`. But lp and rp are the addresses of the elements being compared, not the positions they hold. Those coincide only until qsort() performs its first swap; afterwards the tie-break orders by where an element currently sits rather than by what it contains. The result is a comparator that is not a function of the values it compares, with three consequences: - Output is not reproducible. When qsort() compares an element against a temporary (a pivot copy, which is common), one operand is not in the array at all, so the comparison is stack-vs-heap and ASLR decides it. The same binary on the same input produced two different dictionaries in six consecutive runs. - Output depends on the C library. glibc happens to satisfy the invariant; Apple libc and MSVC do not, so they produce different dictionaries from the same input. - An inconsistent comparator breaks quicksort's partitioning assumptions. On MSVC, training on a 16 MB corpus at d=6 takes 465s; with this fix, 8s. Comparing the stored positions instead makes the key (dmer, position). Positions are unique, so no two elements compare equal, the order is total, and every conforming qsort() must produce the same arrangement. Verified: on glibc the output is unchanged (byte-identical across 215 configurations spanning 20 corpora), and on Apple libc and MSVC the output now matches what glibc produces. * dictBuilder: return 0 when COVER tie-break positions are equal COVER_strict_cmp() and COVER_strict_cmp8() break ties between equal dmers by sample position but returned 1 when the two positions were equal, so cmp(a, a) was 1. A qsort() comparator must be consistent, and that includes comparing an element with itself. Suffix positions are unique, so the case is only reachable on a self-comparison and the change does not affect output: dictionaries are byte-identical before and after across 72 corpus/k/d combinations. Add a fuzzer test for the ordering itself. It trains COVER on low-entropy samples, where ties are frequent, and checks the dictionary hash against the one position ordering produces, for both comparator paths. In-place qsort() implementations order ties by partition state under the former address comparison, so the test fails against it (verified on macOS) and passes with position ordering.
When splitPoint < 1.0, the COVER dictionary builder trains on a subset of the corpus, so trainingSamplesSize can be smaller than the total even after the existing totalSamplesSize check. The partial suffix array size is computed as `trainingSamplesSize - MAX(d, sizeof(U64)) + 1`, which underflows size_t when the training set is too small, producing an enormous allocation request instead of a clean error. Guard against an undersized training set before the subtraction and return srcSize_wrong, and add a regression test in fuzzer.c. Fixes meta#4682.
Treat dictionaries shorter than four bytes as raw content in the v0.5 and v0.6 decoders. Add tiny-dictionary and truncated-header regression coverage across all supported frame formats and both decompression APIs.
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.
…-and-literals-block-size Reject a too large block in ZSTD_compressSequencesAndLiterals()
Full-format dictionaries may contain repeat offsets into content discarded by the short-cache CDict limit or the general index limit. Revalidate all three repcodes after loading the retained suffix and enforce the same bound before attaching a CDict, preventing wrapped dictMatchState indices and wild reads. Add regression coverage for 20 MiB dictionaries, every repcode slot, fast/dfast levels, deferred and streaming dictionary loading, the short-cache boundary, and valid non-truncating controls.
…iterals() Kernel callers read include/linux/zstd.h, which did not mention that lit_capacity must be at least 8 bytes larger than lit_size. This is now enforced, so a smaller capacity generates an error.
Enforce literal buffer capacity contract
Without the tests, C++ is not enabled and CMAKE_CXX_COMPILER_ID is empty. A clang build for the MSVC ABI then named its static library zstd.lib, like the import library of the DLL (meta#4513), and a clang-cl build processed the resource file again, which 7e0324e had disabled. Add a Clang-CL CI job without the tests.
…4800 cmake: fix CXX flag dependency in AddZstdCompilationFlags when C++ is not enabled
stableSort() reached qsort() through a five-variant selection block (ZDICT_QSORT_APPLE/GNU/MSVC/C11/C90) that picked between qsort_r(), qsort_s() and a non-reentrant global, plus the _GNU_SOURCE and __STDC_WANT_LIB_EXT1__ feature tests needed to expose them. Sort the dmers directly instead, with a multi-key quicksort (Bentley & Sedgewick): partition on a single dmer byte and recurse into the equal bucket one byte deeper. A byte proven equal is never examined again, which matters here because dmers are heavily duplicated and a comparison sort restarts every comparison at byte 0. The order produced is the same total order as before -- by dmer, then by position -- so dictionaries are unchanged. Byte order follows each comparator: COVER_cmp8() compares a masked little-endian U64, making byte d-1 most significant, while COVER_cmp() uses memcmp(), making byte 0 most significant. MEM_readLE64() byte-swaps on big-endian hosts, so this holds on every architecture. Recursion descends into the two smaller spans and loops on the largest. If L is the largest span and A is either other, A <= L and A + L <= n, so A <= n/2 and depth is bounded by log2(n) regardless of d or input shape. Measured maximum depth is 18 across corpora designed to stress it. The sort is in-place and allocates nothing, the comparison is inlined rather than reached through a function pointer, and the platform dispatch and its global are removed. With no qsort_r() left to detect, the ZDICT_QSORT build macro goes too: drop its entry from lib/README.md, and its override from the musl CI job, whose musl 1.2.2 has no qsort_r() and now builds without the override.
zstd_deps.h defined _GNU_SOURCE on Linux, Cygwin and MSYS solely to expose qsort_r() to the dictionary builder, as its comment said. The COVER sort no longer calls qsort_r(), so nothing in lib/ needs it, and a library header should not opt every translation unit that includes it into GNU extensions.
jaypatrickhoward
force-pushed
the
cover-mkq-sort
branch
from
November 11, 2026 13:58
0c51848 to
2c8ce5d
Compare
Owner
Author
|
Validation complete; closing (fork-only CI run). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fork-only run of the full CI matrix for the COVER multi-key quicksort branch after its rebase onto current upstream dev, before it is proposed upstream. Do not merge.