diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index 64ea3fd71a73..f551b0b81538 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -767,6 +767,59 @@ 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); + + // 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 1c19bd5a5468..cfb0c2db03a0 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,6 +855,19 @@ class RepeatedArrayFactory { template Status CreateOffsetsBuffer(OffsetType value_length, std::shared_ptr* out) { + // 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: ", + 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)); OffsetType offset = 0; @@ -905,6 +920,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); 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()