uws: make stream-buffer conversion a one-shot ownership transfer - #31983
uws: make stream-buffer conversion a one-shot ownership transfer#31983robobun wants to merge 3 commits into
Conversation
us_socket_stream_buffer_t::to_stream_buffer took &self and rebuilt an owning Vec<u8> from raw parts without clearing them, so safe Rust could mint two owners of the same allocation (double free). Replace it with take_stream_buffer(&mut self) which nulls the raw parts, route destroy through it so teardown is idempotent, and make update drop any still-owned buffer before overwriting the parts. Add crate unit tests and run bun_uws_sys under the cargo-miri CI lane. Fixes #31971
|
Warning Review limit reached
More reviews will be available in 4 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 3:05 AM PT - Jun 10th, 2026
❌ @robobun, your commit 301c914 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31983That installs a local version of the PR into your bun-31983 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Not adding the suggested "Fixes" lines: this PR should not auto-close those issues. The unsound signature fixed here was a latent API hazard, not a live runtime bug. The only caller of the conversion, The three issues also sit on different paths: #31467 crashes in uWS C++ |
There was a problem hiding this comment.
I didn't find any issues — the ownership-transfer rework looks correct and well-tested — but since this changes unsafe raw-pointer ownership semantics on the node:http server write path and adds a crate to the Miri lane, I'd like a human to confirm before merging.
Extended reasoning...
Overview
This PR fixes a soundness hole in us_socket_stream_buffer_t: the old to_stream_buffer(&self) rebuilt an owning Vec<u8> via Vec::from_raw_parts without clearing the stored raw parts, so two safe calls could produce two owners of one allocation (double free). The fix renames it to take_stream_buffer(&mut self) and nulls list_ptr/list_len/list_cap/cursor after the rebuild, routes destroy and update through it (making destroy idempotent and update leak-free), updates the sole caller in src/runtime/socket/uws_jsc.rs, adds six Rust unit tests, wires bun_uws_sys into MIRI_CRATES + the workflow path filter, and adds a source-text lint under test/internal/.
Security risks
None introduced. The change strictly tightens memory safety: it removes a double-free reachable from safe Rust and a silent leak in update. No new attack surface, no auth/crypto/permissions involved.
Level of scrutiny
High. The code under change is unsafe Rust managing Vec raw-part ownership across the C++ FFI boundary, and us_socket_buffered_js_write is on the node:http server response write hot path. I traced the single in-tree caller and confirmed the take→work→update sequence is behaviorally unchanged (the new drop(self.take_stream_buffer()) at the top of update is a no-op there because the prior take already nulled the pointer). destroy now reconstructs with list_len instead of 0, which is equivalent for Vec<u8> drop. The 8-test count matches 6 new stream-buffer tests + 2 pre-existing quic/Header.rs classify tests, all FFI-free, so the Miri addition should be safe.
Other factors
No CODEOWNERS cover these paths. No prior reviewer comments to address. The bug-hunting pass found nothing. The reasoning and test coverage (including Miri mutation verification) are thorough. My only reason for not auto-approving is the criticality of the path: unsafe ownership changes at an FFI boundary in production HTTP server code warrant a human sign-off even when they look right.
|
CI status across three full runs (61359, 61666, 61705 after a maintainer branch update + one retrigger): every hard failure is reproduced on unrelated sibling branches, none touch this diff.
The diff's own lanes are green everywhere: cargo miri test (including the new |
Fixes #31971
Bug
us_socket_stream_buffer_t::to_stream_bufferwas a safe method taking&selfthat rebuilt an owningVec<u8>viaVec::from_raw_partswithout clearinglist_ptr/list_len/list_cap. Calling it twice, or once followed byus_socket_free_stream_buffer, produced two owners of the same allocation: a double free reachable from safe Rust. Both types are re-exported from the crate's public surface.Fix
In
src/uws_sys/us_socket_t.rs:to_stream_buffer(&self)is nowtake_stream_buffer(&mut self): it rebuilds theVecand nullslist_ptr/list_len/list_cap/cursor, so the returned buffer is the allocation's sole owner and a second take yields an empty buffer. The four field resets are the load-bearing lines.total_bytes_writtenis cumulative socket state, not buffer contents, and survives the take.destroyroutes through the take, so teardown is idempotent instead of relying on a "not called more than once" doc contract.updatedrops any still-owned buffer before overwriting the raw parts (previously a silent leak, same lifecycle class). Every method now maintains: non-nulllist_ptrmeans the struct owns exactly one decomposedVec.The only in-tree caller,
us_socket_buffered_js_write(src/runtime/socket/uws_jsc.rs), already ordered JS coercion before materializing the buffer and handed ownership back viaupdate, so node:http server write behavior is unchanged; the one-shot contract is now enforced by the signature rather than by call-site discipline. Its comments are updated for the new semantics.Tests
stream_buffer_testsinsrc/uws_sys/us_socket_t.rs: one-shot take, destroy-after-take, idempotent destroy, update-over-update,total_bytes_writtenpreservation, empty-capacity round trip. They are pure Rust (no FFI at test runtime), sobun_uws_sysis added toMIRI_CRATES(scripts/rust-miri.tsplus the path filter in.github/workflows/miri.yml): the Miri CI lane now runs them and deterministically flags a regression. Verified by mutation: reverting the field resets fails under Miri with "constructing invalid value of type &mut [u8]: encountered a dangling reference (use-after-free)"; removing the drop inupdatefails with "memory leaked".test/internal/uws-stream-buffer-ownership.test.ts: source-text lint pinning thetake_stream_buffer(&mut self)signature and the singleVec::from_raw_partssite, same pattern and placement asban-words.test.ts. The unsound path is not reachable from JS (the one caller ordered around it), so there is no behavioral JS repro; this is thebun bd testartifact and it fails on the unfixed source.Verification:
cargo test -p bun_uws_sys(8 pass) and the fullbun run rust:miriset pass.bun bd testpasses onnode-http-backpressure.test.ts,node-http-backpressure-max.test.ts,node-http-nested-cork.test.ts,node-http-ondata-reregister-leak.test.ts, andnode-http.test.ts(one pre-existing proxy-test failure there reproduces identically on the unmodified release build: environment ECONNREFUSED).