fix(collection,query): harden take/drop count-overflow family - #424
Open
belowzeroff wants to merge 2 commits into
Open
fix(collection,query): harden take/drop count-overflow family#424belowzeroff wants to merge 2 commits into
belowzeroff wants to merge 2 commits into
Conversation
…ll, f32 Follow-up to the INT64_MIN take/drop guard (RayforceDB#409), addressing sibling gaps found in review: - Range-take `end = start + amount` was a raw signed add in the string / vector / dict / list branches, so (take [1 2 3] [1 9223372036854775807]) overflowed to a negative count and drove a memcpy with a garbage size (crash / corruption on release; UBSan trap + oom on debug). Compute the clamped end without ever forming an overflowing start+amount. - Unify the null-count policy: INT64_MIN is the i64 null sentinel (0N), and (take xs 0N) erroring while (drop xs 0N) silently emptied was an asymmetric footgun. Both now reject it as a `type` error, matching the window verbs. - Reject an f32 count up front alongside f64: as_i64 on an f32 atom bit-reads the storage, so an f32 whose bits alias INT64_MIN would trip the guard with a misleading message. Drop the now-redundant ray_is_atom check as well (is_numeric already implies an atom type code).
`select ... take: -9223372036854775808` fed the INT64_MIN count through `-atom_n` / `-n_take` (signed-overflow UB) in the no-sort, apply_sort_take, and DAG tail-pushdown paths, cascading into a `nrows - n` overflow in the TAIL kernel. These paths build the range themselves, so the ray_take_fn guard never saw them. Reject the null-sentinel count with a `type` error at each site before the negation, matching the scalar take/drop policy.
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
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.
Follow-up to the review on #419 (whose INT64_MIN guard already landed via #409). That review found the guard stopped short of its own premise — sibling count paths kept the same signed-overflow UB, plus a pre-existing range-take overflow and a take/drop null-policy asymmetry. This PR closes all four, verified under UBSan against
dev.1.
select take:INT64_MIN negation UB (query.c)select … take: -9223372036854775808fed the count through-atom_n/-n_takein the no-sort,apply_sort_take, and DAG tail-pushdown paths — signed-overflow UB (query.c:635/870/9941) that also cascaded intonrows - nin the TAIL kernel (exec.c:3022). These paths build the range themselves, so theray_take_fnguard never saw them. Now each site rejects the null-sentinel count with atypeerror before the negation; the upstream guards also stop theexec.ccascade.2. Range-take
end = start + amountoverflow (collection.c) — memory-unsafe(take [1 2 3] [1 9223372036854775807])overflowedendin the string / vector / dict / list branches → negative count →memcpywith a garbage size (crash / corruption on release; UBSan trap +oomon debug). The clamped end is now computed without ever forming an overflowingstart + amount(min(amount, len - start)); the dict branch keeps itsend < startguard since it doesn't early-return onstart >= len.3. take/drop null-sentinel asymmetry — unified
INT64_MIN is the i64 null sentinel (
0N).(take xs 0N)erroring while(drop xs 0N)silently emptied was a footgun. Both verbs now reject it as atypeerror, matchingts_window_arg. (Updates the rfl test that pinned drop→empty.)4. Guard cleanup (collection.c)
as_i64on an-RAY_F32atom bit-reads the storage, so an f32 count whose bits alias INT64_MIN would trip the guard with a misleading message — f32 is now rejected up front alongside f64. Dropped the redundantray_is_atomcheck (is_numericalready implies an atom type code).Testing
make test— full suite green (3701/3702 pass, 1 pre-existing skip, 0 failed), ASan/UBSan clean.collection/take.rflandcollection/drop_cut_rotate_cross.rfl; the threeselect take:paths (pushdown / sorted / grouped) plus valid negative takes intable/select.rfl.Closes the review items from #419; #419 itself can be closed as superseded (its guard is already in
devvia #409).