fix(arrow-string): reject FixedSizeBinary concat widths that overflow i32 - #10981
Conversation
… i32 concat_elements_fixed_size_binary summed the two input widths into a usize and then cast that sum to i32 when sizing the builder. Two arrays of width 0x70000000 give an output width of 3758096384, which wraps to -536870912, and FixedSizeBinaryBuilder asserts the length is not negative. The width can come from a user controlled schema, so the process aborted instead of returning an error. ConcatByteViewBuilder in the same file already guards this by checking data_size against i32::MAX before building, so do the same here and return InvalidArgumentError. Widths that already worked are unaffected.
| "Concatenated FixedSizeBinary value length {output_size} exceeds {}", | ||
| i32::MAX |
There was a problem hiding this comment.
| "Concatenated FixedSizeBinary value length {output_size} exceeds {}", | |
| i32::MAX | |
| "Concatenated FixedSizeBinary value length exceeds i32", |
those big numbers arent really readable so not sure how useful they would be in the error
There was a problem hiding this comment.
yeah fair, 2147483647 reads as noise.
heads up though, the test asserts the whole string so the suggestion alone reds ci.
i'll fix test_fixed_size_binary_concat_width_overflow in the same commit either way.
only bit i'd keep is {output_size}, that one's the caller's own width not a constant,
so it says which pair blew up when it comes through concat_elements_dyn. so
"value length {output_size} exceeds i32". happy to take yours as written though, say
which and i'll push.
There was a problem hiding this comment.
pushed in 7a3b559. kept {output_size} and dropped the constant. if you prefer your
version as written, let me know and i will push that instead.
The limit was interpolated from i32::MAX, which renders as a bare 2147483647 and does not read as a type bound. Name the type instead and keep the offending combined width, which is the caller's own value and identifies which pair of arrays tripped it.
|
thanks for this @Cintu07 |
Which issue does this PR close?
concat_elements_fixed_size_binarypanics / over-allocates on untrustedFixedSizeBinarywidths #10972.Rationale for this change
concat_elements_fixed_size_binaryadds the two input widths together as a usize, then caststhe sum to i32 to size the builder. two arrays of width 0x70000000 come out at 3758096384,
which wraps to -536870912, and the builder asserts the value length is not negative. that
width is accepted at array construction and can come from a user controlled schema, so nothing
unusual on the caller's side is needed to reach it
it comes through
concat_elements_dynas well, since that dispatches here for fixed sizebinary inputs
What changes are included in this PR?
use i32::try_from on the combined width and return an invalid argument error when it does not
fit, rather than casting
the byte view builder in this same file already guards exactly this, checking data_size
against i32::MAX before it builds, so this is that guard applied to the fixed size binary path
instead of a new mechanism. widths that already fit behave the same as before
i went through the rest of the file for the same shape while i was in there. that cast was the
only unchecked one, so this is a single site rather than a family the way #10437 and #10575
were
one thing i left out on purpose. the next line still reserves the combined width through
MutableBuffer::with_capacity, so a sum just under i32::MAX asks for roughly 2 GB before asingle row is written. that looked like #10973 rather than this one, but say the word and i
will fold it in
Are these changes tested?
yes.
test_fixed_size_binary_concat_width_overflowuses the widths from the issue and checksthe call comes back as an error instead of panicking. with only the test applied to current
main it fails inside
fixed_size_binary_builder.rsat line 64, which is the panic site in thereport. fmt and clippy with -D warnings are both clean on arrow-string
Are there any user-facing changes?
concatenating two fixed size binary arrays whose widths sum past i32::MAX returns an error now
instead of panicking. no API changes