From 42d986239b9f08591070ea351f7c03d36f0ffabc Mon Sep 17 00:00:00 2001 From: Tony Roberts Date: Mon, 10 Aug 2026 16:45:14 +0100 Subject: [PATCH 1/3] GH-45086: [C++] Fix heap buffer overflow in FillNullForward/Backward on chunked boolean arrays FillNullForwardChunked and FillNullBackwardChunked sized each output chunk's data buffer as `type->byte_width() * chunk->length()`. For BooleanType, byte_width() returns 0 (bit_width() / 8, truncated by integer division), so the buffer was allocated with 0 bytes while the chunk's declared length was unchanged, and filling it wrote real bit data past the end of the allocation. Add DataType::bytes_required(num_elements), a virtual method alongside byte_width()/bit_width() that correctly rounds up for bit-packed types, and use it at both call sites instead of the byte_width()-based calculation. Add regression tests exercising fill-null-forward and fill-null-backward on a chunked boolean array with a chunk large enough to reproduce the crash. --- .../arrow/compute/kernels/vector_replace.cc | 4 +- .../compute/kernels/vector_replace_test.cc | 42 +++++++++++++++++++ cpp/src/arrow/type.h | 17 ++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_replace.cc b/cpp/src/arrow/compute/kernels/vector_replace.cc index 6a9abfc03960..31c14580f1b5 100644 --- a/cpp/src/arrow/compute/kernels/vector_replace.cc +++ b/cpp/src/arrow/compute/kernels/vector_replace.cc @@ -699,7 +699,7 @@ struct FillNullForwardChunked { ARROW_ASSIGN_OR_RAISE(output->buffers[0], ctx->AllocateBitmap(chunk->length())); ARROW_ASSIGN_OR_RAISE( output->buffers[1], - ctx->Allocate(out->type()->byte_width() * chunk->length())); + ctx->Allocate(out->type()->bytes_required(chunk->length()))); } ExecResult chunk_result; chunk_result.value = out->array(); @@ -780,7 +780,7 @@ struct FillNullBackwardChunked { const auto& chunk = chunks[i]; if (is_fixed_width(out->type()->id())) { ArrayData* output = out->mutable_array(); - auto data_bytes = output->type->byte_width() * chunk->length(); + auto data_bytes = output->type->bytes_required(chunk->length()); ARROW_ASSIGN_OR_RAISE(output->buffers[0], ctx->AllocateBitmap(chunk->length())); ARROW_ASSIGN_OR_RAISE(output->buffers[1], ctx->Allocate(data_bytes)); } diff --git a/cpp/src/arrow/compute/kernels/vector_replace_test.cc b/cpp/src/arrow/compute/kernels/vector_replace_test.cc index 9dc8e70ab6a2..b169b63ea24d 100644 --- a/cpp/src/arrow/compute/kernels/vector_replace_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_replace_test.cc @@ -1117,6 +1117,13 @@ class TestFillNullType : public TestReplaceKernel { std::shared_ptr type() override { return default_type_instance(); } }; +class TestFillNullBoolean : public TestReplaceKernel { + protected: + std::shared_ptr type() override { + return TypeTraits::type_singleton(); + } +}; + TYPED_TEST_SUITE(TestFillNullNumeric, NumericBasedTypes); TYPED_TEST_SUITE(TestFillNullDecimal, DecimalArrowTypes); TYPED_TEST_SUITE(TestFillNullBinary, BaseBinaryArrowTypes); @@ -2092,6 +2099,41 @@ TYPED_TEST(TestFillNullBinary, FillBackwardChunkedArray) { R"(["qup"])", R"(["qup", "mnz"])"})); } +// Regression test for GH-45086: FillNullForwardChunked/FillNullBackwardChunked +// size each output chunk's data buffer as `type->byte_width() * chunk->length()`. +// For BooleanType, byte_width() returns 0 (it is bit-packed, not byte-addressable), +// so the buffer was allocated with 0 bytes while the chunk's declared length stayed +// the same, and filling the chunk wrote past the end of the (near-)empty buffer. +// The corruption/crash only reliably manifests once a chunk is large enough to +// write past the buffer's small built-in padding, hence the large pad length here. +TEST_F(TestFillNullBoolean, FillNullForwardChunkedArray) { + constexpr int64_t kPadLength = 4096; + ASSERT_OK_AND_ASSIGN(auto null_pad, MakeArrayOfNull(boolean(), kPadLength)); + auto all_true = ConstantArrayGenerator::Boolean(kPadLength, /*value=*/true); + auto single_true = ConstantArrayGenerator::Boolean(1, /*value=*/true); + + auto input = std::make_shared(ArrayVector{null_pad, single_true, null_pad}, + boolean()); + auto expected = std::make_shared( + ArrayVector{null_pad, single_true, all_true}, boolean()); + + this->AssertFillNullChunkedArray(FillNullForward, input, expected); +} + +TEST_F(TestFillNullBoolean, FillNullBackwardChunkedArray) { + constexpr int64_t kPadLength = 4096; + ASSERT_OK_AND_ASSIGN(auto null_pad, MakeArrayOfNull(boolean(), kPadLength)); + auto all_true = ConstantArrayGenerator::Boolean(kPadLength, /*value=*/true); + auto single_true = ConstantArrayGenerator::Boolean(1, /*value=*/true); + + auto input = std::make_shared(ArrayVector{null_pad, single_true, null_pad}, + boolean()); + auto expected = std::make_shared( + ArrayVector{all_true, single_true, null_pad}, boolean()); + + this->AssertFillNullChunkedArray(FillNullBackward, input, expected); +} + TEST_F(TestFillNullType, TestFillOnNullType) { this->AssertFillNullArray(FillNullForward, this->array(R"([null, null, null, null])"), this->array(R"([null, null, null, null])")); diff --git a/cpp/src/arrow/type.h b/cpp/src/arrow/type.h index 14a05c369e3b..42e54109682d 100644 --- a/cpp/src/arrow/type.h +++ b/cpp/src/arrow/type.h @@ -31,6 +31,7 @@ #include "arrow/result.h" #include "arrow/type_fwd.h" // IWYU pragma: export +#include "arrow/util/bit_util.h" #include "arrow/util/checked_cast.h" #include "arrow/util/endian.h" #include "arrow/util/macros.h" @@ -196,6 +197,22 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this, /// subclasses of FixedWidthType virtual int bit_width() const { return -1; } + /// \brief Returns the number of bytes needed to store `num_elements` + /// values of this fixed-width type, rounding up for bit-packed types + /// (e.g. boolean) that use less than one byte per value. Returns -1 + /// for non-fixed-width types, and should only be used for subclasses + /// of FixedWidthType + virtual int64_t bytes_required(int64_t num_elements) const { + const int64_t width = bit_width(); + if (width < 0) { + return -1; + } + if (width % 8 == 0) { + return (width / 8) * num_elements; + } + return bit_util::BytesForBits(width * num_elements); + } + // \brief EXPERIMENTAL: Enable retrieving shared_ptr from a const // context. std::shared_ptr GetSharedPtr() const { From 9c2e6b6750bf80006f47df0316ddcb31a7c0fb54 Mon Sep 17 00:00:00 2001 From: Tony Roberts Date: Tue, 11 Aug 2026 11:24:11 +0100 Subject: [PATCH 2/3] Address review feedback: use PreallocateFixedWidthArrayData Avoid adding a new virtual method to DataType. Switch to the existing arrow::util::internal::PreallocateFixedWidthArrayData helper, already used by vector_selection_{take,filter}_internal.cc for this kind of chunk allocation and correctly handles boolean's bit-packed layout. Also fixes the same byte_width() * chunk->length() overflow in ReplaceMaskChunked, with a regression test added to match. --- .../arrow/compute/kernels/vector_replace.cc | 24 +++++++++---------- .../compute/kernels/vector_replace_test.cc | 18 ++++++++++++++ cpp/src/arrow/type.h | 17 ------------- 3 files changed, 29 insertions(+), 30 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_replace.cc b/cpp/src/arrow/compute/kernels/vector_replace.cc index 31c14580f1b5..313fbd53c8b0 100644 --- a/cpp/src/arrow/compute/kernels/vector_replace.cc +++ b/cpp/src/arrow/compute/kernels/vector_replace.cc @@ -22,6 +22,7 @@ #include "arrow/compute/kernels/util_internal.h" #include "arrow/compute/registry_internal.h" #include "arrow/util/bitmap_ops.h" +#include "arrow/util/fixed_width_internal.h" #include "arrow/util/logging_internal.h" namespace arrow { @@ -422,12 +423,10 @@ struct ReplaceMaskChunked { ExecResult chunk_result; if (is_fixed_width(out->type()->id())) { auto chunk_out = std::make_shared(chunk->type(), chunk->length()); - chunk_out->buffers.resize(2); - ARROW_ASSIGN_OR_RAISE(chunk_out->buffers[0], - ctx->AllocateBitmap(chunk->length())); - const int64_t slot_width = out->type()->byte_width(); - ARROW_ASSIGN_OR_RAISE(chunk_out->buffers[1], - ctx->Allocate(slot_width * chunk->length())); + ArrayData* chunk_out_arr = chunk_out.get(); + RETURN_NOT_OK(util::internal::PreallocateFixedWidthArrayData( + ctx, chunk->length(), /*source=*/*chunk->data(), + /*allocate_validity=*/true, chunk_out_arr)); chunk_result.value = chunk_out; } if (batch[1].is_scalar()) { @@ -696,10 +695,9 @@ struct FillNullForwardChunked { for (const std::shared_ptr& chunk : values.chunks()) { if (is_fixed_width(out->type()->id())) { ArrayData* output = out->mutable_array(); - ARROW_ASSIGN_OR_RAISE(output->buffers[0], ctx->AllocateBitmap(chunk->length())); - ARROW_ASSIGN_OR_RAISE( - output->buffers[1], - ctx->Allocate(out->type()->bytes_required(chunk->length()))); + RETURN_NOT_OK(util::internal::PreallocateFixedWidthArrayData( + ctx, chunk->length(), /*source=*/*chunk->data(), + /*allocate_validity=*/true, output)); } ExecResult chunk_result; chunk_result.value = out->array(); @@ -780,9 +778,9 @@ struct FillNullBackwardChunked { const auto& chunk = chunks[i]; if (is_fixed_width(out->type()->id())) { ArrayData* output = out->mutable_array(); - auto data_bytes = output->type->bytes_required(chunk->length()); - ARROW_ASSIGN_OR_RAISE(output->buffers[0], ctx->AllocateBitmap(chunk->length())); - ARROW_ASSIGN_OR_RAISE(output->buffers[1], ctx->Allocate(data_bytes)); + RETURN_NOT_OK(util::internal::PreallocateFixedWidthArrayData( + ctx, chunk->length(), /*source=*/*chunk->data(), + /*allocate_validity=*/true, output)); } ExecResult chunk_result; chunk_result.value = out->array(); diff --git a/cpp/src/arrow/compute/kernels/vector_replace_test.cc b/cpp/src/arrow/compute/kernels/vector_replace_test.cc index b169b63ea24d..e729bcff938f 100644 --- a/cpp/src/arrow/compute/kernels/vector_replace_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_replace_test.cc @@ -545,6 +545,24 @@ TEST_F(TestReplaceBoolean, ReplaceWithMask) { } } +// Regression test: ReplaceMaskChunked (the ChunkedArray path of replace_with_mask) +// sized each output chunk's data buffer via byte_width(), which is 0 for boolean +// (bit-packed), the same GH-45086 buffer-overflow pattern fixed elsewhere in this +// file. A chunk needs to be large enough to write past the buffer's small built-in +// padding to reliably reproduce the crash. +TEST_F(TestReplaceBoolean, ReplaceWithMaskChunkedArray) { + constexpr int64_t kChunkLength = 4096; + auto all_false = ConstantArrayGenerator::Boolean(kChunkLength, /*value=*/false); + auto all_true = ConstantArrayGenerator::Boolean(kChunkLength, /*value=*/true); + auto input = std::make_shared( + ArrayVector{all_false, all_false, all_false}, boolean()); + auto expected = std::make_shared( + ArrayVector{all_true, all_true, all_true}, boolean()); + + this->Assert(ReplaceWithMask, Datum(input), this->mask_scalar(true), + this->scalar("true"), Datum(expected)); +} + TEST_F(TestReplaceNull, ReplaceWithMask) { std::vector cases = { {this->array("[]"), this->mask_scalar(false), this->array("[]"), this->array("[]")}, diff --git a/cpp/src/arrow/type.h b/cpp/src/arrow/type.h index 42e54109682d..14a05c369e3b 100644 --- a/cpp/src/arrow/type.h +++ b/cpp/src/arrow/type.h @@ -31,7 +31,6 @@ #include "arrow/result.h" #include "arrow/type_fwd.h" // IWYU pragma: export -#include "arrow/util/bit_util.h" #include "arrow/util/checked_cast.h" #include "arrow/util/endian.h" #include "arrow/util/macros.h" @@ -197,22 +196,6 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this, /// subclasses of FixedWidthType virtual int bit_width() const { return -1; } - /// \brief Returns the number of bytes needed to store `num_elements` - /// values of this fixed-width type, rounding up for bit-packed types - /// (e.g. boolean) that use less than one byte per value. Returns -1 - /// for non-fixed-width types, and should only be used for subclasses - /// of FixedWidthType - virtual int64_t bytes_required(int64_t num_elements) const { - const int64_t width = bit_width(); - if (width < 0) { - return -1; - } - if (width % 8 == 0) { - return (width / 8) * num_elements; - } - return bit_util::BytesForBits(width * num_elements); - } - // \brief EXPERIMENTAL: Enable retrieving shared_ptr from a const // context. std::shared_ptr GetSharedPtr() const { From d5d2dc25b07c2aeee996ab4be0dd2511cc07432b Mon Sep 17 00:00:00 2001 From: Tony Roberts Date: Wed, 12 Aug 2026 10:14:30 +0100 Subject: [PATCH 3/3] MINOR: [C++] Fix clang-format lint failure in vector_replace_test.cc Co-Authored-By: Claude Sonnet 5 --- cpp/src/arrow/compute/kernels/vector_replace_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_replace_test.cc b/cpp/src/arrow/compute/kernels/vector_replace_test.cc index e729bcff938f..dc63bae39a54 100644 --- a/cpp/src/arrow/compute/kernels/vector_replace_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_replace_test.cc @@ -2130,8 +2130,8 @@ TEST_F(TestFillNullBoolean, FillNullForwardChunkedArray) { auto all_true = ConstantArrayGenerator::Boolean(kPadLength, /*value=*/true); auto single_true = ConstantArrayGenerator::Boolean(1, /*value=*/true); - auto input = std::make_shared(ArrayVector{null_pad, single_true, null_pad}, - boolean()); + auto input = std::make_shared( + ArrayVector{null_pad, single_true, null_pad}, boolean()); auto expected = std::make_shared( ArrayVector{null_pad, single_true, all_true}, boolean()); @@ -2144,8 +2144,8 @@ TEST_F(TestFillNullBoolean, FillNullBackwardChunkedArray) { auto all_true = ConstantArrayGenerator::Boolean(kPadLength, /*value=*/true); auto single_true = ConstantArrayGenerator::Boolean(1, /*value=*/true); - auto input = std::make_shared(ArrayVector{null_pad, single_true, null_pad}, - boolean()); + auto input = std::make_shared( + ArrayVector{null_pad, single_true, null_pad}, boolean()); auto expected = std::make_shared( ArrayVector{all_true, single_true, null_pad}, boolean());