Skip to content

Validate untrusted footer metadata on open instead of panicking#8867

Merged
connortsui20 merged 3 commits into
developfrom
ct/fix-slice-panic-segment
Jul 21, 2026
Merged

Validate untrusted footer metadata on open instead of panicking#8867
connortsui20 merged 3 commits into
developfrom
ct/fix-slice-panic-segment

Conversation

@connortsui20

@connortsui20 connortsui20 commented Jul 20, 2026

Copy link
Copy Markdown
Member

Closes: #8819

A fuzzed Vortex file whose footer segment map declared a segment past the end of the file panicked while slicing the backing buffer during open, instead of returning an error. This fixes that plus a few nearby unchecked-slice / unchecked-shift panics in the same file-open and array-decode paths:

  • Footer parsing validates every segment's offset + length against the file size (the Panic in SerializedArray::from_flatbuffer_and_segment (unchecked BufferHandle::slice) #8819 root cause).
  • populate_initial_segments bounds-checks the initial-read slice range before slicing.
  • SerializedArray::from_flatbuffer_and_segment uses checked offset arithmetic and bounds-checks a buffer against its segment before slicing.
  • Alignment exponents from footer segment specs, postscript segments, and array buffer descriptors go through a fallible Alignment::try_from_exponent instead of from_exponent, which panics on a too-large shift.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 12.27%

⚡ 1 improved benchmark
✅ 1795 untouched benchmarks
⏩ 46 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation fsl_large 99.6 µs 88.8 µs +12.27%

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 ct/fix-slice-panic-segment (e086d0d) with develop (5b688a9)

Open in CodSpeed

Footnotes

  1. 46 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.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Polar Signals Profiling Results

Latest Run

Status Commit Job Attempt Link
🟢 Done e086d0d 1 Explore Profiling Data
Previous Runs (2)
Status Commit Job Attempt Link
🟢 Done c4fee4f 1 Explore Profiling Data
🟢 Done 5e48c79 1 Explore Profiling Data

Powered by Polar Signals Cloud

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Benchmarks: Vortex queries 📖

Verdict: No clear signal (low confidence)
Attributed Vortex impact: -0.1%
Engines: DataFusion No clear signal (+8.6%, environment too noisy confidence) · DuckDB No clear signal (-8.1%, medium confidence)
Vortex (geomean): 0.898x ✅
Parquet (geomean): 0.898x ✅
Shifts: Parquet (control) -10.2% · Median polish -10.3%

How to read Verdict and Engines
  • Verdict: Overall PR-level signal after subtracting baseline drift estimated from Parquet control rows. It can be Likely improvement, Likely regression, or No clear signal.
  • Engines: Per-engine attribution. DataFusion is compared against DataFusion/Parquet controls; DuckDB is compared against DuckDB/Parquet controls. This answers whether each engine improved or regressed independently.
  • Confidence: Based on directional consistency, share of rows above the noise floor, and control-run noise.

datafusion / vortex-file-compressed (0.912x ➖, 0↑ 0↓)
name PR e086d0d (ns) base 5b688a9 (ns) ratio (PR/base)
vortex_q00/datafusion:vortex-file-compressed 9716988 10644295 0.91
vortex_q01/datafusion:vortex-file-compressed 6221966 6830634 0.91
datafusion / parquet (0.839x ✅, 2↑ 0↓)
name PR e086d0d (ns) base 5b688a9 (ns) ratio (PR/base)
vortex_q00/datafusion:parquet 🚀 19924331 22843824 0.87
vortex_q01/datafusion:parquet 🚀 4391354 5437392 0.81
duckdb / vortex-file-compressed (0.883x ✅, 1↑ 0↓)
name PR e086d0d (ns) base 5b688a9 (ns) ratio (PR/base)
vortex_q00/duckdb:vortex-file-compressed 9912002 10947785 0.91
vortex_q01/duckdb:vortex-file-compressed 🚀 5989210 6952563 0.86
duckdb / parquet (0.961x ➖, 0↑ 0↓)
name PR e086d0d (ns) base 5b688a9 (ns) ratio (PR/base)
vortex_q00/duckdb:parquet 23311745 24229204 0.96
vortex_q01/duckdb:parquet 9352875 9746132 0.96

No file size changes detected.

@connortsui20
connortsui20 marked this pull request as ready for review July 20, 2026 20:45
@connortsui20
connortsui20 enabled auto-merge (squash) July 20, 2026 20:45
@connortsui20 connortsui20 added the changelog/fix A bug fix label Jul 20, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e48c79c5a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vortex-file/src/footer/mod.rs Outdated
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Comment thread vortex-file/src/footer/deserializer.rs Outdated
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 merged commit 5746d55 into develop Jul 21, 2026
81 checks passed
@connortsui20
connortsui20 deleted the ct/fix-slice-panic-segment branch July 21, 2026 09:34
@connortsui20

Copy link
Copy Markdown
Member Author

I had no idea that codex could talk FOR me here without a prompt... I guess it was the correct fix though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic in SerializedArray::from_flatbuffer_and_segment (unchecked BufferHandle::slice)

2 participants