tadeja commented on code in PR #49883:
URL: https://github.com/apache/arrow/pull/49883#discussion_r3538387381
##########
cpp/src/arrow/buffer_builder.h:
##########
@@ -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"
Review Comment:
```suggestion
#include "arrow/util/bitmap_generate.h"
#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
```
##########
cpp/src/arrow/buffer_builder.h:
##########
@@ -104,15 +113,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))) {
Review Comment:
```suggestion
if (ARROW_PREDICT_FALSE(
internal::AddWithOverflow(current_capacity, current_capacity,
&doubled))) {
```
Fix failing [Lint C++
clang-format](https://github.com/apache/arrow/actions/runs/25537781751/job/74957044077?pr=49883#step:5:107)
##########
cpp/src/arrow/buffer_builder.h:
##########
@@ -429,9 +495,15 @@ 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");
+ }
+ return Resize(min_length, false);
Review Comment:
```suggestion
return Resize(BufferBuilder::GrowByFactor(bit_length_, min_length),
false);
```
[Second commit
dropped](https://github.com/apache/arrow/pull/49883/changes/b5ea90c22394e41767f87945520c723e3a1bdfa5#diff-97bafa416fe89efc193d47ca8319d6710b8e41016d97f97e3c7ab48ef8eafbfbL501)
the `GrowByVector` wrapper, changing growth from 'double' to 'exact fit' and
so making it quadratic bitmap growth. Restore to amortized geometric growth
with `GrowByVector`.
##########
cpp/src/arrow/buffer_builder.h:
##########
@@ -250,12 +274,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");
+ }
Review Comment:
```suggestion
```
Optionally remove redundant `num_copies` negative check which is also done
on the next line with `ARROW_RETURN_NOT_OK(Reserve(num_copies));` that returns
`Status::Invalid("Reserve: negative additional_elements")`.
##########
cpp/src/arrow/buffer_test.cc:
##########
@@ -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));
Review Comment:
```suggestion
ASSERT_RAISES(OutOfMemory, builder.Reserve(overflow_add));
```
Adjust the test error code too.
##########
cpp/src/arrow/memory_pool.cc:
##########
@@ -69,6 +69,17 @@ namespace {
constexpr char kDefaultBackendEnvVar[] = "ARROW_DEFAULT_MEMORY_POOL";
constexpr char kDebugMemoryEnvVar[] = "ARROW_DEBUG_MEMORY_POOL";
+constexpr int64_t kMaxBufferCapacity = std::numeric_limits<int64_t>::max() -
63;
+
+Status ValidateBufferCapacity(int64_t capacity, const char* operation) {
+ if (ARROW_PREDICT_FALSE(capacity < 0)) {
+ return Status::Invalid(operation, ": negative capacity");
Review Comment:
```suggestion
return Status::Invalid(operation, ": negative capacity: ", capacity);
```
##########
cpp/src/arrow/memory_pool.cc:
##########
@@ -69,6 +69,17 @@ namespace {
constexpr char kDefaultBackendEnvVar[] = "ARROW_DEFAULT_MEMORY_POOL";
constexpr char kDebugMemoryEnvVar[] = "ARROW_DEBUG_MEMORY_POOL";
+constexpr int64_t kMaxBufferCapacity = std::numeric_limits<int64_t>::max() -
63;
+
+Status ValidateBufferCapacity(int64_t capacity, const char* operation) {
+ if (ARROW_PREDICT_FALSE(capacity < 0)) {
+ return Status::Invalid(operation, ": negative capacity");
+ }
+ if (ARROW_PREDICT_FALSE(capacity > kMaxBufferCapacity)) {
+ return Status::CapacityError(operation, ": capacity overflow");
Review Comment:
```suggestion
return Status::OutOfMemory(operation, ": capacity too large: ",
capacity);
```
Ensure backward compatibility of a public error code - `OutOfMemory`.
Additionally append `capacity` for easier debug.
##########
cpp/src/arrow/buffer_test.cc:
##########
@@ -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;
Review Comment:
```suggestion
const int64_t max_num_elements =
std::numeric_limits<int64_t>::max() / sizeof(TypeParam) + 1;
```
Fix failing [Lint C++
clang-format](https://github.com/apache/arrow/actions/runs/25537781751/job/74957044077?pr=49883#step:5:122).
##########
cpp/src/arrow/buffer_builder.h:
##########
Review Comment:
```suggestion
if (ARROW_PREDICT_FALSE(final_length < 0)) {
return Status::Invalid("FinishWithLength: negative final length");
}
const auto final_byte_length = bit_util::BytesForBits(final_length);
```
Add the missed negative check.
##########
cpp/src/arrow/memory_pool.cc:
##########
@@ -1026,6 +1031,7 @@ Result<std::unique_ptr<ResizableBuffer>>
AllocateResizableBuffer(const int64_t s
Result<std::unique_ptr<ResizableBuffer>> AllocateResizableBuffer(const int64_t
size,
const int64_t
alignment,
MemoryPool*
pool) {
+ RETURN_NOT_OK(ValidateBufferCapacity(size, "AllocateResizableBuffer"));
Review Comment:
```suggestion
```
Leftover redundant validation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
