From 6c131e738355fc377bbc5b6a0f9560906698da09 Mon Sep 17 00:00:00 2001 From: Sriniketh24 Date: Sat, 23 May 2026 08:08:30 +0530 Subject: [PATCH 1/3] GH-36388: [C++][Python] Return error from MakeArrayFromScalar on offset overflow MakeArrayFromScalar silently created an invalid array with negative offsets when the total data size (value_size * repetition_count) exceeded the maximum value of the offset type. For 32-bit offset types like StringType and BinaryType, this threshold is INT32_MAX (~2 GB). The root cause was in CreateOffsetsBuffer where the running offset accumulated via OffsetType addition without checking for overflow, wrapping around to negative values. Added an early overflow check in CreateOffsetsBuffer that computes the total size in int64_t and compares against the offset type's maximum. On overflow, a Status::Invalid error is returned with a message suggesting the use of large_* types. This is AI-assisted work by Claude. --- cpp/src/arrow/array/array_test.cc | 27 +++++++++++++++++++++++++++ cpp/src/arrow/array/util.cc | 13 +++++++++++++ python/pyarrow/tests/test_array.py | 10 ++++++++++ 3 files changed, 50 insertions(+) diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index 64ea3fd71a73..fecbf79aa03e 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -767,6 +767,33 @@ TEST_F(TestArray, TestMakeArrayFromScalar) { } } +TEST_F(TestArray, TestMakeArrayFromScalarOffsetOverflow) { + // Regression test for GH-36388: MakeArrayFromScalar should return an error + // when the total data size would overflow 32-bit offsets instead of silently + // producing an invalid array with negative offsets. + + // A single-byte string repeated 2^31 times overflows int32 offsets + auto scalar = MakeScalar("x"); + int64_t length = static_cast(1) << 31; + ASSERT_RAISES(Invalid, MakeArrayFromScalar(*scalar, length)); + + // A two-byte string repeated just over INT32_MAX/2 times also overflows + auto scalar2 = MakeScalar("xy"); + int64_t length2 = (static_cast(1) << 30) + 1; + ASSERT_RAISES(Invalid, MakeArrayFromScalar(*scalar2, length2)); + + // Binary type has the same issue + auto bin_scalar = std::make_shared(Buffer::FromString("abc")); + int64_t length3 = (static_cast(std::numeric_limits::max()) / 3) + 1; + ASSERT_RAISES(Invalid, MakeArrayFromScalar(*bin_scalar, length3)); + + // Large string type should NOT overflow (uses 64-bit offsets) + auto large_scalar = std::make_shared("x"); + // Just verify it doesn't raise for a small count (we can't allocate 2^31 bytes here) + ASSERT_OK_AND_ASSIGN(auto arr, MakeArrayFromScalar(*large_scalar, 16)); + ASSERT_EQ(arr->length(), 16); +} + TEST_F(TestArray, TestMakeArrayFromScalarSliced) { // Regression test for ARROW-13437 auto scalars = GetScalars(); diff --git a/cpp/src/arrow/array/util.cc b/cpp/src/arrow/array/util.cc index 1c19bd5a5468..91b5819a750f 100644 --- a/cpp/src/arrow/array/util.cc +++ b/cpp/src/arrow/array/util.cc @@ -853,6 +853,19 @@ class RepeatedArrayFactory { template Status CreateOffsetsBuffer(OffsetType value_length, std::shared_ptr* out) { + // Check that the total data size does not overflow the offset type. + // For 32-bit offset types (e.g. StringType, BinaryType), value_length * length_ + // must fit in int32_t, otherwise the offsets wrap around and produce an invalid + // array with negative offsets. + if (value_length > 0 && length_ > 0) { + int64_t total_size = static_cast(value_length) * length_; + if (total_size > static_cast(std::numeric_limits::max())) { + return Status::Invalid( + "Cannot create array: total data size (", total_size, + " bytes) would overflow the offset type. Consider using a large_* " + "type (e.g. large_string, large_binary) for data exceeding 2 GB."); + } + } TypedBufferBuilder builder(pool_); RETURN_NOT_OK(builder.Resize(length_ + 1)); OffsetType offset = 0; diff --git a/python/pyarrow/tests/test_array.py b/python/pyarrow/tests/test_array.py index a103519dc5ac..ba1c486e0c67 100644 --- a/python/pyarrow/tests/test_array.py +++ b/python/pyarrow/tests/test_array.py @@ -442,6 +442,16 @@ def test_array_from_dictionary_scalar(): assert result.equals(expected) +def test_repeat_offset_overflow(): + # GH-36388: pa.repeat should raise an error when the total data size + # would overflow 32-bit offsets, instead of returning an invalid array. + with pytest.raises(pa.ArrowInvalid, match="overflow"): + pa.repeat("x", 2**31) + + with pytest.raises(pa.ArrowInvalid, match="overflow"): + pa.repeat("xy", 2**30 + 1) + + def test_array_getitem(): arr = pa.array(range(10, 15)) lst = arr.to_pylist() From c7c6026cf11cc85dff6f055d7dd9507276144c1f Mon Sep 17 00:00:00 2001 From: Sriniketh24 Date: Mon, 10 Aug 2026 12:42:00 -0400 Subject: [PATCH 2/3] GH-36388: rework overflow check using MultiplyWithOverflow, add length checks Per @AlenkaF's suggestion to build on the approach from #38504: - Replace the manual int64_t multiplication (which could itself silently overflow for 64-bit offset types like large_string/large_binary) with arrow::internal::MultiplyWithOverflow, which is correct for both 32-bit and 64-bit OffsetType. - Reject length > numeric_limits::max() up front, independent of value size (e.g. an empty string repeated too many times). - Reject negative length in MakeArrayFromScalar itself. - Add C++ test cases for the length-exceeds-offset-type and negative-length paths, on top of the existing overflow tests and the Python-level pa.repeat() regression test. Credit to @llama90's work in #38504, which reviewer @js8544 had already validated this approach on before it went stale. --- cpp/src/arrow/array/array_test.cc | 10 ++++++++++ cpp/src/arrow/array/util.cc | 32 +++++++++++++++++++------------ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index fecbf79aa03e..79465ef0f41d 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -792,6 +792,16 @@ TEST_F(TestArray, TestMakeArrayFromScalarOffsetOverflow) { // Just verify it doesn't raise for a small count (we can't allocate 2^31 bytes here) ASSERT_OK_AND_ASSIGN(auto arr, MakeArrayFromScalar(*large_scalar, 16)); ASSERT_EQ(arr->length(), 16); + + // A length that itself exceeds the offset type's range must be rejected too, + // even independent of the value size (e.g. an empty string repeated too many + // times to index with int32 offsets). + auto empty_scalar = std::make_shared(""); + int64_t length4 = static_cast(std::numeric_limits::max()) + 1; + ASSERT_RAISES(Invalid, MakeArrayFromScalar(*empty_scalar, length4)); + + // A negative length must be rejected outright. + ASSERT_RAISES(Invalid, MakeArrayFromScalar(*scalar, -1)); } TEST_F(TestArray, TestMakeArrayFromScalarSliced) { diff --git a/cpp/src/arrow/array/util.cc b/cpp/src/arrow/array/util.cc index 91b5819a750f..57f037ea3964 100644 --- a/cpp/src/arrow/array/util.cc +++ b/cpp/src/arrow/array/util.cc @@ -43,6 +43,7 @@ #include "arrow/util/checked_cast.h" #include "arrow/util/decimal.h" #include "arrow/util/endian.h" +#include "arrow/util/int_util_overflow.h" #include "arrow/util/logging_internal.h" #include "arrow/util/sort_internal.h" #include "arrow/visit_data_inline.h" @@ -51,6 +52,7 @@ namespace arrow { using internal::checked_cast; +using internal::MultiplyWithOverflow; // ---------------------------------------------------------------------- // Loading from ArrayData @@ -853,18 +855,21 @@ class RepeatedArrayFactory { template Status CreateOffsetsBuffer(OffsetType value_length, std::shared_ptr* out) { - // Check that the total data size does not overflow the offset type. - // For 32-bit offset types (e.g. StringType, BinaryType), value_length * length_ - // must fit in int32_t, otherwise the offsets wrap around and produce an invalid - // array with negative offsets. - if (value_length > 0 && length_ > 0) { - int64_t total_size = static_cast(value_length) * length_; - if (total_size > static_cast(std::numeric_limits::max())) { - return Status::Invalid( - "Cannot create array: total data size (", total_size, - " bytes) would overflow the offset type. Consider using a large_* " - "type (e.g. large_string, large_binary) for data exceeding 2 GB."); - } + // `length_` is the repeat count and is always representable in OffsetType here: + // MakeArrayFromScalar rejects negative lengths up front, and a length that itself + // exceeds the offset type's range can never produce a valid offsets buffer. + if (length_ > static_cast(std::numeric_limits::max())) { + return Status::Invalid("length exceeds the maximum value of offset_type: ", + length_, " is greater than ", + std::numeric_limits::max()); + } + // Guard against the total data size (value_length * length_) overflowing the + // offset type, which would otherwise silently wrap around and produce an + // invalid array with negative/garbage offsets. + OffsetType total_size; + if (MultiplyWithOverflow(value_length, static_cast(length_), + &total_size)) { + return Status::Invalid("offset overflow in repeated array construction"); } TypedBufferBuilder builder(pool_); RETURN_NOT_OK(builder.Resize(length_ + 1)); @@ -918,6 +923,9 @@ Result> MakeArrayOfNull(const std::shared_ptr& Result> MakeArrayFromScalar(const Scalar& scalar, int64_t length, MemoryPool* pool) { + if (length < 0) { + return Status::Invalid("length cannot be negative: ", length); + } // Null union scalars still have a type code associated if (!scalar.is_valid && !is_union(scalar.type->id())) { return MakeArrayOfNull(scalar.type, length, pool); From 9b76e0a6f7e317fcd02409a375b453ff7804eef3 Mon Sep 17 00:00:00 2001 From: Sriniketh24 Date: Thu, 3 Sep 2026 15:41:58 -0400 Subject: [PATCH 3/3] GH-36388: fix false-positive in offset overflow check Per @bkietz's review on #38504 (discussion_r1394400239): checking length_ alone against OffsetType::max() is wrong, since it rejects valid cases like an empty string repeated more than OffsetType::max() times (total data size stays 0, so it can never actually overflow). Only the product value_length * length_ needs to fit in OffsetType. Compute that product in int64_t and compare against OffsetType::max() directly, exactly as bkietz suggested. Moved the two cases that need multi-GB allocations to succeed (as opposed to fail-fast) into a separate LARGE_MEMORY_TEST-gated test, matching the convention used elsewhere in this file (see table_test.cc). --- cpp/src/arrow/array/array_test.cc | 30 +++++++++++++++++++++++------- cpp/src/arrow/array/util.cc | 25 +++++++++++-------------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index 79465ef0f41d..f551b0b81538 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -793,17 +793,33 @@ TEST_F(TestArray, TestMakeArrayFromScalarOffsetOverflow) { ASSERT_OK_AND_ASSIGN(auto arr, MakeArrayFromScalar(*large_scalar, 16)); ASSERT_EQ(arr->length(), 16); - // A length that itself exceeds the offset type's range must be rejected too, - // even independent of the value size (e.g. an empty string repeated too many - // times to index with int32 offsets). - auto empty_scalar = std::make_shared(""); - int64_t length4 = static_cast(std::numeric_limits::max()) + 1; - ASSERT_RAISES(Invalid, MakeArrayFromScalar(*empty_scalar, length4)); - // A negative length must be rejected outright. ASSERT_RAISES(Invalid, MakeArrayFromScalar(*scalar, -1)); } +// These cases require multi-GB allocations to actually exercise the success path at +// the int32 offset boundary (as opposed to the rejection path above, which fails +// before any large buffer is fully populated), so they're gated like other +// large-memory tests in this codebase (see gtest_util.h). +TEST_F(TestArray, LARGE_MEMORY_TEST(MakeArrayFromScalarOffsetBoundary)) { + // Only the total data size (value size * length) needs to fit in OffsetType, not + // length on its own: an empty string can be repeated far more than + // int32_t::max() times, since every offset stays 0. This guards against the + // false-positive case flagged in https://github.com/apache/arrow/pull/38504 + // (a length-only check would incorrectly reject this). + auto empty_scalar = std::make_shared(""); + int64_t empty_length = static_cast(std::numeric_limits::max()) + 1; + ASSERT_OK_AND_ASSIGN(auto empty_arr, MakeArrayFromScalar(*empty_scalar, empty_length)); + ASSERT_EQ(empty_arr->length(), empty_length); + + // Boundary case that should just work: "aa" repeated int32::max/2 times fits + // exactly within int32 offsets (avoid false positives at the edge). + auto scalar3 = MakeScalar("aa"); + int64_t length5 = static_cast(std::numeric_limits::max()) / 2; + ASSERT_OK_AND_ASSIGN(auto arr3, MakeArrayFromScalar(*scalar3, length5)); + ASSERT_EQ(arr3->length(), length5); +} + TEST_F(TestArray, TestMakeArrayFromScalarSliced) { // Regression test for ARROW-13437 auto scalars = GetScalars(); diff --git a/cpp/src/arrow/array/util.cc b/cpp/src/arrow/array/util.cc index 57f037ea3964..cfb0c2db03a0 100644 --- a/cpp/src/arrow/array/util.cc +++ b/cpp/src/arrow/array/util.cc @@ -855,21 +855,18 @@ class RepeatedArrayFactory { template Status CreateOffsetsBuffer(OffsetType value_length, std::shared_ptr* out) { - // `length_` is the repeat count and is always representable in OffsetType here: - // MakeArrayFromScalar rejects negative lengths up front, and a length that itself - // exceeds the offset type's range can never produce a valid offsets buffer. - if (length_ > static_cast(std::numeric_limits::max())) { + // The only thing that must fit in OffsetType is the total data size + // (value_length * length_), not length_ or value_length individually — e.g. an + // empty string repeated far more than OffsetType::max() times is perfectly valid, + // since every offset stays 0. Compute the product in int64_t (wide enough for a + // 32-bit OffsetType) and compare against OffsetType::max() directly, per + // https://github.com/apache/arrow/pull/38504#discussion_r1394400239. + int64_t total_size; + if (MultiplyWithOverflow(static_cast(value_length), length_, &total_size) || + total_size > static_cast(std::numeric_limits::max())) { return Status::Invalid("length exceeds the maximum value of offset_type: ", - length_, " is greater than ", - std::numeric_limits::max()); - } - // Guard against the total data size (value_length * length_) overflowing the - // offset type, which would otherwise silently wrap around and produce an - // invalid array with negative/garbage offsets. - OffsetType total_size; - if (MultiplyWithOverflow(value_length, static_cast(length_), - &total_size)) { - return Status::Invalid("offset overflow in repeated array construction"); + std::to_string(total_size), " is greater than ", + std::to_string(std::numeric_limits::max())); } TypedBufferBuilder builder(pool_); RETURN_NOT_OK(builder.Resize(length_ + 1));