[runtime/iouring-*] fix write-after-free bug #1003
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a write‐after‐free bug by updating the StableBuf trait and propagating the correct buffer type through the io_uring storage and network layers. The key changes include:
- Adding Sync to the StableBuf trait to ensure thread safety.
- Updating mpsc channel payloads to pass an Arc‑wrapped StableBuf.
- Revising buffer handling in read, write, send, and recv operations to avoid use-after-free issues.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| utils/src/stable_buf.rs | Added Sync to the StableBuf trait for thread safety. |
| runtime/src/storage/iouring.rs | Updated mpsc channel payloads and buffer handling. |
| runtime/src/network/iouring.rs | Adjusted channel signatures and buffer conversions. |
| runtime/src/iouring/mod.rs | Revised channel handling and updated tests accordingly. |
There was a problem hiding this comment.
Pull Request Overview
This PR extends the io_uring integration to carry buffer references across async operations and ensures they are not freed prematurely, fixing a write-after-free bug.
- Added
Syncbound toStableBufso buffers can safely cross thread boundaries - Updated all
io_sender/submitterchannels to carry anOption<Arc<dyn StableBuf>> - Wrapped buffers in
Arcin read/write/send/recv implementations and stored them in-flight until completion
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| utils/src/stable_buf.rs | Added Sync bound to StableBuf trait |
| runtime/src/storage/iouring.rs | Extended channel tuples with buffer Arc and updated Blob read/write |
| runtime/src/network/iouring.rs | Extended network/send/recv channels with buffer Arc and updated Sink/Stream |
| runtime/src/iouring/mod.rs | Updated run loop to accept and drop buffer Arc from waiters |
Comments suppressed due to low confidence (1)
utils/src/stable_buf.rs:10
- Adding a
Syncbound toStableBufis a breaking change for existing implementors. Verify that all currentStableBufimplementations are updated to satisfySync.
pub unsafe trait StableBuf: Unpin + Send + Sync + 'static {
runtime/src/network/iouring.rs
Outdated
| let buf_ref = unsafe { std::slice::from_raw_parts_mut(buf.stable_mut_ptr(), buf_len) }; | ||
| let buf_arc = Arc::new(buf); |
There was a problem hiding this comment.
The pointer is taken from buf before it's moved into the Arc, which can break for inline-buffer types (e.g., arrays). You should wrap buf in an Arc first and then call stable_mut_ptr on the Arc to get a stable pointer.
| let buf_ref = unsafe { std::slice::from_raw_parts_mut(buf.stable_mut_ptr(), buf_len) }; | |
| let buf_arc = Arc::new(buf); | |
| let buf_arc = Arc::new(buf); | |
| let buf_ref = unsafe { std::slice::from_raw_parts_mut(buf_arc.stable_mut_ptr(), buf_len) }; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think this is safe for the only types that actually implement StableBufMut -- namely Vec<u8> and BytesMut. When we move buf into buf_arc we are moving ownership, but the underlying bytes remain in the same place.
There was a problem hiding this comment.
This is arguably kind of brittle. Open to ideas for how to improve.
There was a problem hiding this comment.
Changed so that we take the reference to the StableBuf / StableBufMut after we put it into an Arc
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a write‐after‐free bug by improving buffer lifetime management and ensuring thread safety. Key changes include updating the StableBuf trait to include Sync, wrapping buffers with Arc in io_uring operations for both storage and network, and replacing tuple-based channel messages with a unified Op struct.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| utils/src/stable_buf.rs | Added Sync to the StableBuf trait to enforce thread safety. |
| runtime/src/storage/iouring.rs | Updated read_at and write_at methods to wrap buffers in an Arc, ensuring they remain valid. |
| runtime/src/network/iouring.rs | Replaced tuple channels with iouring::Op and wrapped buffers in an Arc for both sending and receiving. |
| runtime/src/iouring/mod.rs | Introduced the Op struct and updated the run function to manage buffer lifetimes via Arc. |
Comments suppressed due to low confidence (1)
utils/src/stable_buf.rs:10
- The addition of Sync to StableBuf improves thread safety; please ensure that all current StableBuf implementors are compatible with this change.
pub unsafe trait StableBuf: Unpin + Send + Sync + 'static {
|
As part of merging this, I think we should re-enable the network tests? |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a write‐after‐free bug by ensuring that buffers passed to io_uring operations live past request submission, and it updates the buffer API across several modules to use the new StableBuf abstraction.
- Replace Vec with StableBuf to manage buffer lifetimes correctly.
- Update API signatures to use Into for improved flexibility and consistency.
- Refactor io_uring operation submission to include buffer ownership, avoiding premature deallocation.
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| runtime/src/utils/buffer/read.rs | Modified to use StableBuf instead of Vec in buffer management. |
| runtime/src/storage/* | Updated buffer parameters and conversion API for storage operations. |
| runtime/src/network/* | Refactored network buffer handling in send/recv and test assertions. |
| runtime/src/iouring/mod.rs | Adjusted io_uring operation submission to include buffer lifetime management. |
| .github/workflows/*.yml | Adjusted timeouts and re-enabled previously commented-out features. |
| runtime/src/deterministic.rs | Updated buffer assertions to align with the new StableBuf API. |
Comments suppressed due to low confidence (1)
runtime/src/iouring/mod.rs:237
- The variable 'next_work_id' is not incremented after assignment, which may lead to work ID collisions when multiple operations are submitted. Consider adding an increment (e.g. next_work_id += 1) after assigning work_id.
let work_id = next_work_id;
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1003 +/- ##
==========================================
+ Coverage 90.97% 90.99% +0.01%
==========================================
Files 192 191 -1
Lines 54094 54232 +138
==========================================
+ Hits 49212 49347 +135
- Misses 4882 4885 +3
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
(Hopefully) fixes write-after-free panics we've seen in some CI tests. This PR passes the buffer being acted on by the io_uring to the iouring event loop to ensure the buffer lives past the operation's completion.
Resolves #954