Fix UBSan in LinearQuantizer: guard float-to-int cast against NaN/Inf/overflow - #3
Closed
groeneai wants to merge 36 commits into
Closed
Fix UBSan in LinearQuantizer: guard float-to-int cast against NaN/Inf/overflow#3groeneai wants to merge 36 commits into
groeneai wants to merge 36 commits into
Conversation
Since this is exported, we better use a more specific name.
* Algorithmic updates * replace pointwise iterator by blockwise iterator * replace pointwise iterator by blockwise iterator * Merge test units (szcompressor#95) * update python support for Windows * update config to support save and load to ini * update error handling * update conf * Speeding up SZ3 Python API Compress and Decompress functions (szcompressor#99)
* bugfix for sampling --------- Co-authored-by: Jliu-1 <jliu217@central.uh.edu>
* brand new pysz package v1.0
* bugfix for hdf5 on windows
* 1. for big-endian system, force compressed format to be always little-endian 2. add helper to convert config to/from HDF5 cd_values * add documentation
* add bio modules and algorithms (ALGO_BioMD and ALGO_BioMDXTC) * new huffmanV2 with less storage overhead * remove warning during make * update integration test for h5 * update github CI --------- Co-authored-by: LangdaoZhang <zhanglangdao@outlook.com> Co-authored-by: Kai Zhao <kzhao@Kais-MacBook-Pro.local>
* Add initial version of SZ3 ParaView plugin * Update documentation for ParaView plugin * Fix some warnings
* fix bug in compressed format * update pysz to 1.0.3
…) data (ClickHouse) build_code shifted a 64-bit value by 64 when the Huffman tree has a single symbol (constant data gives a zero-length code), which is undefined behavior and aborts under UBSan.
Extract the header and config parsing out of SZ_decompress into a reusable SZ_load_config helper. This lets a caller inspect the compression configuration (e.g. the algorithm) before dispatching to an algorithm-specific decoder, so untrusted input can be rejected up front.
…House) The number of quantization indices is read from the compressed data and was trusted. Every decomposition reachable here consumes exactly conf.num indices while walking the data grid, so a smaller count caused an out-of-bounds read of quant_inds during decompression and a larger one drove an untrusted allocation. Read the count with the bounded overload and require it to equal conf.num. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
On decompression these walked vectors filled from the compressed data using running indices that were not bounded against the vector sizes, so crafted input could read past their ends: - LinearQuantizer::recover_unpred indexed unpred[index++] unbounded, and load resized unpred to an untrusted count before the bounded read could reject it; - RegressionPredictor consumed N+1 coefficients per block without checking the coefficient vector size; - ComposedPredictor read selection[current_index++] and indexed predictors[sid] with an unbounded index and an unchecked predictor id. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
unpad_tree followed child indices read from the compressed data without checking them against the node count, so a crafted tree could read the L/R/C/t arrays out of bounds or form a cycle. pad_tree always assigns a child a higher index than its parent, so enforce i < child < nodeCount. Also reject a stateNum too small to hold nodeCount nodes, otherwise new_node2 would write past the end of the node pool. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
…ickHouse) On the ALGO_LOSSLESS decompress path the dispatcher passes the pre-allocated output buffer (conf.num elements) to Lossless_zstd::decompress, but the decompressed size was read from the (untrusted) payload and used as the zstd destination capacity without checking it against the buffer. A crafted block could keep conf.num correct while declaring a larger lossless size, so zstd would write past the end of the buffer. Pass the buffer capacity into the lossless decoder and reject a payload that declares a larger decompressed size before zstd runs. Found while integrating SZ3 into ClickHouse and fuzzing the decompressor: ClickHouse/ClickHouse#108788
The generic lossy decompressor (`SZGenericCompressor::decompress`, used by `ALGO_INTERP` / `ALGO_LORENZO_REG` / `ALGO_INTERP_LORENZO`) reads the size of its internal buffer from the untrusted compressed payload and passed it to `Lossless_zstd::decompress` with `dst == nullptr` and no capacity, so a corrupted block could force an arbitrary `malloc(dstLen)` before any validation. Bound that allocation by the largest internal buffer the configuration could have produced: during compression the buffer is zstd-compressed and `ZSTD_compressBound(B) >= B`, so a stored generic-lossy block satisfies `B < SZ_compress_size_bound = 4096 + conf.size_est() + ZSTD_compressBound(conf.num * sizeof(T))`. `Lossless_zstd::decompress` now treats a non-zero incoming `dstLen` as an upper bound on the size it may allocate when `dst == nullptr`, and rejects a payload that declares more before allocating it. `conf.num` is validated against the trusted output size by the caller, so the bound can not be inflated by corrupted input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e (ClickHouse) Lossless_zstd::decompress validated that the declared decompressed size fits the output buffer, but never required ZSTD_decompress to actually produce that many bytes. A crafted ALGO_LOSSLESS block could declare a size matching the trusted output size while the zstd frame expands to fewer bytes, leaving the tail of the output buffer uninitialized; the caller then copies it out as if it were decompressed data. Require the produced size to equal the declared size. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788
…kHouse) Two memory-safety gaps remained in the generic lossy decompression path, both reachable from a crafted (untrusted) compressed payload: 1. Validate the inner interpolation dimensions. `ALGO_INTERP` stores its own dimensions array inside the compressed payload (`InterpolationDecomposition`), separately from the trusted `Config::dims`. A block could keep `config.num` equal to the trusted output size while declaring larger interpolation dimensions, so the decompressor would iterate past the end of the output buffer and the decoded quantization vector. `InterpolationDecomposition::decompress` now rejects a block whose stored dimensions do not match the trusted configuration before it uses them for anything. 2. Make the internal scratch buffer ownership exception-safe. The generic decompressor allocated the internal buffer with a raw `malloc` and freed it only on the success path, so any of the parsing steps that run on untrusted data (`decomposition.load`, `encoder.load`, the quantization-index count read, `encoder.decode`) leaked it on a corrupted block. `SZGenericCompressor::decompress` now owns the buffer with RAII, and `Lossless_zstd::decompress` frees a buffer it allocated itself when zstd decompression fails. Found while integrating SZ3 into ClickHouse: ClickHouse/ClickHouse#108788 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…read (ClickHouse) Two sanitizer findings from the SZ3 codec unit tests. `LorenzoPredictor` formed out-of-bounds pointers during compression. The neighbour helpers computed `d[-offset]` with an unsigned `offset`, which is `*(d + (size_t)(-offset))` and wraps the pointer below the buffer. The accessed element is in bounds (the predictor pads by 2), but the pointer computation is undefined behavior and was flagged by `-fsanitize=pointer-overflow`. Compute the address with pointer subtraction so the offset stays a small negative step. `SZGenericCompressor` let `HuffmanEncoder::decode` read past the end of the decompressed scratch buffer on a corrupted or truncated block. `load` records the bytes remaining right after the Huffman tree and uses that as the bound for the encoded stream, but the compressor wrote the `quant_inds` count between the tree and the stream, so the recorded bound was `sizeof(size_t)` bytes too large. Move that count before the encoder so the tree is immediately followed by its encoded stream, as the predictor-side encoders already do. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…verflow LinearQuantizer::quantize_and_overwrite cast fabs(diff) * error_bound_reciprocal directly to int64_t. When the input value is NaN, diff is NaN; for infinities or huge magnitudes the product exceeds the int64_t range. Converting such a value to an integer is undefined behaviour, reported by UBSan as '... is outside the range of representable values of type long'. Check the finite quantization range on the double before the cast. Values that cannot be represented as an index (NaN, +/-Inf, overflow) are stored losslessly in unpred, which is the same outcome the out-of-range branch already produced for finite values, so quantization of representable values is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
Author
|
cc @alexey-milovidov — one-line fix for the UBSan reported on ClickHouse master (Stress test, amd_asan_ubsan): the quantizer cast a NaN/Inf/overflowing float magnitude to int64_t. The finite range is now checked before the cast; non-representable values fall through to the existing lossless |
Member
|
Superseded: this exact fix is already on |
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
6 participants
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.
LinearQuantizer::quantize_and_overwritecastfabs(diff) * error_bound_reciprocaldirectly toint64_t. When the input value is NaN,diffis NaN; for infinities or huge magnitudes the product exceeds theint64_trange. Converting such a value to an integer is undefined behaviour.UBSan on ClickHouse master (Stress test, amd_asan_ubsan) reports:
This is reachable from a normal INSERT of a Float64/Float32 column with the SZ3 codec when the data contains NaN or Inf:
CompressionCodecSZ3::doCompressData->SZ_compress->quantize_and_overwrite.Fix: check the finite quantization range on the
doublebefore the cast. Values that cannot be represented as an index (NaN, +/-Inf, overflow) are stored losslessly inunpred, which is the same outcome the out-of-range branch already produced for finite values, so quantization of representable values is unchanged (verified equivalent to the previous code over ~1.1M random finite in-range inputs, 0 differences).