GH-50840: [C++] Fix dead overflow guard in Take on binary-like arrays

[pearu]

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:

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: 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.

• GitHub Issue: [C++] Take on binary/string arrays silently overflows int32 offsets, producing corrupt arrays and segfaults #50840

[github-actions]

⚠️ GitHub issue #50840 has been automatically assigned in GitHub to PR creator.

[github-actions]

⚠️ GitHub issue #50840 has no components, please add labels for components.

[uros-b]

Nice minimal fix for a critical bug, thank you @pearu!

[zanmato1984]

+1

Thanks for locating and fix this tricky issue!

[zanmato1984]

Merging.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit a7d0bfa.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.