pearu opened a new pull request, #50841:
URL: https://github.com/apache/arrow/pull/50841
### Rationale for this change
`compute::Take` on `string`/`binary` arrays silently overflows the int32
offsets buffer when the selected data exceeds `INT32_MAX` bytes, returning
`Status::OK()` with a corrupt array. Downstream this produces garbage values
and segfaults — most visibly in pyarrow, where `np.asarray()` on a dictionary
array whose dense form exceeds 2 GiB crashes the interpreter.
A guard for this already exists, but a misplaced closing parenthesis makes
it dead code on GCC and clang:
```cpp
ARROW_PREDICT_FALSE(static_cast<int64_t>(offset) +
static_cast<int64_t>(val_size)) > kOffsetLimit
```
expands to `(__builtin_expect(!!(offset + val_size), 0)) > kOffsetLimit`.
The `!!` collapses the sum to 0 or 1, which is never greater than
`kOffsetLimit` (2147483646), so the branch is never taken. MSVC and the
fallback definitions expand `ARROW_PREDICT_FALSE(x)` to `(x)`, so those builds
were unaffected.
Present since `c07486c29f` (ARROW-5760, 2020-06-11). See #50840 for full
analysis.
### What changes are included in this PR?
- Move the closing parenthesis so the comparison happens inside
`ARROW_PREDICT_FALSE`, in `VarBinarySelectionImpl::GenerateOutput`.
- Add `TestTakeKernel.TakeBinaryOffsetOverflow`, a `LARGE_MEMORY_TEST`
covering the overflow.
Deliberately minimal: it does not attempt to make the oversized
dictionary-decode case *succeed*. A 32-bit `string` cannot represent >2 GiB, so
`Take` refusing is the correct behaviour; making the pyarrow conversion work is
a separate enhancement.
### Are these changes tested?
Yes, and the test was verified to distinguish both states:
| check | result |
|---|---|
| new test **with** fix | PASS (1.08 s, ~2 GiB peak) |
| new test **without** fix | FAIL — `Expected: has substring "...overflowed
binary array capacity" / Actual: "OK"` |
| `arrow-compute-vector-selection-test`, `ARROW_LARGE_MEMORY_TESTS=ON` |
169/169 pass |
| clang-format 18.1.8 | clean |
The test uses 2048 × 1 MiB = 2 GiB, one value past the limit — ~2 GiB peak
and ~1 s, rather than the multi-GB/multi-minute shape of the original
reproducer.
Separately, I confirmed the end-to-end path on `main` @ `42694575d0`: before
the fix `Cast(dictionary<int16,string> -> string)` on a 2.5 GB decode returns
OK with 7,050,328 negative offsets and a final offset of −1794967296 (=
2500000000 − 2³²); after the fix it returns `Invalid: Take operation overflowed
binary array capacity`.
Note that `LARGE_MEMORY_TEST` compiles to `DISABLED_*` unless
`ARROW_LARGE_MEMORY_TESTS=ON`, which in CI only happens in the "AMD64 Ubuntu
Large Memory Tests" job of `cpp_extra.yml` — nightly, or on PRs labelled `CI:
Extra: C++`. I don't have permission to add that label; a committer may want
to, so the new test is exercised before merge.
### Are there any user-facing changes?
Yes. `Take` (and anything built on it, including dictionary decoding and
`DictionaryArray` → numpy/pandas conversion) now raises `Invalid: Take
operation overflowed binary array capacity` where it previously returned
corrupt data or crashed. Code that unknowingly relied on the corrupt result
will now see an error — which is the intent.
**This PR contains a "Critical Fix".** It fixes both a bug that caused
incorrect or invalid data to be produced — silently corrupt offset buffers,
returned as a valid array with `Status::OK()` — and a bug that causes a crash
even when the API contract is upheld, since those offsets lead to out-of-bounds
reads and segfaults on ordinary `Take` usage.
### AI usage
Per the [AI-generated code
guidance](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code):
the diagnosis, the one-line fix, and the test were produced with Claude Code,
and reviewed and verified by me. Correctness was checked by (1) compiling the
macro expansion standalone to confirm the guard never fires as written, (2)
running the new test against both the fixed and unfixed kernel to confirm it
distinguishes them, and (3) reproducing the corrupt offsets and the post-fix
clean error end-to-end through `compute::Cast`.
---
_🤖 Drafted by Claude Code (an AI agent) and reviewed & approved by pearu._
--
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]
