This is an automated email from the ASF dual-hosted git repository.
zanmato1984 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new a7d0bfa5ad7 GH-50840: [C++] Fix dead overflow guard in Take on
binary-like arrays (#50841)
a7d0bfa5ad7 is described below
commit a7d0bfa5ad71af8500f64a0e187ab9523e5da0d5
Author: Pearu Peterson <[email protected]>
AuthorDate: Mon Aug 10 23:28:27 2026 +0300
GH-50840: [C++] Fix dead overflow guard in Take on binary-like arrays
(#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 [...]
---
_🤖 Drafted by Claude Code (an AI agent) and reviewed & approved by pearu._
* GitHub Issue: #50840
Authored-by: Pearu Peterson <[email protected]>
Signed-off-by: Rossi Sun <[email protected]>
---
.../compute/kernels/vector_selection_internal.cc | 6 +++---
.../arrow/compute/kernels/vector_selection_test.cc | 23 ++++++++++++++++++++++
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/cpp/src/arrow/compute/kernels/vector_selection_internal.cc
b/cpp/src/arrow/compute/kernels/vector_selection_internal.cc
index 7fe8d9b8866..ab58d935054 100644
--- a/cpp/src/arrow/compute/kernels/vector_selection_internal.cc
+++ b/cpp/src/arrow/compute/kernels/vector_selection_internal.cc
@@ -517,9 +517,9 @@ struct VarBinarySelectionImpl : public
Selection<VarBinarySelectionImpl<Type>, T
// Use static property to prune this code from the filter path in
// optimized builds
- if (Adapter::is_take &&
- ARROW_PREDICT_FALSE(static_cast<int64_t>(offset) +
- static_cast<int64_t>(val_size)) >
kOffsetLimit) {
+ if (Adapter::is_take &&
ARROW_PREDICT_FALSE(static_cast<int64_t>(offset) +
+
static_cast<int64_t>(val_size) >
+ kOffsetLimit)) {
return Status::Invalid("Take operation overflowed binary array
capacity");
}
offset += val_size;
diff --git a/cpp/src/arrow/compute/kernels/vector_selection_test.cc
b/cpp/src/arrow/compute/kernels/vector_selection_test.cc
index 5fa2d6824dc..c7972098539 100644
--- a/cpp/src/arrow/compute/kernels/vector_selection_test.cc
+++ b/cpp/src/arrow/compute/kernels/vector_selection_test.cc
@@ -1706,6 +1706,29 @@ TEST_F(TestTakeKernelFSB, TakeFixedSizeBinary) {
TakeCAC(type, {kABNullDE, kABC}, "[4,
10]").Value(&chunked_arr));
}
+// GH-50840: taking more data than a 32-bit offset can address must raise
+// instead of silently overflowing the offsets buffer.
+TEST_F(TestTakeKernel, LARGE_MEMORY_TEST(TakeBinaryOffsetOverflow)) {
+ // 2048 * 1 MiB = 2 GiB of output, one value past the int32 offset limit.
+ constexpr int64_t kValueSize = 1 << 20;
+ constexpr int64_t kNumIndices = 2048;
+
+ StringBuilder values_builder;
+ ASSERT_OK(values_builder.Append(std::string(kValueSize, 'x')));
+ ASSERT_OK_AND_ASSIGN(auto values, values_builder.Finish());
+
+ Int32Builder indices_builder;
+ ASSERT_OK(indices_builder.Reserve(kNumIndices));
+ for (int64_t i = 0; i < kNumIndices; ++i) {
+ indices_builder.UnsafeAppend(0);
+ }
+ ASSERT_OK_AND_ASSIGN(auto indices, indices_builder.Finish());
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, ::testing::HasSubstr("Take operation overflowed binary array
capacity"),
+ TakeAAA(*values, *indices));
+}
+
using ListAndListViewArrowTypes =
::testing::Types<ListType, LargeListType, ListViewType, LargeListViewType>;
