Skip to content

GH-49884: [C++] Fix integer overflow in BufferBuilder reachable from JSON parsing #49883

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
metsw24-max wants to merge 2 commits into
base: main
Choose a base branch
from
+135 −15
Open

GH-49884: [C++] Fix integer overflow in BufferBuilder reachable from JSON parsing #49883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d1254c
Fix integer overflow in BufferBuilder reachable from JSON parsing
metsw24-max Apr 28, 2026
b5ea90c
updated
metsw24-max Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 112 additions & 15 deletions cpp/src/arrow/buffer_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "arrow/buffer.h"
#include "arrow/status.h"
#include "arrow/util/bit_util.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/bitmap_generate.h"
#include "arrow/util/bitmap_ops.h"
#include "arrow/util/macros.h"
Expand All @@ -35,6 +36,10 @@

namespace arrow {

static inline bool BufferBuilderCapacityTooLarge(int64_t capacity) {
return ARROW_PREDICT_FALSE(capacity > std::numeric_limits<int64_t>::max() - 63);
}

// ----------------------------------------------------------------------
// Buffer builder classes

Expand Down Expand Up @@ -74,6 +79,12 @@ class ARROW_EXPORT BufferBuilder {
/// shrinking the builder.
/// \return Status
Status Resize(const int64_t new_capacity, bool shrink_to_fit = true) {
if (ARROW_PREDICT_FALSE(new_capacity < 0)) {
return Status::Invalid("Resize: negative capacity");
}
if (ARROW_PREDICT_FALSE(BufferBuilderCapacityTooLarge(new_capacity))) {
return Status::CapacityError("Resize: capacity overflow");
}
Comment on lines +82 to +87
Copy link
Copy Markdown
Member

@pitrou pitrou May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put these checks in AllocateResizableBuffer and PoolBuffer::Resize instead? That would benefit more use cases.

if (buffer_ == NULLPTR) {
ARROW_ASSIGN_OR_RAISE(buffer_,
AllocateResizableBuffer(new_capacity, alignment_, pool_));
Expand All @@ -91,11 +102,25 @@ class ARROW_EXPORT BufferBuilder {
/// \param[in] additional_bytes number of additional bytes to make space for
/// \return Status
Status Reserve(const int64_t additional_bytes) {
auto min_capacity = size_ + additional_bytes;
if (ARROW_PREDICT_FALSE(additional_bytes < 0)) {
Copy link
Copy Markdown
Member

@pitrou pitrou May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of similar checks that we're adding in many methods. Can we find a way of factoring these out, and hopefully avoid forgetting any call sites where such checks must be done?

return Status::Invalid("Reserve: negative additional_bytes");
}
int64_t min_capacity;
if (ARROW_PREDICT_FALSE(
internal::AddWithOverflow(size_, additional_bytes, &min_capacity))) {
return Status::CapacityError("Reserve: capacity overflow");
}
if (ARROW_PREDICT_FALSE(BufferBuilderCapacityTooLarge(min_capacity))) {
return Status::CapacityError("Reserve: capacity overflow");
}
if (min_capacity <= capacity_) {
return Status::OK();
}
return Resize(GrowByFactor(capacity_, min_capacity), false);
int64_t requested_capacity = GrowByFactor(capacity_, min_capacity);
if (ARROW_PREDICT_FALSE(BufferBuilderCapacityTooLarge(requested_capacity))) {
return Status::CapacityError("Reserve: capacity overflow");
}
return Resize(requested_capacity, false);
}

/// \brief Return a capacity expanded by the desired growth factor
Expand All @@ -104,15 +129,27 @@ class ARROW_EXPORT BufferBuilder {
// (versus 1.5x) seems to have slightly better performance when using
// jemalloc, but significantly better performance when using the system
// allocator. See ARROW-6450 for further discussion
return std::max(new_capacity, current_capacity * 2);
int64_t doubled;
if (ARROW_PREDICT_FALSE(internal::AddWithOverflow(current_capacity, current_capacity,
&doubled))) {
return new_capacity;
}
return std::max(new_capacity, doubled);
}

/// \brief Append the given data to the buffer
///
/// The buffer is automatically expanded if necessary.
Status Append(const void* data, const int64_t length) {
if (ARROW_PREDICT_FALSE(size_ + length > capacity_)) {
ARROW_RETURN_NOT_OK(Resize(GrowByFactor(capacity_, size_ + length), false));
if (ARROW_PREDICT_FALSE(length < 0)) {
return Status::Invalid("Append: negative length");
}
int64_t new_size;
if (ARROW_PREDICT_FALSE(internal::AddWithOverflow(size_, length, &new_size))) {
return Status::CapacityError("Append: size overflow");
}
if (ARROW_PREDICT_FALSE(new_size > capacity_)) {
ARROW_RETURN_NOT_OK(Resize(GrowByFactor(capacity_, new_size), false));
}
UnsafeAppend(data, length);
return Status::OK();
Expand Down Expand Up @@ -186,6 +223,9 @@ class ARROW_EXPORT BufferBuilder {
/// mostly for memory allocation).
Result<std::shared_ptr<Buffer>> FinishWithLength(int64_t final_length,
bool shrink_to_fit = true) {
if (ARROW_PREDICT_FALSE(final_length < 0)) {
return Status::Invalid("FinishWithLength: negative final length");
}
size_ = final_length;
return Finish(shrink_to_fit);
}
Expand Down Expand Up @@ -250,12 +290,22 @@ class TypedBufferBuilder<
}

Status Append(const T* values, int64_t num_elements) {
return bytes_builder_.Append(reinterpret_cast<const uint8_t*>(values),
num_elements * sizeof(T));
if (ARROW_PREDICT_FALSE(num_elements < 0)) {
return Status::Invalid("Append: negative number of elements");
}
int64_t num_bytes;
if (ARROW_PREDICT_FALSE(internal::MultiplyWithOverflow(
num_elements, static_cast<int64_t>(sizeof(T)), &num_bytes))) {
return Status::CapacityError("Append: size overflow");
}
return bytes_builder_.Append(reinterpret_cast<const uint8_t*>(values), num_bytes);
}

Status Append(const int64_t num_copies, T value) {
ARROW_RETURN_NOT_OK(Reserve(num_copies + length()));
if (ARROW_PREDICT_FALSE(num_copies < 0)) {
return Status::Invalid("Append: negative number of copies");
}
ARROW_RETURN_NOT_OK(Reserve(num_copies));
UnsafeAppend(num_copies, value);
return Status::OK();
}
Expand Down Expand Up @@ -284,15 +334,45 @@ class TypedBufferBuilder<
}

Status Resize(const int64_t new_capacity, bool shrink_to_fit = true) {
return bytes_builder_.Resize(new_capacity * sizeof(T), shrink_to_fit);
if (ARROW_PREDICT_FALSE(new_capacity < 0)) {
return Status::Invalid("Resize: negative capacity");
}
int64_t num_bytes;
if (ARROW_PREDICT_FALSE(internal::MultiplyWithOverflow(
new_capacity, static_cast<int64_t>(sizeof(T)), &num_bytes))) {
return Status::CapacityError("Resize: capacity overflow");
}
if (ARROW_PREDICT_FALSE(BufferBuilderCapacityTooLarge(num_bytes))) {
return Status::CapacityError("Resize: capacity overflow");
}
return bytes_builder_.Resize(num_bytes, shrink_to_fit);
}

Status Reserve(const int64_t additional_elements) {
return bytes_builder_.Reserve(additional_elements * sizeof(T));
if (ARROW_PREDICT_FALSE(additional_elements < 0)) {
return Status::Invalid("Reserve: negative additional_elements");
}
int64_t num_bytes;
if (ARROW_PREDICT_FALSE(internal::MultiplyWithOverflow(
additional_elements, static_cast<int64_t>(sizeof(T)), &num_bytes))) {
return Status::CapacityError("Reserve: size overflow");
}
if (ARROW_PREDICT_FALSE(BufferBuilderCapacityTooLarge(num_bytes))) {
return Status::CapacityError("Reserve: size overflow");
}
return bytes_builder_.Reserve(num_bytes);
}

Status Advance(const int64_t length) {
return bytes_builder_.Advance(length * sizeof(T));
if (ARROW_PREDICT_FALSE(length < 0)) {
return Status::Invalid("Advance: negative length");
}
int64_t num_bytes;
if (ARROW_PREDICT_FALSE(internal::MultiplyWithOverflow(
length, static_cast<int64_t>(sizeof(T)), &num_bytes))) {
return Status::CapacityError("Advance: size overflow");
}
return bytes_builder_.Advance(num_bytes);
}

void UnsafeAdvance(const int64_t length) {
Expand All @@ -316,7 +396,15 @@ class TypedBufferBuilder<
/// only for memory allocation).
Result<std::shared_ptr<Buffer>> FinishWithLength(int64_t final_length,
bool shrink_to_fit = true) {
return bytes_builder_.FinishWithLength(final_length * sizeof(T), shrink_to_fit);
if (ARROW_PREDICT_FALSE(final_length < 0)) {
return Status::Invalid("FinishWithLength: negative final length");
}
int64_t num_bytes;
if (ARROW_PREDICT_FALSE(internal::MultiplyWithOverflow(
final_length, static_cast<int64_t>(sizeof(T)), &num_bytes))) {
return Status::CapacityError("FinishWithLength: final length overflow");
}
return bytes_builder_.FinishWithLength(num_bytes, shrink_to_fit);
}

void Reset() { bytes_builder_.Reset(); }
Expand Down Expand Up @@ -429,9 +517,18 @@ class TypedBufferBuilder<bool> {
}

Status Reserve(const int64_t additional_elements) {
return Resize(
BufferBuilder::GrowByFactor(bit_length_, bit_length_ + additional_elements),
false);
if (ARROW_PREDICT_FALSE(additional_elements < 0)) {
return Status::Invalid("Reserve: negative additional_elements");
}
int64_t min_length;
if (ARROW_PREDICT_FALSE(
internal::AddWithOverflow(bit_length_, additional_elements, &min_length))) {
return Status::CapacityError("Reserve: capacity overflow");
}
if (ARROW_PREDICT_FALSE(BufferBuilderCapacityTooLarge(min_length))) {
return Status::CapacityError("Reserve: capacity overflow");
}
return Resize(min_length, false);
}

Status Advance(const int64_t length) {
Expand Down
23 changes: 23 additions & 0 deletions cpp/src/arrow/buffer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,19 @@ TEST(TestBufferBuilder, ResizeReserve) {
ASSERT_EQ(9, builder.length());
}

TEST(TestBufferBuilder, InvalidReserveAndAppendLengths) {
const std::string data = "x";
auto data_ptr = data.c_str();
BufferBuilder builder;

ASSERT_RAISES(Invalid, builder.Append(data_ptr, -1));
ASSERT_RAISES(Invalid, builder.Reserve(-1));
ASSERT_RAISES(Invalid, builder.Advance(-1));

const int64_t overflow_add = std::numeric_limits<int64_t>::max() - 8;
ASSERT_RAISES(CapacityError, builder.Reserve(overflow_add));
}

TEST(TestBufferBuilder, Alignment) {
const std::string data = "some data";
auto data_ptr = data.c_str();
Expand Down Expand Up @@ -862,6 +875,16 @@ TYPED_TEST(TypedTestBufferBuilder, AppendCopies) {
}
}

TYPED_TEST(TypedTestBufferBuilder, NegativeAndOverflowAppend) {
TypedBufferBuilder<TypeParam> builder;

ASSERT_RAISES(Invalid, builder.Append(-1, static_cast<TypeParam>(0)));

const int64_t max_num_elements = std::numeric_limits<int64_t>::max() / sizeof(TypeParam) + 1;
ASSERT_RAISES(CapacityError,
builder.Append(max_num_elements, static_cast<TypeParam>(0)));
}

TEST(TestBoolBufferBuilder, Basics) {
TypedBufferBuilder<bool> builder;

Expand Down
Loading