Fix memory pool lifetime for Arrow stream buffers - #464
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a use-after-free risk where Arrow stream buffers can outlive the arrow::MemoryPool used to allocate them, especially when readers are closed while callers still hold buffers (or async reads complete after close).
Changes:
- Keep the
arrow::MemoryPoolalive for the lifetime of returnedarrow::Bufferobjects usingshared_ptraliasing. - Add deterministic unit tests covering
Read,ReadAt, andReadAsyncwith a minimal deferredInputStreammock.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/paimon/common/utils/arrow/arrow_stream_adapter_test.cpp | Adds a deferred/mock input stream and new tests that ensure buffers remain valid after the adapter/pool are destroyed. |
| src/paimon/common/utils/arrow/arrow_input_stream_adapter.cpp | Wraps returned buffers so they retain ownership of the memory pool until buffer destruction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a0dca34 to
89c5009
Compare
| ASSERT_NE(adapter, nullptr); | ||
|
|
||
| auto result = adapter->Read(kTestSize); | ||
| ASSERT_TRUE(result.ok()); |
There was a problem hiding this comment.
could you use ASSERT_OK_AND_ASSIGN here
There was a problem hiding this comment.
No, seems to be an arrow::Result instead of paimon::Result here, so ASSERT_OK_AND_ASSIGN is not supported.
There was a problem hiding this comment.
Oh, I misspoke earlier. You can just use std::shared_ptr<arrow::Buffer> buffer = adapter->Read(kTestSize).ValueOrDie(); directly.
dd772d5 to
6e4fa8e
Compare
Keep the Arrow memory pool alive while stream buffers remain referenced. Add deterministic coverage for synchronous and asynchronous reads after adapter teardown. Co-authored-by: GPT-5.6 Terra <codex@users.noreply.github.com>
6e4fa8e to
5572ac1
Compare
I found this while debugging an S3 crash. I initially suspected the S3 filesystem, but the issue was in the Arrow input-stream adapter: a returned buffer can outlive its memory pool when a reader is closed while a caller still holds a buffer. This is more likely with network filesystems, where asynchronous reads may finish after the reader is closed.
This patch uses
shared_ptr's aliasing constructor to keep the pool alive with the returned buffer. It also adds deterministic coverage forRead,ReadAt, andReadAsyncusing a smallInputStreammock. Without the change, the tests crash; with it, they pass.