Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential unsoundness in filter_run_end_array #6569

Closed
cramertj opened this issue Oct 15, 2024 · 3 comments · Fixed by #6675
Closed

Potential unsoundness in filter_run_end_array #6569

cramertj opened this issue Oct 15, 2024 · 3 comments · Fixed by #6675
Assignees
Labels
arrow Changes to the arrow crate bug

Comments

@cramertj
Copy link

I believe this comment is incorrect:

// in filter_array the predicate array is checked to have the same len as the run end array
// this means the largest value in the run_ends is == to pred.len()
// so we're always within bounds when calling value_unchecked
for pred in (start..end).map(|i| unsafe { filter_values.value_unchecked(i as usize) }) {

It allows for re_arr.len() <= pred.len().
Instead, filter_array only errors in the case where values.len() is smaller, not where it is larger.

if predicate.filter.len() > values.len() {

Furtnermore, since this is a requirement placed on callers of filter_run_end_array, filter_run_end_array should be changed to unsafe fn and include a comment like the following:

/// # Safety
///
/// The caller must ensure that the `pred` and `re_arr` are the same length.
@cramertj cramertj added the bug label Oct 15, 2024
@cramertj cramertj changed the title Potential unsoundness in filter_run_array Potential unsoundness in filter_run_end_array Oct 15, 2024
@alamb
Copy link
Contributor

alamb commented Oct 16, 2024

Sounds like a good thing to fix. Thank you 🙏

@delamarch3
Copy link
Contributor

take

@alamb
Copy link
Contributor

alamb commented Nov 16, 2024

label_issue.py automatically added labels {'arrow'} from #6675

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants