fix(zstd): return decode error instead of panicking on corrupt view lengths#8843
fix(zstd): return decode error instead of panicking on corrupt view lengths#8843mvanhorn wants to merge 1 commit into
Conversation
Merging this PR will improve performance by 11.17%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | true_count_vortex_buffer[128] |
580.6 ns | 522.2 ns | +11.17% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mvanhorn:fix/zstd-decode-oob-panic (eb172f4) with develop (c5e075a)
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
robert3005
left a comment
robert3005
left a comment
There was a problem hiding this comment.
I think we are going to hold on making changes here until we have a complete solution for all invalid data paths since similar error to this was reported in #8821
…engths
reconstruct_views read a u32 view-length prefix from ZSTD-decompressed
(untrusted) data and used it as a slice bound without validation. A corrupt
or fuzzed length produced an out-of-bounds slice panic ("range end index
... out of range for slice of length ...") in reconstruct_views.
Make reconstruct_views return VortexResult: bounds-check both the length
prefix read and the value slice (with a checked_add to avoid usize
overflow), returning a decode error instead of panicking. Both zstd
callers and the vortex-cuda caller propagate the error.
Closes vortex-data#8822
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
eb172f4 to
f6c6f58
Compare
|
Makes sense, happy to hold until the invalid-data paths have a complete solution. Point me at the direction that comes out of #8821 and I'll adapt this or close it. DCO is fixed in the meantime. |
## Rationale for this change - Closes: #8807 Three benchmarks stayed flaky after #8742, flipping between the same two values on PRs that can't affect them. Same root causes and fixes as #8742: | Benchmark | Seen flaky on | Why | Fix | | --- | --- | --- | --- | | `true_count_vortex_buffer[128]` | ±11.17% on 9 unrelated PRs (#8805, #8811, #8812, #8820, #8843, #8803, …) | a 128-bit popcount measures harness overhead and code layout, not the count | drop the 128 size | | runend `compress[(100000, 4)]` | ±11.9% on #8805, #8750, #8856 | allocates in the timed region; glibc malloc differs across runner images | mimalloc as global allocator | | `cast_decimal` `copy_*[65536]` | identical flags on #8838 and #8724 | same glibc-malloc cause (512 KB alloc per iteration) | mimalloc as global allocator | Left alone: `compact_sliced[(4096, 90)]` (single sighting) and the CUDA walltime benches (hosted-runner walltime noise, a runner config issue). The allocator swap shifts every benchmark in the two touched binaries once — see the comment below. Needs a one-time CodSpeed acknowledgment, like #8742. ## What changes are included in this PR? One commit per benchmark; bench files only. Ran `cargo check` + `clippy` on the three bench targets, smoke-ran the binaries, `cargo +nightly fmt`. --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
reconstruct_viewsdecodes a[u32-LE length][value bytes]stream from ZSTD-decompressed (untrusted) data and used the length prefix as a slice bound without validation, so a corrupt or fuzzed length triggered an out-of-bounds slice panic (range end index ... out of range for slice of length ...). This makesreconstruct_viewsreturnVortexResultand bounds-checks the untrusted reads, returning a decode error instead of panicking.Rationale for this change
This is a fuzzer-reported crash (#8822): decoding attacker-controllable ZSTD array data could panic a worker thread instead of surfacing a recoverable decode error. Decoding untrusted input should never panic; it should return an error the caller can handle. The panic reproduces on
developwith a length prefix that exceeds the remaining buffer.What changes are included in this PR?
encodings/zstd/src/array.rs:reconstruct_viewsnow returnsVortexResult<(Vec<ByteBuffer>, Buffer<BinaryView>)>. Both untrusted reads (the view-length prefix and the value slice) are bounds-checked (withchecked_addto avoidusizeoverflow) and return a decode error instead of panicking. The two in-crate callers propagate the error with?.vortex-cuda/src/kernel/encodings/zstd.rs: the CUDA decode caller propagates the new error with?.Errrather than panicking. The existingreconstruct_viewstests are unchanged in behavior.What APIs are changed? Are there any user-facing changes?
The
pub fn reconstruct_viewsinvortex-zstdchanges its return type from(Vec<ByteBuffer>, Buffer<BinaryView>)toVortexResult<(Vec<ByteBuffer>, Buffer<BinaryView>)>. Decoding a corrupt ZSTD array now yields aVortexErrorinstead of panicking. There is no behavior change for well-formed input.AI was used for assistance.