Nayana-Naik73 opened a new pull request, #53915:
URL: https://github.com/apache/arrow/pull/53915
### Rationale for this change
`mask_utf8_utf8_utf8_utf8` sizes its arena output as `max(upper_length,
lower_length, num_length) * data_len` in `int32_t`. That bound is wrong in two
ways:
* When all three replacement strings are empty the size is 0, but characters
that are not letters or digits are still copied one byte per byte.
`mask('QwErTy:4)', '', '', '')` asks the arena for 0 bytes and then writes
`":)"`. `SimpleArena::Allocate(0)` does not advance its cursor, so the next
allocation in the batch gets the same pointer and overwrites this row's output,
and at the end of a chunk the write lands past the chunk. The existing
`TestMask` case with empty replacements already goes through this path; it only
passes because the arena chunk has spare room.
* The replacement arguments can be column values, so the product can
overflow `int32_t` (signed overflow, flagged by UBSan). For example 46341 *
46341 goes negative, and `SimpleArena::Allocate` with a negative size returns
the current pointer and moves the cursor backwards, so the masking loop then
writes far out of bounds.
### What changes are included in this PR?
Reserve at least one output byte per input byte, and compute the bound with
`MultiplyWithOverflow`, returning a "Would overflow maximum output size" error
like the other Gandiva string functions do. The 1-, 2- and 3-argument `mask`
overloads delegate to this function, so they are covered too.
### Are these changes tested?
Yes, `TestGdvFnStubs.TestMask` gets two extra cases: an empty-replacement
call followed by a second call that would reuse the same arena pointer, and an
oversized replacement that must return the overflow error. I don't have LLVM on
this machine, so I could not build the full Gandiva test target. I compiled
`mask_utf8_utf8_utf8_utf8` and its wrappers from the source file together with
`TestMask`, against the real `ExecutionContext`/`SimpleArena` and libarrow.
Without the fix the new assertion fails (the first result reads back as `"#$"`)
and the overflow case crashes. With the fix the test passes, including under
ASan/UBSan, and all existing `TestMask` expectations are unchanged.
clang-format 18.1.8 and cpplint are clean.
### Are there any user-facing changes?
`mask()` now returns an error instead of corrupting memory when the output
would exceed 2 GiB. Results for valid inputs are unchanged.
**This PR contains a "Critical Fix".** It fixes an out-of-bounds write in
the Gandiva arena, reachable with crafted input to `mask()`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
