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

fix: prevent potential out-of-range access in FixedSizeListArray #5902

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

BubbleCal
Copy link
Contributor

Which issue does this PR close?

Closes #5901

Rationale for this change

fsl.value(i) gets out of range if i * value_length is over i32::MAX

What changes are included in this PR?

changed value_offset_at(i) return type from i32 to usize,
and make value(i) access the underlying values by offset from value_offset_at(i) so it still work if i * value_length is over i32 but within usize

Are there any user-facing changes?

No

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 17, 2024
Copy link
Contributor

@alamb alamb left a 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 🤔

@alamb
Copy link
Contributor

alamb commented Jun 17, 2024

It appears this PR is failing on CI -- and likely needs needs clippy run

Signed-off-by: BubbleCal <[email protected]>
@BubbleCal
Copy link
Contributor Author

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 🤔

sure i can add benchmark for this, and the results from my machine show no performance difference

@@ -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 = [
Copy link
Contributor

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]>
@alamb alamb merged commit c7e88a2 into apache:master Jun 17, 2024
26 checks passed
@alamb
Copy link
Contributor

alamb commented Jun 17, 2024

Thanks again @BubbleCal

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FixedSizeList got out of range when the total length of the underlying values over i32::MAX
2 participants