-
Notifications
You must be signed in to change notification settings - Fork 892
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
fix: prevent potential out-of-range access in FixedSizeListArray #5902
Conversation
Signed-off-by: BubbleCal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @BubbleCal -- this looks reasonable to me
It would be great to have some sort of test for this, but I can imagine that creating one would require several GB of memory so that might not be ideal
I wonder if there are any benchmarks we could run to ensure this doesn't regress performance 🤔
It appears this PR is failing on CI -- and likely needs needs clippy run |
Signed-off-by: BubbleCal <[email protected]>
sure i can add benchmark for this, and the results from my machine show no performance difference |
arrow-array/Cargo.toml
Outdated
@@ -35,10 +35,14 @@ bench = false | |||
|
|||
|
|||
[target.'cfg(target_arch = "wasm32")'.dependencies] | |||
ahash = { version = "0.8", default-features = false, features = ["compile-time-rng"] } | |||
ahash = { version = "0.8", default-features = false, features = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes appear to be whitespace only changes
Signed-off-by: BubbleCal <[email protected]>
Thanks again @BubbleCal |
Which issue does this PR close?
Closes #5901
Rationale for this change
fsl.value(i)
gets out of range ifi * value_length
is overi32::MAX
What changes are included in this PR?
changed
value_offset_at(i)
return type fromi32
tousize
,and make
value(i)
access the underlying values by offset fromvalue_offset_at(i)
so it still work ifi * value_length
is over i32 but withinusize
Are there any user-facing changes?
No