kou commented on code in PR #45498:
URL: https://github.com/apache/arrow/pull/45498#discussion_r1951796744
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -621,6 +645,25 @@ TEST(BlockParser, MismatchingNumColumns) {
EXPECT_RAISES_WITH_MESSAGE_THAT(
Invalid, testing::HasSubstr("CSV parse error: Expected 2 columns, got
1: a"), st);
}
+ // Vary the number of columns and mismatch, to catch buffer overflow issues
+ for (int32_t num_cols : {1, 2, 5, 10, 100}) {
+ ARROW_SCOPED_TRACE("num_cols = ", num_cols);
+ for (int32_t mismatch : {-20, -5, -4, -1, 1, 2, 5, 10, 50, 1024, 32767}) {
Review Comment:
Do we need all these cases?
It seems that `-1, 1, 32767` are enough.
##########
cpp/src/arrow/csv/parser_test.cc:
##########
@@ -621,6 +645,25 @@ TEST(BlockParser, MismatchingNumColumns) {
EXPECT_RAISES_WITH_MESSAGE_THAT(
Invalid, testing::HasSubstr("CSV parse error: Expected 2 columns, got
1: a"), st);
}
+ // Vary the number of columns and mismatch, to catch buffer overflow issues
+ for (int32_t num_cols : {1, 2, 5, 10, 100}) {
Review Comment:
Why do we need `2, 5, 10` here? `1, 100` aren't enough?
##########
cpp/src/arrow/csv/parser.cc:
##########
@@ -158,7 +173,7 @@ class ResizableValueDescWriter : public
ValueDescWriter<ResizableValueDescWriter
void PushValue(ParsedValueDesc v) {
if (ARROW_PREDICT_FALSE(values_size_ == values_capacity_)) {
values_capacity_ = values_capacity_ * 2;
- ARROW_CHECK_OK(values_buffer_->Resize(values_capacity_ *
sizeof(*values_)));
+ status_ &= values_buffer_->Resize(values_capacity_ * sizeof(*values_));
values_ =
reinterpret_cast<ParsedValueDesc*>(values_buffer_->mutable_data());
}
values_[values_size_++] = v;
Review Comment:
If the `Resize()` failed, is `values_[values_size_++] = v` safe?
--
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]
