You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Jefffrey (via GitHub)" <gi...@apache.org> on 2024/02/21 11:40:05 UTC
[PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Jefffrey opened a new pull request, #5417:
URL: https://github.com/apache/arrow-rs/pull/5417
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #5412
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Ensure addition and multiplication saturate instead of overflowing (especially for release builds). This will ensure we panic instead of segfaulting.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "jhorstmann (via GitHub)" <gi...@apache.org>.
jhorstmann commented on PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#issuecomment-1959236632
Looks good, nice test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1497376341
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -67,10 +67,16 @@ impl MutableBuffer {
}
/// Allocate a new [MutableBuffer] with initial capacity to be at least `capacity`.
+ ///
+ /// # Panics
+ ///
+ /// If `capacity`, when rounded up to a multiple of 64, is larger than `isize::MAX`
+ /// then this function will panic. Avoid allocating capacities above this limit.
#[inline]
pub fn with_capacity(capacity: usize) -> Self {
let capacity = bit_util::round_upto_multiple_of_64(capacity);
- let layout = Layout::from_size_align(capacity, ALIGNMENT).unwrap();
+ let layout = Layout::from_size_align(capacity, ALIGNMENT)
+ .expect("failed to create layout for MutableBuffer");
Review Comment:
Panic should come here. Add some documentation around this, and switch to expect for a slightly better message
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -117,6 +117,8 @@ mod tests {
assert_eq!(64, round_upto_multiple_of_64(64));
assert_eq!(128, round_upto_multiple_of_64(65));
assert_eq!(192, round_upto_multiple_of_64(129));
+ // TODO: investigate why this is failing
+ // assert_eq!(usize::MAX, round_upto_multiple_of_64(usize::MAX));
Review Comment:
Found this edge case, don't think it's relevant to the original issue but might be interesting to check out (since looks like function will round down if `num` is large enough)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1498199517
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -117,6 +117,8 @@ mod tests {
assert_eq!(64, round_upto_multiple_of_64(64));
assert_eq!(128, round_upto_multiple_of_64(65));
assert_eq!(192, round_upto_multiple_of_64(129));
+ // TODO: investigate why this is failing
+ // assert_eq!(usize::MAX, round_upto_multiple_of_64(usize::MAX));
Review Comment:
On the other hand, without the saturating add it would've just overflowed so would get an ever lower number (in release mode at least)
I think the bit manipulation involved doesn't handle it well when input is maxed out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "jhorstmann (via GitHub)" <gi...@apache.org>.
jhorstmann commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1499086593
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -39,7 +39,12 @@ pub fn round_upto_multiple_of_64(num: usize) -> usize {
/// be a power of 2.
pub fn round_upto_power_of_2(num: usize, factor: usize) -> usize {
debug_assert!(factor > 0 && (factor & (factor - 1)) == 0);
- (num + (factor - 1)) & !(factor - 1)
+ let rounded = (num.saturating_add(factor - 1)) & !(factor - 1);
Review Comment:
I would maybe go with `checked_add` instead of saturation + assert:
```
(num.checked_add(factor - 1).expect("failed to round to next power of 2")) & !(factor - 1)
```
The formatting of the assert message bloats the assembly a little bit. This is probably not an issue, but might interact with inlining decisions via `MutableBuffer::push -> MutableBuffer::reserve -> round_upto_multiple_of_64`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1498344086
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -117,6 +117,8 @@ mod tests {
assert_eq!(64, round_upto_multiple_of_64(64));
assert_eq!(128, round_upto_multiple_of_64(65));
assert_eq!(192, round_upto_multiple_of_64(129));
+ // TODO: investigate why this is failing
+ // assert_eq!(usize::MAX, round_upto_multiple_of_64(usize::MAX));
Review Comment:
Perhaps we use checked add and return the maximum multiple of two in such a case? Or check the value is sufficiently small at the start?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1499908823
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -39,7 +39,12 @@ pub fn round_upto_multiple_of_64(num: usize) -> usize {
/// be a power of 2.
pub fn round_upto_power_of_2(num: usize, factor: usize) -> usize {
debug_assert!(factor > 0 && (factor & (factor - 1)) == 0);
- (num + (factor - 1)) & !(factor - 1)
+ let rounded = (num.saturating_add(factor - 1)) & !(factor - 1);
Review Comment:
Yeah this makes sense, thanks will try it out :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in when allocating buffers don't overflow [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1497927982
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -117,6 +117,8 @@ mod tests {
assert_eq!(64, round_upto_multiple_of_64(64));
assert_eq!(128, round_upto_multiple_of_64(65));
assert_eq!(192, round_upto_multiple_of_64(129));
+ // TODO: investigate why this is failing
+ // assert_eq!(usize::MAX, round_upto_multiple_of_64(usize::MAX));
Review Comment:
Is it possible this relates to the change to saturating add?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1498963299
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -117,6 +117,8 @@ mod tests {
assert_eq!(64, round_upto_multiple_of_64(64));
assert_eq!(128, round_upto_multiple_of_64(65));
assert_eq!(192, round_upto_multiple_of_64(129));
+ // TODO: investigate why this is failing
+ // assert_eq!(usize::MAX, round_upto_multiple_of_64(usize::MAX));
Review Comment:
> Or check the value is sufficiently small at the start?
Probably this, as `usize::MAX` would have no greater value that is also a multiple of two. I'll add this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#issuecomment-1959155127
I removed the panic doc as I figured that even if they supply the maximum capacity to not cause a panic, it would probably cause an OOM considering the amount of memory they'd probably be trying to allocate... not to mention the max amount would actually depend on the ALIGNMENT which varies by arch. Let me know if it would be better to have this doc in spite of that :thinking:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Ensure addition/multiplications in arrow-buffer saturate [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5417:
URL: https://github.com/apache/arrow-rs/pull/5417#discussion_r1498344086
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -117,6 +117,8 @@ mod tests {
assert_eq!(64, round_upto_multiple_of_64(64));
assert_eq!(128, round_upto_multiple_of_64(65));
assert_eq!(192, round_upto_multiple_of_64(129));
+ // TODO: investigate why this is failing
+ // assert_eq!(usize::MAX, round_upto_multiple_of_64(usize::MAX));
Review Comment:
Perhaps we use checked add and return the maximum multiple of two in such a case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org