Skip to content

GH-48311: [C++] Fix OOB memory access in buffered IO#48322

Merged
mapleFU merged 6 commits intoapache:mainfrom
chegoryu:fix-buffered-io
Jan 15, 2026
Merged

GH-48311: [C++] Fix OOB memory access in buffered IO#48322
mapleFU merged 6 commits intoapache:mainfrom
chegoryu:fix-buffered-io

Conversation

@chegoryu
Copy link
Copy Markdown
Contributor

@chegoryu chegoryu commented Dec 3, 2025 •
edited by github-actions Bot
Loading

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:

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-actions
Copy link
Copy Markdown

github-actions Bot commented Dec 3, 2025

⚠️ GitHub issue #48311 has been automatically assigned in GitHub to PR creator.

@chegoryu
Copy link
Copy Markdown
Contributor Author

chegoryu commented Jan 1, 2026

@wgtmac Hi, I don't know who to ping, so I chose you since you’ve looked at one of my reviews.

Can you suggest the right person to call to review this? This bug is really annoying and appears very randomly (and looks like a corrupted allocator, so it's hard to debug).

@wgtmac
Copy link
Copy Markdown
Member

wgtmac commented Jan 7, 2026

cc @mapleFU

mapleFU
mapleFU reviewed Jan 7, 2026
View reviewed changes
Comment thread cpp/src/arrow/io/buffered.cc
// No need to reserve space for more than the total remaining number of bytes.
if (bytes_buffered_ == 0) {
// Special case: we can not keep the current buffer because it does not
// Special case: we can override data in the current buffer because it does not
Copy link
Copy Markdown
Member

@mapleFU mapleFU Jan 7, 2026

Choose a reason for hiding this comment

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

Can you rewrite comment in SetBufferSize? Since buffer_pos_ would be rewritten now

Copy link
Copy Markdown
Contributor Author

@chegoryu chegoryu Jan 7, 2026

Choose a reason for hiding this comment

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

Missed that comment, thanks, fixed

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 7, 2026
@mapleFU
Copy link
Copy Markdown
Member

mapleFU commented Jan 8, 2026 •
edited
Loading

@pitrou Would you mind take a look? Otherwise would merge if no objection next Tuesday

@mapleFU mapleFU requested a review from pitrou January 12, 2026 09:18
@mapleFU mapleFU merged commit fb0bac6 into apache:main Jan 15, 2026
51 checks passed
@mapleFU mapleFU removed the awaiting committer review Awaiting committer review label Jan 15, 2026
@mapleFU mapleFU mentioned this pull request Jan 15, 2026
Closed
@conbench-apache-arrow
Copy link
Copy Markdown

conbench-apache-arrow Bot commented Jan 15, 2026

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit fb0bac6.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

raulcd pushed a commit that referenced this pull request Feb 3, 2026
@chegoryu @mapleFU @raulcd
GH-48311: [C++] Fix OOB memory access in buffered IO (#48322)
34bf0e2 
### 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mapleFU mapleFU mapleFU approved these changes

@pitrou pitrou Awaiting requested review from pitrou

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chegoryu @wgtmac @mapleFU @raulcd @pitrou