Skip to content

Commit fb0bac6

Browse files
chegoryumapleFU
andauthored
GH-48311: [C++] Fix OOB memory access in buffered IO (#48322)
### Rationale for this change Fixing: #48311 ### What changes are included in this PR? Applied fix from #48311 and added test ### Are these changes tested? Yes, added test, without my patch test fails with debug check: ```cpp Note: Google Test filter = TestBufferedInputStream.PeekAfterExhaustingBuffer [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from TestBufferedInputStream [ RUN ] TestBufferedInputStream.PeekAfterExhaustingBuffer /Users/chegoryu/Junk/git/arrow/cpp/src/arrow/io/buffered.cc:337: Check failed: buffer_->size() - buffer_pos_ >= nbytes ``` ### Are there any user-facing changes? No, this PR fixes a bug * GitHub Issue: #48311 Lead-authored-by: Egor Chunaev <ch.egor.yu@gmail.com> Co-authored-by: mwish <maplewish117@gmail.com> Co-authored-by: chegoryu <ch.egor.yu@gmail.com> Signed-off-by: mwish <maplewish117@gmail.com>
1 parent d65eb94 commit fb0bac6

2 files changed

Lines changed: 44 additions & 4 deletions

File tree

cpp/src/arrow/io/buffered.cc

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -285,8 +285,9 @@ class BufferedInputStream::Impl : public BufferedBase {
285285

286286
// Resize internal read buffer. Note that the internal buffer-size
287287
// should not be larger than the raw_read_bound_.
288-
// It might change the buffer_size_, but will not change buffer states
289-
// buffer_pos_ and bytes_buffered_.
288+
// It might change the buffer_size_, and may reset buffer_pos_ to 0
289+
// when bytes_buffered_ == 0 to reuse the beginning of the buffer.
290+
// bytes_buffered_ will not be changed.
290291
Status SetBufferSize(int64_t new_buffer_size) {
291292
if (new_buffer_size <= 0) {
292293
return Status::Invalid("Buffer size should be positive");
@@ -297,12 +298,14 @@ class BufferedInputStream::Impl : public BufferedBase {
297298
new_buffer_size, ", buffer_pos: ", buffer_pos_,
298299
", bytes_buffered: ", bytes_buffered_, ", buffer_size: ", buffer_size_);
299300
}
301+
bool need_reset_buffer_pos = false;
300302
if (raw_read_bound_ >= 0) {
301303
// No need to reserve space for more than the total remaining number of bytes.
302304
if (bytes_buffered_ == 0) {
303-
// Special case: we can not keep the current buffer because it does not
305+
// Special case: we can override data in the current buffer because it does not
304306
// contain any required data.
305307
new_buffer_size = std::min(new_buffer_size, raw_read_bound_ - raw_read_total_);
308+
need_reset_buffer_pos = true;
306309
} else {
307310
// We should keep the current buffer because it contains data that
308311
// can be read.
@@ -311,7 +314,11 @@ class BufferedInputStream::Impl : public BufferedBase {
311314
buffer_pos_ + bytes_buffered_ + (raw_read_bound_ - raw_read_total_));
312315
}
313316
}
314-
return ResizeBuffer(new_buffer_size);
317+
auto status = ResizeBuffer(new_buffer_size);
318+
if (status.ok() && need_reset_buffer_pos) {
319+
buffer_pos_ = 0;
320+
}
321+
return status;
315322
}
316323

317324
Result<std::string_view> Peek(int64_t nbytes) {

cpp/src/arrow/io/buffered_test.cc

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,6 +514,39 @@ TEST_F(TestBufferedInputStream, PeekPastBufferedBytes) {
514514
ASSERT_EQ(0, buffered_->bytes_buffered());
515515
}
516516

517+
TEST_F(TestBufferedInputStream, PeekAfterExhaustingBuffer) {
518+
// GH-48311: When bytes_buffered_ == 0 and raw_read_bound_ >= 0,
519+
// SetBufferSize should reset buffer_pos_ to 0 and reuse the beginning of the buffer
520+
MakeExample1(/*buffer_size=*/10, default_memory_pool(), /*raw_read_bound=*/25);
521+
522+
// Fill the buffer
523+
ASSERT_OK_AND_ASSIGN(auto view, buffered_->Peek(10));
524+
EXPECT_EQ(view, kExample1.substr(0, 10));
525+
ASSERT_EQ(10, buffered_->bytes_buffered());
526+
ASSERT_EQ(10, buffered_->buffer_size());
527+
528+
// Read all buffered bytes to exhaust the buffer (bytes_buffered_ == 0),
529+
// at this point buffer_pos_ is non-zero
530+
ASSERT_OK_AND_ASSIGN(auto bytes, buffered_->Read(10));
531+
EXPECT_EQ(std::string_view(*bytes), kExample1.substr(0, 10));
532+
ASSERT_EQ(0, buffered_->bytes_buffered());
533+
ASSERT_EQ(10, buffered_->buffer_size());
534+
535+
// Peek should trigger SetBufferSize with bytes_buffered_ == 0,
536+
// which should reset buffer_pos_ to 0 and reuse the beginning of the buffer,
537+
// so resulting size of the buffer should be 15 instead of 25
538+
ASSERT_OK_AND_ASSIGN(view, buffered_->Peek(15));
539+
EXPECT_EQ(view, kExample1.substr(10, 15));
540+
ASSERT_EQ(15, buffered_->bytes_buffered());
541+
ASSERT_EQ(15, buffered_->buffer_size());
542+
543+
// Do read just in case
544+
ASSERT_OK_AND_ASSIGN(bytes, buffered_->Read(15));
545+
EXPECT_EQ(std::string_view(*bytes), kExample1.substr(10, 15));
546+
ASSERT_EQ(0, buffered_->bytes_buffered());
547+
ASSERT_EQ(15, buffered_->buffer_size());
548+
}
549+
517550
class TestBufferedInputStreamBound : public ::testing::Test {
518551
public:
519552
void SetUp() { CreateExample(/*bounded=*/true); }

0 commit comments

Comments
 (0)