Skip to content

Fix remaining_length accounting after Huffman decode in RegressionPredictor / ComposedPredictor - #137

Open
alexey-milovidov wants to merge 1 commit into
szcompressor:masterfrom
ClickHouse:ch-fix-lorenzo-reg-remaining-length
Open

alexey-milovidov wants to merge 1 commit into
szcompressor:masterfrom
ClickHouse:ch-fix-lorenzo-reg-remaining-length

Conversation

@alexey-milovidov

Copy link
Copy Markdown

Problem

RegressionPredictor::load threads a remaining_length byte counter through its sub-reads so later reads know how many bytes are left. After Huffman-decoding the regression coefficients it does:

encoder.load(c, remaining_length);
regression_coeff_quant_inds = encoder.decode(c, coeff_size);
encoder.postprocess_decode();
remaining_length -= coeff_size * sizeof(int);   // <-- wrong amount

decode advances the read pointer c by the compressed (Huffman) stream size, but remaining_length is decremented by coeff_size * sizeof(int) — the uncompressed index count. Those two are unrelated, so after this point remaining_length no longer reflects the bytes actually consumed (it typically overshoots the compressed stream and understates the remainder, and can even wrap past zero).

ComposedPredictor::load has the mirror defect: it Huffman-decodes a per-block predictor-selection stream but never decrements remaining_length for it at all.

Today the read helpers in MemoryUtil.hpp only guard the counter with assert, so a release build tolerates the wrong value and the bug is latent. But any reader that actually enforces the bound will then reject valid data. This surfaced while integrating SZ3 into ClickHouse, whose hardened build makes those reads throw instead of assert: a column compressed with ALGO_LORENZO_REG could be written but failed every subsequent read.

Fix

Decrement remaining_length by exactly the bytes decode consumed, measured as the pointer advance:

const uchar *decode_start = c;
regression_coeff_quant_inds = encoder.decode(c, coeff_size);
encoder.postprocess_decode();
remaining_length -= static_cast<size_t>(c - decode_start);

and apply the same accounting to the selection stream in ComposedPredictor::load. Behavior for valid data is unchanged; the counter now tracks the real byte cursor.

Context

Found while integrating SZ3 into ClickHouse. Companion fix on the ClickHouse fork: ClickHouse#2

…mposed predictors

RegressionPredictor::load decremented remaining_length by the uncompressed
index count (coeff_size * sizeof(int)) after Huffman decode(), but decode()
advances the read pointer only by the compressed stream size. The uncompressed
count overshoots the compressed stream, understating remaining_length for the
subsequent quantizer encoder.load(), whose bound check then spuriously rejects
valid data with "SZ3 Huffman: tree exceeds compressed buffer".

RegressionPredictor is only used by ALGO_LORENZO_REG, so a column compressed
with CODEC(SZ3('ALGO_LORENZO_REG', ...)) could be written on insert but fail
every subsequent read with CORRUPTED_DATA (effective data loss). Small element
counts are affected; large counts happen to leave enough slack to survive the
understated bound.

Account for exactly the bytes decode() consumed via pointer difference.
ComposedPredictor::load had the mirror defect (it never decremented
remaining_length for its selection stream); tightened the same way.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 1, 2026
Bug fixes only: no compressed-format change and no interface signature change.

Covers the reviewed content of #131, #133, #134, #135, #137, #138 and #139, plus the
findings ported from the fz branch. #132 is only partly covered -- see the PR body for
the four exclusions and the measurement behind each.

31 dataset x algorithm x error-bound combinations are byte-identical to master, and every
master-produced file still decompresses to the same bytes.
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour; so were two signed overflows on values taken from the stream,
    the doubled state count in HuffmanEncoder::load and the doubled index in
    LinearQuantizer::recover_pred
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

Not taken from the PRs as written: #132's internal-buffer bound (above), and #135's
XtcBasedEncoder bounds are reinstated in a form that does not reject valid streams.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ayzk added a commit that referenced this pull request Sep 15, 2026
Consolidates the eight open bug-fix pull requests (#131 #132 #133 #134 #135 #137 #138 #139)
with the findings from reviewing them, as one change. The compressed stream is untrusted, and
several modules read lengths and counts out of it and then used them to index, allocate or
loop without checking them against anything.

The format is unchanged. A stream written by this build is byte-identical to one written by
master, and either build reads the other's output; verified on interp/lorenzo_reg/nopred in
1D, 2D and 3D.

Encoder::decode now takes the remaining byte count
------------------------------------------------
Its signature carried the symbol count and nothing else, so an encoder could not tell how
many bytes it was allowed to touch. HuffmanEncoder read its payload length out of the stream
and walked that far; BypassEncoder memcpy'd sizeof(T) * targetLength; RunlengthEncoder read a
value and a count per run. None of them had anything to compare against.

decode() now takes `size_t &remaining_length` alongside targetLength and charges what it
consumed, the same shape load() already had. The two numbers are independent -- an entropy
coder's bitstream has no terminator, so the symbol count is what says stop, while the byte
count is what says how far it may read -- and both are available at every call site.

The symbol count stays where it is in the stream, so no bytes moved.

Other bounds
------------
  - HuffmanEncoderV2's tree loading, and XtcBasedEncoder, against corrupted input
  - ComposedPredictor's predictor selection index, and its value
  - RegressionPredictor's coefficient stream, which each block consumes N + 1 entries of
  - InterpolationDecomposition's stored dimensions, which drive a grid walk over buffers
    that conf sizes
  - the declared bin count, against the element count conf carries
  - the bins InterpolationDecomposition and TimeSeriesDecomposition walk, checked once before
    the walk rather than on each access
  - Config::load reading one byte past the config, and validating contents only where the
    config is a compressed-stream trailer
  - the HDF5 filter's compressed buffer, sized from SZ_compress_size_bound
  - ALGO_LOSSLESS's output buffer: the declared size went to ZSTD_decompress as the capacity of a
    buffer the caller owns, so a stream declaring more than conf.num elements wrote past it. The
    size check that followed ran after the write. Lossless_zstd now honours a caller's capacity

Also
----
  - Huffman's shift for single-symbol input, and the non-finite float cast in LinearQuantizer,
    were undefined behaviour; so were two signed overflows on values taken from the stream,
    the doubled state count in HuffmanEncoder::load and the doubled index in
    LinearQuantizer::recover_pred
  - scratch buffers are held as unique_ptr so an exception from the encoder or the lossless
    layer does not leak them
  - PR #132's bound on the internal decompression buffer is dropped: that buffer is sized
    from the bin count and type, which no bound derivable from conf alone covers

XtcBasedEncoder's magicInts lookups are clamped on both sides rather than rejected on one.
LASTIDX is the table's length, and the encoder walks to it whenever no entry reaches minDiff --
which is every input with fewer than two triplets, since minDiff is then still INT_MAX. Both
sides read one past the table there; rejecting it on decode alone broke ALGO_BIOMDXTC for inputs
under six elements. Its bit-packing buffer is also zeroed: it went into the compressed output
uninitialised, which is why the same input did not compress to the same bytes twice.

MDZ passed its buffer capacity to decompress() as the stream length, having discarded what
compress() returned. zstd rejected every frame, and the result was decoded from uninitialised
memory without anything noticing.

Not taken from the PRs as written: #132's internal-buffer bound (above).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants