Skip to content

fix(collection,query): harden take/drop count-overflow family - #424

Open
belowzeroff wants to merge 2 commits into
RayforceDB:devfrom
belowzeroff:fix/take-drop-count-overflow-family
Open

fix(collection,query): harden take/drop count-overflow family#424
belowzeroff wants to merge 2 commits into
RayforceDB:devfrom
belowzeroff:fix/take-drop-count-overflow-family

Conversation

@belowzeroff

Copy link
Copy Markdown
Contributor

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: -9223372036854775808 fed the count through -atom_n / -n_take in the no-sort, apply_sort_take, and DAG tail-pushdown paths — signed-overflow UB (query.c:635/870/9941) that also cascaded into nrows - n in the TAIL kernel (exec.c:3022). These paths build the range themselves, so the ray_take_fn guard never saw them. Now each site rejects the null-sentinel count with a type error before the negation; the upstream guards also stop the exec.c cascade.

2. Range-take end = start + amount overflow (collection.c) — memory-unsafe

(take [1 2 3] [1 9223372036854775807]) overflowed end in the string / vector / dict / list branches → negative count → memcpy with a garbage size (crash / corruption on release; UBSan trap + oom on debug). The clamped end is now computed without ever forming an overflowing start + amount (min(amount, len - start)); the dict branch keeps its end < start guard since it doesn't early-return on start >= 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 a type error, matching ts_window_arg. (Updates the rfl test that pinned drop→empty.)

4. Guard cleanup (collection.c)

as_i64 on an -RAY_F32 atom 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 redundant ray_is_atom check (is_numeric already implies an atom type code).

Testing

  • make test — full suite green (3701/3702 pass, 1 pre-existing skip, 0 failed), ASan/UBSan clean.
  • Coverage added: range-take overflow clamps and the unified type-error policy in collection/take.rfl and collection/drop_cut_rotate_cross.rfl; the three select take: paths (pushdown / sorted / grouped) plus valid negative takes in table/select.rfl.

Closes the review items from #419; #419 itself can be closed as superseded (its guard is already in dev via #409).

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant