(arrow) branch main updated: GH-51156: [C++] Raise CapacityError instead of truncating binary values over 2 GiB (#51158)

[apitrou]

This is an automated email from the ASF dual-hosted git repository.

pitrou 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 f4ee045dc3 GH-51156: [C++] Raise CapacityError instead of truncating 
binary values over 2 GiB (#51158)
f4ee045dc3 is described below

commit f4ee045dc3a5dbb19695ec8055d046f2d79ebba8
Author: Nathan Goldbaum <nathan.goldb...@gmail.com>
AuthorDate: Mon Sep 7 03:08:53 2026 -0600

    GH-51156: [C++] Raise CapacityError instead of truncating binary values 
over 2 GiB (#51158)
    
    ### Rationale for this change
    
    `BaseBinaryBuilder::Append(std::string_view)`, 
`ExtendCurrent(std::string_view)` and 
`ChunkedBinaryBuilder::Append(std::string_view)` cast the value size to 
`int32_t` before validating it, so a value of 2 GiB or more is stored as an 
empty or truncated value. See #51156.
    
    ### What changes are included in this PR?
    
    The three overloads validate the 64-bit size before narrowing it, returning 
the same `CapacityError` that `ValidateOverflow` produces for the other paths.
    
    ### Are these changes tested?
    
    Yes, two `LARGE_MEMORY_TEST` cases in `array_binary_test.cc`. They fail 
without the fix.
    
    ### Are there any user-facing changes?
    
    `BinaryBuilder`, `StringBuilder` and `ChunkedBinaryBuilder` now return 
`CapacityError` for a `string_view` value of 2 GiB or more instead of storing 
wrong data.
    
    **This PR contains a "Critical Fix".** The builders produced incorrect data 
for such values.
    
    AI disclosure: I used an AI model to think about and iterate on the fix. It 
also originally identified the bug.
    
    * GitHub Issue: #51156
    
    Authored-by: Nathan Goldbaum <nathan.goldb...@gmail.com>
    Signed-off-by: Antoine Pitrou <anto...@python.org>
---
 cpp/src/arrow/array/array_binary_test.cc | 23 +++++++++++++++++++++++
 cpp/src/arrow/array/builder_binary.h     |  8 ++++++++
 2 files changed, 31 insertions(+)

diff --git a/cpp/src/arrow/array/array_binary_test.cc 
b/cpp/src/arrow/array/array_binary_test.cc
index 70245906f8..d0cc2c7709 100644
--- a/cpp/src/arrow/array/array_binary_test.cc
+++ b/cpp/src/arrow/array/array_binary_test.cc
@@ -946,6 +946,29 @@ TEST_F(TestChunkedBinaryBuilder, LargeElementCount) {
   }
 }
 
+TEST(TestBinaryBuilder, LARGE_MEMORY_TEST(AppendOverflow)) {
+  const std::string value(static_cast<size_t>(1) << 31, 'x');
+
+  BinaryBuilder builder;
+  ASSERT_RAISES(CapacityError, builder.Append(std::string_view(value)));
+  ASSERT_OK(builder.Append("x"));
+  ASSERT_RAISES(CapacityError, builder.ExtendCurrent(std::string_view(value)));
+  ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
+  ASSERT_OK(array->ValidateFull());
+  ASSERT_EQ(1, array->length());
+}
+
+TEST_F(TestChunkedBinaryBuilder, LARGE_MEMORY_TEST(AppendOverflow)) {
+  Init(100);
+  const std::string value(static_cast<size_t>(1) << 31, 'x');
+
+  ASSERT_RAISES(CapacityError, builder_->Append(std::string_view(value)));
+  ArrayVector chunks;
+  ASSERT_OK(builder_->Finish(&chunks));
+  ASSERT_EQ(1, chunks.size());
+  ASSERT_EQ(0, chunks[0]->length());
+}
+
 TEST(TestChunkedStringBuilder, BasicOperation) {
   const int chunksize = 100;
   internal::ChunkedStringBuilder builder(chunksize);
diff --git a/cpp/src/arrow/array/builder_binary.h 
b/cpp/src/arrow/array/builder_binary.h
index d0e761ae96..12f7e45f34 100644
--- a/cpp/src/arrow/array/builder_binary.h
+++ b/cpp/src/arrow/array/builder_binary.h
@@ -84,6 +84,7 @@ class BaseBinaryBuilder
   }
 
   Status Append(std::string_view value) {
+    ARROW_RETURN_NOT_OK(ValidateOverflow(static_cast<int64_t>(value.size())));
     return Append(value.data(), static_cast<offset_type>(value.size()));
   }
 
@@ -100,6 +101,7 @@ class BaseBinaryBuilder
   }
 
   Status ExtendCurrent(std::string_view value) {
+    ARROW_RETURN_NOT_OK(ValidateOverflow(static_cast<int64_t>(value.size())));
     return ExtendCurrent(reinterpret_cast<const uint8_t*>(value.data()),
                          static_cast<offset_type>(value.size()));
   }
@@ -949,6 +951,12 @@ class ARROW_EXPORT ChunkedBinaryBuilder {
   }
 
   Status Append(std::string_view value) {
+    if (ARROW_PREDICT_FALSE(value.size() >
+                            
static_cast<size_t>(BinaryBuilder::memory_limit()))) {
+      return Status::CapacityError("array cannot contain more than ",
+                                   BinaryBuilder::memory_limit(), " bytes, 
have ",
+                                   value.size());
+    }
     return Append(reinterpret_cast<const uint8_t*>(value.data()),
                   static_cast<int32_t>(value.size()));
   }