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
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>
|
This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days |
|
This PR was closed because it has been inactive for 7 days since being marked as stale. |
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.