You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "wiedld (via GitHub)" <gi...@apache.org> on 2024/12/17 00:37:10 UTC
[PR] Handle possible overflows in StringArrayBuilder / LargeStringArrayBuilder [datafusion]
wiedld opened a new pull request, #13802:
URL: https://github.com/apache/datafusion/pull/13802
## Which issue does this PR close?
Closes #13759
## Rationale for this change
We identified a potential code vulnerability during a security audit at InfluxData.
This fixes that vulnerability, by provide instead the maximum valid capacity number accepted by MutableBuffer.
## What changes are included in this PR?
Handle:
* usize overflow
* up to max accepted [bit_util::round_upto_multiple_of_64](https://docs.rs/arrow-buffer/53.3.0/src/arrow_buffer/buffer/mutable.rs.html#79) (or it just panics there)
* up to max accepted by [std::alloc::Layout](https://docs.rs/arrow-buffer/53.3.0/src/arrow_buffer/buffer/mutable.rs.html#80) (or it just panics there)
## Are these changes tested?
yes
## Are there any user-facing changes?
no
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Handle possible overflows in StringArrayBuilder / LargeStringArrayBuilder [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #13802:
URL: https://github.com/apache/datafusion/pull/13802#discussion_r1888727574
##########
datafusion/functions/src/strings.rs:
##########
@@ -124,8 +125,18 @@ pub struct StringArrayBuilder {
impl StringArrayBuilder {
pub fn with_capacity(item_capacity: usize, data_capacity: usize) -> Self {
- let mut offsets_buffer =
- MutableBuffer::with_capacity((item_capacity + 1) * size_of::<i32>());
+ // MutableBuffer::with_capacity will round up to multiples of `ALIGNMENT`
+ let capacity = std::cmp::max(
Review Comment:
I overthought things. Fixed.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Handle possible overflows in StringArrayBuilder / LargeStringArrayBuilder [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #13802:
URL: https://github.com/apache/datafusion/pull/13802#discussion_r1888729596
##########
datafusion/functions/src/strings.rs:
##########
@@ -124,8 +125,18 @@ pub struct StringArrayBuilder {
impl StringArrayBuilder {
pub fn with_capacity(item_capacity: usize, data_capacity: usize) -> Self {
- let mut offsets_buffer =
- MutableBuffer::with_capacity((item_capacity + 1) * size_of::<i32>());
+ // MutableBuffer::with_capacity will round up to multiples of `ALIGNMENT`
+ let capacity = std::cmp::max(
+ item_capacity.saturating_add(1).saturating_mul(ALIGNMENT),
+ data_capacity,
+ );
+ // rust core's Layout::from_size_align is capped at isize::Max
+ let capacity = std::cmp::min(
Review Comment:
I suppose it doesn't matter where it panics, as long as we get the proper panic. I fixed based on the [approach you mentioned here](https://github.com/apache/datafusion/pull/13802#discussion_r1888360133).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Handle possible overflows in StringArrayBuilder / LargeStringArrayBuilder [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13802:
URL: https://github.com/apache/datafusion/pull/13802
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Handle possible overflows in StringArrayBuilder / LargeStringArrayBuilder [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13802:
URL: https://github.com/apache/datafusion/pull/13802#discussion_r1888360133
##########
datafusion/functions/src/strings.rs:
##########
@@ -124,8 +125,18 @@ pub struct StringArrayBuilder {
impl StringArrayBuilder {
pub fn with_capacity(item_capacity: usize, data_capacity: usize) -> Self {
- let mut offsets_buffer =
- MutableBuffer::with_capacity((item_capacity + 1) * size_of::<i32>());
+ // MutableBuffer::with_capacity will round up to multiples of `ALIGNMENT`
+ let capacity = std::cmp::max(
Review Comment:
The idea of the `with_capacity` is to allocate enough space for `item_capacity + 1` offsets, each of which is an `i32`
The code below then assumes that it has successfully allocated space for that many offsets and uses the `unsafe` API assuming that the allocation is sized for `item_capacity + 1` items
In the case where the `capacity` calculation overflows, that assumption is violated.
Thus, I think the right fix is for the code in DataFusion to check that the value it passes to `mutable` is in fact sufficient to hold `item_capacity + 1` `i32` values
I think you can do this by using https://doc.rust-lang.org/std/primitive.i32.html#method.checked_mul instead of `*` and panic'ing if the calculation is bad
##########
datafusion/functions/src/strings.rs:
##########
@@ -124,8 +125,18 @@ pub struct StringArrayBuilder {
impl StringArrayBuilder {
pub fn with_capacity(item_capacity: usize, data_capacity: usize) -> Self {
- let mut offsets_buffer =
- MutableBuffer::with_capacity((item_capacity + 1) * size_of::<i32>());
+ // MutableBuffer::with_capacity will round up to multiples of `ALIGNMENT`
+ let capacity = std::cmp::max(
+ item_capacity.saturating_add(1).saturating_mul(ALIGNMENT),
+ data_capacity,
+ );
+ // rust core's Layout::from_size_align is capped at isize::Max
+ let capacity = std::cmp::min(
Review Comment:
I am not sure what case you are trying to prevent by checking `ALIGNMENT`, but it looks like maybe you have found a problem passing large (but not overflowing) values to `MutableBuffer::with_capacity`
If this is the case, I have two suggestions:
1. Such a check belongs in arrow-rs (`MutableBuffer::with_capacity`)
2. File a ticket explaining what the issue is in arrow-rs along with a reproducer before adding additional checks.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org