Consolidate the pending bug fixes: 8 open PRs plus the fz-branch findings by ayzk · Pull Request #144 · szcompressor/SZ3

ayzk · 2026-09-01T06:22:11Z

Bug fixes only. Nothing here changes the compressed format or an interface signature, and every algorithm's output is byte-identical to master's -- verified over 31 combinations of dataset (miranda velocityx.d64 float64, cesm-atm ODV_bcar1 float32, the in-tree smoke file), algorithm and error bound, plus decompressing files master itself produced.

Supersedes #143.

The open pull requests, reviewed line by line

PR	Defect	Taken
#131	`Config::load` computed the end of the config blob as `c + confSize` after `c` had already advanced past the length prefix, one byte too far. `save()` includes the prefix in `confSize`, so the end is `c0 + confSize`. The last optional-field guard then fires with one byte left and reads a field that is not there -- past the end of the buffer, since the config is the last thing in the stream.	yes (via #132's rewrite, which contains the same fix)
#132	Bounds-checks the whole decompression path: header size in `SZ_decompress` (which previously ignored the `cmpSize` it was given entirely), a bounded `Config::load` overload with `N`/bit-width/dims-product validation, `Lossless_zstd` (including returning a zstd error code as a size), Huffman tree loading, `unpad_tree` child indices, predictor selection indices, `recover_unpred`, `unpred_size` before `resize`, OMP thread counts and per-thread sizes, and RAII for buffers freed on the throwing paths.	yes, except two parts -- see below
#133	`out1 << (64 - len)` with `len == 0` -- a single-symbol Huffman tree, which a constant field produces -- shifts a 64-bit value by 64.	yes
#134	`HuffmanEncoderV2::loadAsDFSOrder` read a fixed header and a DFS bitstream with no bounds at all, `reserve(tree.n << 1)` overflowed, and it advanced the cursor without ever decrementing `remaining_length`.	yes, plus two gaps closed below
#135	`smallIdx`, read from the compressed stream, indexed the fixed-size `magicInts` table unchecked; the first `malloc` in `XtcBasedEncoder` was overwritten and leaked; `Lossless_bypass` did not check its `malloc`.	yes
#137	`RegressionPredictor::load` debited `remaining_length` by the decoded bin count while `decode()` advances by the much smaller encoded byte count, so the budget ran out early; `ComposedPredictor::load` had the mirror-image bug and never debited it at all, leaving the bound too loose.	yes
#138	`static_cast<int64_t>(fabs(diff) * error_bound_reciprocal)` is UB for NaN, infinities and huge magnitudes.	yes -- observable behaviour is unchanged (NaN/inf/1e38 produce the same bin and the same written-back value before and after)
#139	Scratch buffers were `free()`d at the end of a function that can throw; and the OMP path's per-chunk capacity omitted the size header `Lossless_zstd::compress` writes, so a poorly compressible chunk threw.	yes

The two parts of #132 not taken

The compressed format change. #132 moves the quant_inds count from after the encoder's serialized tree to before it, so the tree is immediately followed by its encoded stream and the bound load() records is exact. That is a format change and SZ3_DATA_VERSION is not bumped, so the version check passes and a file written by any released SZ3 fails with a misleading SZ3 Huffman: invalid node count. Verified by decompressing a master-produced file with #132's build.

The bound it was buying is recovered without touching the format, by letting the caller supply it instead of having load() infer it:

// HuffmanEncoder
void set_decode_bound(size_t remaining);

SZGenericCompressor calls it once it has consumed the count field, and both predictors call it because their tree and stream are contiguous. load() no longer records a bound at all: the tree and the encoded stream are not required to share a buffer, and tools/test/modules/test_encoder.cpp deliberately puts them in separate ones -- inferring the bound there made that test fail.

The dstLen reinterpretation. #132 treats LosslessInterface::decompress's dstLen as an input capacity when the caller supplies the buffer. The declared contract is that it is an output, and callers do pass uninitialised values -- LosslessTest.LosslessBypass fails with it, and LosslessTest.LosslessZstd only passed because the garbage happened to be large. The self-allocating branch, where a non-zero incoming value is opt-in and zero means no bound, is kept.

Gaps found while reviewing, fixed here

HuffmanEncoderV2 sizes its dense tables with veccode.resize(tree.maxval), and maxval is read straight from the stream. Bounds-check HuffmanEncoderV2 tree loading against corrupted input #134 bounds the node count but not this, leaving an unbounded allocation. Bounded by the encoder's own invariant (preprocess_encode only leaves usemp == 0 while maxval < 1 << 28).
tree.n is an int holding a value read as 64-bit, so Bounds-check HuffmanEncoderV2 tree loading against corrupted input #134's new bound is checked with an explicit sign test rather than letting the comparison convert it (which also silenced a -Wsign-compare warning the PR introduced).
tools/test/modules/test_lossless.cpp passed an uninitialised size to decompress().
Nothing in the suite exercised the HDF5 filter's buffer sizing: a chunk small enough for the
compressed block to exceed the raw chunk size never appeared. test_h5_filter.py gains a third
chunk mode that asks for 8-element chunks, which the pre-fix filter rejects with
buffer not large enough on every algorithm. The mode is restricted to a leading slice of at
most 4096 chunks -- over a whole 280M-element field it would mean 35M chunks, and the HDF5 chunk
index alone takes the process to 15.3 GiB of a runner's 16 GiB. Measured in a 16 GiB container at
hacc's exact size: 9.6 GiB and passing.

From the `fz` branch, verified to reproduce here

Lossless_bypass::compress ignored its destination capacity and memcpy'd the payload regardless -- an unconditional heap overflow whenever the payload is larger than the caller's buffer. ALGO_BIOMDXTC is the one algorithm that pairs its codec with bypass, so nothing shrinks the payload; AddressSanitizer on Linux caught it as a 59599-byte overflow, glibc reported munmap_chunk(): invalid pointer, and macOS's allocator did not abort at all.
TimeSeriesDecomposition violated its error bound 1.94x with data_ts0 == nullptr: block_data's compress-side path has no write-back, so timestep 0 was never updated with its reconstruction while decompress() predicted from its own. 0.997x after the fix.
ArithmeticEncoder sign-extended bytesToInt64_bigEndian(bytes) >> 20, pushing the value outside the 44-bit MAX_CODE window. A 60-stream sweep gave 30 correct, 23 wrong, 7 SIGSEGV.
HuffmanEncoder's stateNum = max - offset + 2 narrows to int, so a wide bin range goes negative and the state-table malloc is UB.
RunlengthEncoder, NoPredictionDecomposition and InterpolationDecomposition had no size_est(), so SZGenericCompressor sized its buffer from 0 while the quantizer's unpredictable list could be arbitrarily large.
KmeansUtil had reserve() followed by operator[] writes, and uniform_int_distribution(0, num) indexing data[num]. Live in mdz when dims[1] > 5000.
The HDF5 filter sized its compressed buffer below SZ_compress's own minimum, so an explicit chunk of a few hundred elements aborted.
24 headers were not self-contained under one or both standard libraries. libc++ provides more transitively than libstdc++, so this is latent until a translation unit changes its include order -- it broke the Linux build on fz once a new test file did.
.gitignore's bare test pattern matched tools/test/, hiding every test file from git.

Verification

clang and GCC 16, Release + BUILD_TESTING=ON + BUILD_H5Z_FILTER=ON: 0 errors, 0 warnings, all tests pass
Every header compiles standalone under both toolchains (57/57)
31 dataset x algorithm x error-bound combinations byte-identical to master; every master-produced file decompresses to the same bytes
Each ported fix was A/B'd against its unfixed state

Generated with Claude Code

…ings Bug fixes only. Nothing here changes the compressed format or an interface signature, and every algorithm's output is byte-identical to master's (verified over 31 dataset/algorithm/error-bound combinations, plus decompressing files master produced). From the open pull requests, reviewed line by line: #131 Config::load read one byte past the end of the config blob #132 bounds-check the whole decompression path (see the two skips below) #133 shift-by-64 UB for a single-symbol Huffman tree #134 bounds-check HuffmanEncoderV2 tree loading #135 bounds-check XtcBasedEncoder's magicInts index; plug a leaked buffer #137 remaining_length accounting after Huffman decode in both predictors #138 non-finite float to int64_t cast UB in LinearQuantizer #139 scratch buffers leaked when compression throws; OMP chunk capacity was missing room for the size header Lossless_zstd writes Two parts of #132 are deliberately not taken: - It moves the quant_inds count from after the encoder's tree to before it so the tree is immediately followed by its encoded stream. That is a compressed format change and it is not versioned, so files written by any released SZ3 fail to decode. The bound it was buying is recovered instead by letting the caller supply it: HuffmanEncoder::set_decode_bound(), called by SZGenericCompressor once it has consumed the count, and by both predictors. load() no longer guesses the bound from its own buffer -- the tree and the stream are not required to share a buffer, and test_encoder.cpp puts them in separate ones. - It reinterprets LosslessInterface::decompress's `dstLen` as an input capacity when the caller provides the buffer. The declared contract is that it is an output, and callers do pass uninitialised values, so this fails nondeterministically (it broke LosslessTest.LosslessBypass here). The self-allocating branch, where a non-zero value is opt-in, is kept. Also fixed while reviewing: - HuffmanEncoderV2 sized its dense tables from an unbounded `maxval` read straight from the stream; #134 bounds the node count but not this. - HuffmanEncoderV2's node count is an int holding a value read as 64-bit, so the new bound is checked with an explicit sign test rather than an implicit conversion. - test_lossless.cpp passed an uninitialised size to decompress(). From the fz branch, verified to reproduce here: Lossless_bypass::compress ignored its destination capacity, so any payload larger than the caller's buffer was an unconditional heap overflow; TimeSeriesDecomposition violated its error bound 1.94x on the null-reference-frame path; ArithmeticEncoder sign-extended a shift, corrupting about half of all streams; HuffmanEncoder's stateNum narrowing overflowed on a wide bin range; RunlengthEncoder and two decompositions had no size_est(), so the compressor sized its buffer from 0; KmeansUtil had reserve+operator[] UB and an off-by-one read; the HDF5 filter sized its buffer below SZ_compress's own minimum; 24 headers were not self-contained under libstdc++ or libc++; .gitignore's bare `test` pattern hid every test file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Taking the fz-branch fixes file by file overwrote them. The magicInts index and the packed-data size both come from the compressed stream and were used unchecked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR consolidates a large set of hardening and correctness bug fixes across the SZ3 library (core decompression safety, lossless codecs, predictors/encoders, and portability via self-contained headers), while aiming to keep formats and outputs byte-identical to master.

Changes:

Adds systematic bounds checks and fail-closed behavior on multiple decompression paths to prevent OOB reads/writes on corrupted inputs.
Fixes several correctness/UB issues (e.g., NaN/Inf quantization cast UB, Huffman single-symbol shift UB, arithmetic decoder sign-extension).
Improves robustness/portability: RAII for scratch buffers on throwing paths, adds missing standard/project includes, adjusts .gitignore.

Reviewed changes

Copilot reviewed 40 out of 41 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tools/test/modules/test_lossless.cpp	Initializes output size to avoid passing garbage into `decompress()` tests.
tools/H5Z-SZ3/src/H5Z_SZ3.cpp	Sizes HDF5 filter buffer using SZ3’s size bound to avoid too-small chunks aborting.
include/SZ3/utils/Statistic.hpp	Adds missing standard headers for self-contained compilation.
include/SZ3/utils/Sample.hpp	Adds missing `<cassert>` include and keeps project include ordering correct.
include/SZ3/utils/QuantOptimization.hpp	Adds missing `<cmath>` / `def.hpp` includes for self-contained compilation.
include/SZ3/utils/MemoryUtil.hpp	Replaces release-stripped asserts with exceptions for bounded reads.
include/SZ3/utils/KmeansUtil.hpp	Fixes UB in sampling (`reserve` + `operator[]`, distribution upper-bound).
include/SZ3/utils/Iterator.hpp	Adds missing project header include for self-contained compilation.
include/SZ3/utils/Extraction.hpp	Adds missing includes and project headers for self-contained compilation.
include/SZ3/utils/Config.hpp	Adds bounded `Config::load` overload and validates dims/num consistency.
include/SZ3/utils/BlockwiseIterator.hpp	Adds `block_data::values()` to materialize unpadded data when padding is used.
include/SZ3/quantizer/Quantizer.hpp	Adds `quantizer_size_est()` helper to optionally use quantizer size estimates.
include/SZ3/quantizer/LinearQuantizer.hpp	Fixes float→int UB for NaN/Inf/overflow; bounds unpredictable reads/allocations.
include/SZ3/preprocessor/Transpose.hpp	Adds missing standard headers for self-contained compilation.
include/SZ3/preprocessor/PreFilter.hpp	Adds missing standard headers for self-contained compilation.
include/SZ3/predictor/RegressionPredictor.hpp	Fixes `remaining_length` accounting and bounds coefficient stream consumption.
include/SZ3/predictor/LorenzoPredictor.hpp	Avoids pointer-overflow UB by using pointer subtraction for neighbor access.
include/SZ3/predictor/ComposedPredictor.hpp	Bounds predictor selection indices and fixes `remaining_length` accounting.
include/SZ3/lossless/Lossless.hpp	Adds missing includes for interface header self-containment.
include/SZ3/lossless/Lossless_zstd.hpp	Hardened zstd decompress: header checks, bounded self-allocation, exact-size enforcement, RAII on failure.
include/SZ3/lossless/Lossless_bypass.hpp	Adds dstCap check on compress; bounds self-allocation and allocation failure checks on decompress.
include/SZ3/encoder/XtcBasedEncoder.hpp	Adds missing headers/includes needed for compilation/self-containment.
include/SZ3/encoder/RunlengthEncoder.hpp	Adds `size_est()` and tracks bin count for buffer sizing.
include/SZ3/encoder/HuffmanEncoderV2.hpp	Bounds-checks serialized tree header/bitstream and prevents untrusted large allocations.
include/SZ3/encoder/HuffmanEncoder.hpp	Adds decode bounding hook; hardens decode/tree parsing; fixes single-symbol shift UB; bounds tree indices/state table sizing.
include/SZ3/encoder/ArithmeticEncoder.hpp	Fixes sign-extension in `value` extraction by using unsigned shift.
include/SZ3/decomposition/TimeSeriesDecomposition.hpp	Fixes timestep-0 reconstruction usage when `data_ts0 == nullptr` to maintain error-bound correctness.
include/SZ3/decomposition/SZBioMDXtcDecomposition.hpp	Adds missing standard header include.
include/SZ3/decomposition/NoPredictionDecomposition.hpp	Adds a `size_est()` override for better buffer sizing.
include/SZ3/decomposition/InterpolationDecomposition.hpp	Adds stored-dimension validation on decompress and a `size_est()` override.
include/SZ3/decomposition/Decomposition.hpp	Adds missing includes to keep header self-contained.
include/SZ3/decomposition/BlockwiseDecomposition.hpp	Fixes includes and ordering for self-contained compilation.
include/SZ3/compressor/SZGenericCompressor.hpp	Adds RAII for scratch buffers, bounds internal lossless buffer allocation, and threads decode bounds to encoders when supported.
include/SZ3/compressor/specialized/SZTruncateCompressor.hpp	Uses RAII for scratch buffer to avoid leaks on thrown compression.
include/SZ3/compressor/specialized/SZExaaltCompressor.hpp	Adds missing includes and adjusts include ordering for self-containment.
include/SZ3/api/sz.hpp	Adds RAII to heap-owning compress API and bounds-checks decompression header/payload/config blob.
include/SZ3/api/impl/SZImplOMP.hpp	Adds OMP-path bounds checks and RAII for per-thread buffers; accounts for lossless header sizing.
include/SZ3/api/impl/SZDispatcher.hpp	Adds RAII to avoid leaks on thrown zstd fallback compression.
include/SZ3/api/impl/SZAlgoInterp.hpp	Adds missing include for self-contained compilation.
include/SZ3/api/impl/SZAlgoBioMD.hpp	Reorders/includes required headers explicitly for self-contained compilation.
.gitignore	Fixes `test` pattern to avoid hiding `tools/test/`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 void read(T1 *array, size_t num_elements, uchar const *&compressed_data_pos, size_t &remaining_length) {
-    assert(num_elements * sizeof(T1) <= remaining_length);
+    if (num_elements * sizeof(T1) > remaining_length) {
+        throw std::invalid_argument("SZ3: compressed stream is truncated");
+    }


+        const unsigned char* const c0 = c;
+        auto require = [&](size_t n) {
+            if (cmpSize - static_cast<size_t>(c - c0) < n)
+                throw std::out_of_range("SZ3 Config::load: read past the end of the config");
+        };


        //        }
        //        std::cout << std::endl;
-        std::uniform_int_distribution<> dis2(0, num);
+        std::uniform_int_distribution<> dis2(0, static_cast<int>(num) - 1);


+    size_t size_est() override {
+        return sizeof(original_dimensions) + sizeof(blocksize) + sizeof(interp_id) + sizeof(direction_sequence_id) +
+               quantizer_size_est(quantizer) + 128;
+    }


num_elements * sizeof(T1) can overflow on its own, which is the computation the check is supposed to guard. Matches the scalar overload and PR #132. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

It bounds that buffer by SZ_compress_size_bound, which is the size of the *output* buffer. compress() sizes the internal one as max(1000, 2 * (decomposition.size_est() + encoder.size_est() + sizeof(Q) * bins)), so for a 64-bit bin type it is far larger and valid streams are rejected. Every module in this tree emits int bins, so the bound happens to hold and these tests cannot reach it -- the fz branch has three modules that do (BitplaneEncoder, BitTruncationQuantizer, FixedPointQuantizer) and all three failed. A corrupted declared size is still caught, after the allocation, by the zstd frame check and the size comparison this PR adds to Lossless_zstd::decompress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

PR #132 rejects N outside [1, 4] and dimensions whose product differs from the element count. Both hold for a config read from the end of a compressed stream, and neither holds for the HDF5 filter's cd_values: those carry placeholder zeros that set_local fills in later, as cdvalueHelper.py says in as many words. The check killed the whole filter -- 80 of cesm-atm's 160 integration cases aborted with "invalid number of dimensions", across every algorithm. The single-argument overload, which is what the filter and the OpenMP path use, now says so explicitly and skips the content checks; the bounded overload used by SZ_decompress keeps them. Verified against the real cesm-atm field through the HDF5 filter: five algorithms pass, and removing the guard reproduces the abort. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The filter has to size its output buffer from SZ_compress_size_bound rather than from the raw chunk size, and nothing in the suite exercised that: a chunk small enough for the compressed block to exceed it never appeared. This adds a third chunk mode that asks for 8-element chunks, which the pre-fix filter rejects with "buffer not large enough" on every algorithm. The mode is restricted to a leading slice of at most 4096 chunks. Over a whole field it would mean tens of millions of chunks, and the HDF5 chunk index alone takes the process past a CI runner's memory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

master now carries the consolidated bug fixes (#144). Three conflicts, all in commentary: fz-dev had a stale one-line comment above Config::load's fuller doc block (dropped), one condition differed only in line wrapping (took master's single line, which fits the 120-column limit), and fz-dev's note on the dropped decompression-buffer bound names the modules that actually emit 64-bit bins, so it is the accurate wording here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

This reverts commit 35174f3, reversing changes made to 57ce9e9.

Copilot AI lite review requested due to automatic review settings September 1, 2026 06:22

ayzk mentioned this pull request Sep 1, 2026

Fix 14 pre-existing defects found while working on fz #143

Closed

Copilot started reviewing on behalf of ayzk September 1, 2026 06:22 View session

Restore PR #135's XtcBasedEncoder bounds

6a7985b

Taking the fz-branch fixes file by file overwrote them. The magicInts index and the packed-data size both come from the compressed stream and were used unchecked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI reviewed Sep 1, 2026

View reviewed changes

ayzk and others added 4 commits August 31, 2026 23:31

Compare by division in the bounded array read

40523ce

num_elements * sizeof(T1) can overflow on its own, which is the computation the check is supposed to guard. Matches the scalar overload and PR #132. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

ayzk merged commit 35174f3 into master Sep 1, 2026
13 checks passed

ayzk added a commit that referenced this pull request Sep 1, 2026

Revert "Consolidate the pending bug fixes (#144)"

4ac704f

This reverts commit 35174f3, reversing changes made to 57ce9e9.

Navigation Menu

Uh oh!

Consolidate the pending bug fixes: 8 open PRs plus the fz-branch findings - #144

Consolidate the pending bug fixes: 8 open PRs plus the fz-branch findings#144
ayzk merged 6 commits into
masterfrom
master-fixes

ayzk commented Sep 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ayzk commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The open pull requests, reviewed line by line

The two parts of #132 not taken

Gaps found while reviewing, fixed here

From the fz branch, verified to reproduce here

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ayzk commented Sep 1, 2026 •

edited

Loading

From the `fz` branch, verified to reproduce here