fix(reader): harden RunEnd/Constant/Zoned/Pco against malformed input - #330
Merged
Conversation
Part of TODO.md's "Per-encoding adversarial tests" security item (CLAUDE.md §Security contract): a malformed file must always throw VortexException, never a raw JDK exception. - RunEndEncodingDecoder: reject negative num_runs and zero runs paired with a non-empty row count — previously decoded "successfully" into a lazy array backed by an empty ends/values child, then threw a raw IndexOutOfBoundsException/ArithmeticException on first read. - ConstantEncodingDecoder: reject a Decimal-typed constant whose scalar oneof doesn't carry bytes_value (raw NullPointerException reading its length). - ScanIterator.decodeZoneTable: bound the zone-map table's declared row count (an unvalidated layout field, deliberately decoupled from the data layout's chunk count) before it sizes an ArrayList — a negative value threw a raw IllegalArgumentException and a value just over Integer.MAX_VALUE wrapped negative on the int cast. - PcoTansDecoder.build: size the degenerate (zero-bin) decode table to tableSize instead of a fixed 1-state table — a page's initial ANS state indices are read with ansSizeLog bits regardless of bin count, so a corrupt file pairing zero bins with a nonzero ansSizeLog indexed a stale 1-entry array out of bounds. - PcoEncodingDecoder: reject a bin offsetBits > 64 (wider than any latent); validate that declared per-page value counts are non-negative and sum to the expected valid row count before allocating/writing latent buffers sized or offset by them. TODO.md: mark RunEnd, Constant, Zoned, and Pco done — all eleven per-encoding gotchas now closed.
Independent subagent review of the prior commit (12d7466) surfaced two gaps: - RunEndEncodingDecoder only rejected the degenerate zero-run case; the spec (encoding-format/dict-runend-sparse.md §RunEnd) is explicit that a conformant reader SHOULD itself validate what the reference writer guarantees but the reference reader does not enforce: `ends` strictly increasing, `ends[0] >= offset` when sliced, and `ends[numRuns-1] >= offset + n`. None of these crashed (the binary search stays in-bounds regardless of content), but they silently resolved rows against the wrong run instead of failing. Added one O(numRuns) validation pass; the coverage check is `>=` not `==` since a sliced window's trailing run legitimately extends past it (see the new trailingRunPastWindow_decodesNormally test, the spec's own worked example). - ScanIterator.decodeZoneTable's nZones bound (added in 12d7466) still let a value up to Integer.MAX_VALUE through to `new ArrayList<>((int) nZones)` — a single allocation at that scale is itself an OutOfMemoryError vector. Switched to an unsized ArrayList that grows with what the loop actually produces.
4 tasks
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
1 participant
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.
Summary
VortexExceptionon crafted/malformed input, never a raw JDK exception.num_runsagainst a non-empty row count, and validatesendsper the format spec (strictly increasing,ends[0] >= offset,ends[numRuns-1] >= offset + n).bytes_valueinstead of NPE-ing.ScanIterator.decodeZoneTable): bounds the zone-map table's declared row count before it sizes a collection, and avoids a single huge pre-sized allocation.offsetBits > 64, and validates declared per-page value counts against the actual row count before any latent-buffer writes.Test plan
./mvnw -pl fsst,core,reader -am test./mvnw -pl writer -am test./mvnw -pl integration -am verify(includes real Rust-written files with Pco/RunEnd columns)🤖 Generated with Claude Code