Repository navigation
fix(layout): check list offsets before the list reader uses them - #68
Conversation
The list reader trusted decoded offsets: offsets that decrease or run past the elements built wrong lists without an error, or panicked. Check them where they are read, build list layouts through a fallible constructor that checks children the way deserialization does, and add the element count with an overflow check. Signed-off-by: Rasheed Khoshnaw <[email protected]>
|
Reviewed head I found no upstream PR carrying this validation change. vortex-data/vortex#11082 is under active review, but addresses the separate sliced-list writer fix already in QC develop, so I do not think we need to wait on it for this work. One correctness issue to address before merging: Decode the primitive offsets and check actual signed values for negativity before casting, then retain the existing monotonicity and bounds checks. Also tighten the existing negative-offset fixture: its expected message is A local, unpushed fix is prepared. Both new forged-statistics regressions (same-width and widening casts) fail on this PR head and pass with the fix. With the fix: 160 list-layout tests and 2 physical-plan tests pass, as do scoped all-target/all-feature Clippy with |
|
Good catch on cached stats letting negative offsets through the cast. Could you push your fix to this branch? It sounds complete with the two forged-stats regressions, and there's no point in me redoing it. I'll tighten the empty expected message on the negative-offset fixture if you haven't already. |
Decode offsets to primitive values and reject negative signed offsets before the u64 cast can trust persisted min/max statistics. Preserve the monotonicity and elements-bound checks. Cover forged exact statistics for same-width and widening casts, and require the malformed negative-offset fixture to report the negative-offset validation error. Signed-off-by: Christopher Wirt <[email protected]>
Summary
Closes #56. Part of #55.
The list reader trusted the offsets it decoded. When offsets decrease, go negative or point past the elements, most reads return with no error, and a sparse read of decreasing offsets panics in
vortex-mask. Readers now check offsets as they read them, so a malformed file fails the read with an error.Changes
ListReader::fetch_raw_offsetschecks the offsets it returns. They must be the expected count, castable tou64(negative values fail), non-decreasing, and the last one can't be past the elements child's row count. Every read path gets its offsets from here, so the range arithmetic,kept_elementsandListArray::new_uncheckedafter it can rely on that. Only the offsets a read uses get checked, so pruning doesn't change and nothing scans the whole column on open. Therow_range.end + 1there is now checked too.ListLayout::newbecomesListLayout::try_new. It checks the children the way deserialization does: elements match the list's element dtype, offsets are non-nullable integers with at least one row, and validity is non-nullable bool with one row per list. Before, it used saturating subtraction andvortex_expect.checked_add.Not in this PR: which integer types the frozen format accepts for offsets. That's a #54 decision. Readers still accept any integer type that casts to
u64.Testing
malformed_offsets_fail_the_readcovers decreasing, past-the-end and negative offsets on full, bounded, sparse and length-only reads. Without the check, 8 of the 12 cases return with no error, 2 fail later with unrelated errors (an out-of-bounds slice and a filter length mismatch), and 1 panics. Only one, a sparse read of negative offsets, already failed cleanly.try_new_rejects_mismatched_childrencovers the wrong elements dtype, float offsets, nullable offsets, no offsets and a validity row count that doesn't match.cargo test -p vortex-layout -p vortex-fileandcargo clippy -p vortex-layout -p vortex-file --all-targets.