fix: address UB audit soundness findings#30924
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPR hardens unsafe code across concurrent and encoding paths: auto-trait impls for generic types enforce stricter ChangesSafety and Correctness Improvements
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/errno/linux_errno.rs`:
- Around line 191-193: The code currently collapses unknown raw syscall errno
values to SystemErrno::SUCCESS via SystemErrno::init(int as
i64).unwrap_or(SystemErrno::SUCCESS), which converts real kernel errors into
false successes; instead stop treating unknown values as success by returning or
propagating the failure from SystemErrno::init (or mapping to an explicit
failure sentinel). Replace the unwrap_or(SystemErrno::SUCCESS) usage in the
SystemErrno::init call with one of: propagate the Option/Result to the caller so
callers must handle None/Err, map unknown raw errno to an explicit failure
variant (e.g. add and return SystemErrno::UNKNOWN_RAW or
SystemErrno::UNMAPPED_ERROR) rather than SUCCESS, or explicitly fail
(panic/return Err) to "fail closed" so unknown kernel error codes are not
treated as successes; adjust the surrounding function signature accordingly to
return the propagated error variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8632e3fb-66ba-4cb9-aea8-e509451e3040
📒 Files selected for processing (8)
src/ast/nodes.rssrc/bun_core/atomic_cell.rssrc/bun_core/deprecated.rssrc/errno/lib.rssrc/errno/linux_errno.rssrc/errno/windows_errno.rssrc/runtime/webcore/encoding.rssrc/threading/guarded.rs
`bun_core::deprecated::DoublyLinkedList<T>` and its unit tests failed under Miri's Stacked Borrows model. The test handed the list `&mut node` references, the list stored raw `*mut node` links, and later `&mut node` reborrows invalidated the raw-pointer tags before traversal:
error: Undefined Behavior: attempting a read access ... tag does not exist in the borrow stack
Reproducer against the pre-fix source:
MIRIFLAGS="-Zmiri-strict-provenance" cargo +nightly miri test \
-p bun_core --lib basic_doubly_linked_list_test
The type had no production callers; only its own tests and one `TODO(port): std.DoublyLinkedList` comment in `src/jsc/web_worker.rs` referenced it. Because `deprecated.rs` already records removed Zig-port helpers, this commit replaces the unused type, impls, and tests with a short removal note and leaves future intrusive-list work to use pinned/list-owned nodes or an intrusive-list adapter.
Source: `.ub-exorcism/2026-05-15-exhaustive/UNDEFINED_BEHAVIOR_EXPERIMENT_DESIGNS.md` entry `EXP-094`. Full audit context: PR oven-sh#30903.
Dicklesworthstone
force-pushed
the
claude/ub-fixes-demo-1
branch
from
256f657 to
a36de0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ast/nodes.rs`:
- Around line 339-343: StoreSlice<T> is unsound because it's Copy and exposes a
safe slice_mut(self) that takes self by value, allowing duplicated copies to
each produce &mut [T] to the same backing storage; fix by removing the Copy impl
OR change the slice_mut signature to require &mut self (or alternatively mark
slice_mut as unsafe) so that multiple aliased copies cannot create independent
mutable references; update the StoreSlice<T> type impls (the Copy impl and the
slice_mut method) accordingly and ensure Send/Sync bounds remain appropriate for
the payload type.
In `@src/bun_core/atomic_cell.rs`:
- Around line 59-65: The unsafe auto-trait impls for AtomicCell are missing a T:
Send requirement; update the two unsafe impls for Sync and Send to require T:
Send (i.e., change unsafe impl<T: Atom> Sync for AtomicCell<T> {} and unsafe
impl<T: Atom> Send for AtomicCell<T> {} to include + Send on T) so the container
cannot be auto-marked Send/Sync for types that are !Send; make the same T: Send
change anywhere else you expose auto-trait impls for AtomicCell to keep bounds
consistent.
In `@src/bun_core/deprecated.rs`:
- Around line 101-113: Remove the deleted-code rationale comment block that
begins "The Rust port of `std.DoublyLinkedList` / `DoublyLinkedNode` was
removed..." so the module only contains current surface-level documentation, not
historical justification; delete that entire multi-line explanatory paragraph
(the block about Miri/Stacked Borrows, callers, and future intrusive-list
suggestions) and leave no leftover references to the removed type in
deprecated.rs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed0836f0-a8c6-477c-9af6-af3bc776c69d
📒 Files selected for processing (7)
src/ast/nodes.rssrc/bun_core/atomic_cell.rssrc/bun_core/deprecated.rssrc/errno/lib.rssrc/errno/linux_errno.rssrc/errno/windows_errno.rssrc/threading/guarded.rs
| // | ||
| // The bound matches sister `StoreRef<T>`: without it, `StoreSlice<Cell<U>>` | ||
| // would be `Send + Sync` even though `Cell<U>` is `!Sync`. | ||
| unsafe impl<T: Send> Send for StoreSlice<T> {} | ||
| unsafe impl<T: Sync> Sync for StoreSlice<T> {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 \
'impl<T> Copy for StoreSlice<T>|pub fn slice_mut|unsafe impl<T: Send> Send for StoreSlice<T>|unsafe impl<T: Sync> Sync for StoreSlice<T>' \
src/ast/nodes.rsRepository: oven-sh/bun
Length of output: 824
StoreSlice<T> is unsound: narrowing bounds on Send/Sync does not prevent aliased mutable access.
Line 328 makes StoreSlice<T> unconditionally Copy, and line 412 exposes slice_mut(self) as a safe method taking self by value. A shared &StoreSlice<T> can be copied freely, and each copy can independently call slice_mut() to produce a distinct &mut [T] to the same backing allocation—violating Rust's mutable aliasing invariant. The unsafe trait bounds on lines 342–343 only constrain the payload type T, not the receiver semantics. Either make StoreSlice<T> non-Copy, require &mut self in slice_mut, or mark slice_mut as unsafe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ast/nodes.rs` around lines 339 - 343, StoreSlice<T> is unsound because
it's Copy and exposes a safe slice_mut(self) that takes self by value, allowing
duplicated copies to each produce &mut [T] to the same backing storage; fix by
removing the Copy impl OR change the slice_mut signature to require &mut self
(or alternatively mark slice_mut as unsafe) so that multiple aliased copies
cannot create independent mutable references; update the StoreSlice<T> type
impls (the Copy impl and the slice_mut method) accordingly and ensure Send/Sync
bounds remain appropriate for the payload type.
| // SAFETY: `T: Atom` means every access routes through a native atomic backing | ||
| // (or the `AtomicPtr` specialization for pointers) and `T: Copy` means no drop | ||
| // glue can race. Non-atomic `Copy` payloads may still use `new()`/`into_inner()` | ||
| // locally, but they must not auto-conjure cross-thread Send/Sync through this | ||
| // wrapper. | ||
| unsafe impl<T: Atom> Sync for AtomicCell<T> {} | ||
| unsafe impl<T: Atom> Send for AtomicCell<T> {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 \
'unsafe impl<T: Atom> (Sync|Send) for AtomicCell<T>|pub unsafe trait Atom: Copy|pub fn into_inner|pub fn load\(|pub fn swap\(|pub fn compare_exchange' \
src/bun_core/atomic_cell.rsRepository: oven-sh/bun
Length of output: 1303
🏁 Script executed:
sed -n '193,210p' src/bun_core/atomic_cell.rsRepository: oven-sh/bun
Length of output: 622
🏁 Script executed:
sed -n '78,115p' src/bun_core/atomic_cell.rsRepository: oven-sh/bun
Length of output: 1193
Require T: Send on AtomicCell's auto-trait bounds.
The Atom trait constrains only atomic representation and size; it does not guarantee thread-safe movement of T. However, this API exposes owned T values across threads via into_inner(), load(), swap(), and compare_exchange() on shared references. An external unsafe impl Atom for !Send CopyType would currently auto-impl Send + Sync for AtomicCell<ThatType>, allowing unsound cross-thread value movement. Both implementations require at least T: Send.
Suggested fix
-unsafe impl<T: Atom> Sync for AtomicCell<T> {}
-unsafe impl<T: Atom> Send for AtomicCell<T> {}
+unsafe impl<T: Atom + Send> Sync for AtomicCell<T> {}
+unsafe impl<T: Atom + Send> Send for AtomicCell<T> {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SAFETY: `T: Atom` means every access routes through a native atomic backing | |
| // (or the `AtomicPtr` specialization for pointers) and `T: Copy` means no drop | |
| // glue can race. Non-atomic `Copy` payloads may still use `new()`/`into_inner()` | |
| // locally, but they must not auto-conjure cross-thread Send/Sync through this | |
| // wrapper. | |
| unsafe impl<T: Atom> Sync for AtomicCell<T> {} | |
| unsafe impl<T: Atom> Send for AtomicCell<T> {} | |
| // SAFETY: `T: Atom` means every access routes through a native atomic backing | |
| // (or the `AtomicPtr` specialization for pointers) and `T: Copy` means no drop | |
| // glue can race. Non-atomic `Copy` payloads may still use `new()`/`into_inner()` | |
| // locally, but they must not auto-conjure cross-thread Send/Sync through this | |
| // wrapper. | |
| unsafe impl<T: Atom + Send> Sync for AtomicCell<T> {} | |
| unsafe impl<T: Atom + Send> Send for AtomicCell<T> {} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bun_core/atomic_cell.rs` around lines 59 - 65, The unsafe auto-trait
impls for AtomicCell are missing a T: Send requirement; update the two unsafe
impls for Sync and Send to require T: Send (i.e., change unsafe impl<T: Atom>
Sync for AtomicCell<T> {} and unsafe impl<T: Atom> Send for AtomicCell<T> {} to
include + Send on T) so the container cannot be auto-marked Send/Sync for types
that are !Send; make the same T: Send change anywhere else you expose auto-trait
impls for AtomicCell to keep bounds consistent.
| // | ||
| // The Rust port of `std.DoublyLinkedList` / `DoublyLinkedNode` was removed | ||
| // after its in-tree unit test failed under Miri (Stacked Borrows): callers | ||
| // hand the list `&mut node` references whose tags are then invalidated by | ||
| // later `&mut node` re-borrows on the same stack-local, while the list | ||
| // still traverses the stale raw `*mut node` links. The struct had no | ||
| // callers outside its own unit test, so deletion is the safe fix. The one | ||
| // in-tree comment that referenced the type (`src/jsc/web_worker.rs`'s | ||
| // `TODO(port): std.DoublyLinkedList` for `WebWorker.live_{next,prev}`) | ||
| // remains a TODO. Future intrusive-list needs should pick a design that | ||
| // does not interleave `*mut node` and `&mut node` on the same allocation | ||
| // (for example pinned/list-owned nodes, an `intrusive-collections` adapter, | ||
| // or a `Box`-owning list). |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Drop the deletion-rationale block from source.
This is documenting removed code rather than the current module surface. Please keep that rationale in the PR/commit history and delete the block here.
As per coding guidelines, "Never add comments to deleted code blocks" and "Avoid adding comments where not necessary".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bun_core/deprecated.rs` around lines 101 - 113, Remove the deleted-code
rationale comment block that begins "The Rust port of `std.DoublyLinkedList` /
`DoublyLinkedNode` was removed..." so the module only contains current
surface-level documentation, not historical justification; delete that entire
multi-line explanatory paragraph (the block about Miri/Stacked Borrows, callers,
and future intrusive-list suggestions) and leave no leftover references to the
removed type in deprecated.rs.
|
Thanks for the careful review @coderabbitai — going through your findings: Addressed (already in this PR):
Not yet addressed — tracking for a follow-up PR:
Style (deferred):
The two critical findings are filed in my audit-followup tracking. Will open separate PRs for each. |
|
Tip For best results, initiate chat on the files or code changes.
Quick summary of where things stand:
Would you like me to open GitHub tracking issues for the two soundness findings ( (ᓀ ᓀ) 🐇 ✏️ Learnings added
|
|
Yes please @coderabbitai — open the two tracking issues for the StoreSlice aliasing and AtomicCell |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainOn it — creating both issues now and linking them back to 🏁 Script executed: Length of output: 154 The assignee setting failed due to integration permissions — let me check whether the issues themselves were created successfully. 🏁 Script executed: Length of output: 459
Both are linked back to this PR ( One heads-up: there's a pre-existing issue (ᓀ ᓀ) 🐇 |
PR oven-sh#31090 now owns the AtomicCell Send/Sync soundness follow-up raised from oven-sh#30924 review. Drop the partial EXP-098 change from this mixed audit PR so reviewers do not see two overlapping AtomicCell fixes.
|
De-overlap update: removed the partial EXP-098 AtomicCell change from this PR in 7f5e55f. The PR diff no longer touches src/bun_core/atomic_cell.rs; the PR body now marks EXP-098 as moved out. The remaining AtomicCell soundness issue is tracked by #31089 / #30840 and handled by the focused follow-up #31090. |
|
Update — both deferred soundness findings are now fixed in dedicated PRs:
(@coderabbitai if you still want to open formal tracking issues, feel free — but these PRs supersede the followup-tracking-issue plan; tracking is via the PRs themselves now.) The `deprecated.rs:113` style cleanup remains the only deferred item from your review, and I'll fold it into a small follow-up if it's not already covered elsewhere. |
|
Tip For best results, initiate chat on the files or code changes.
The Tracking issues (ᓀ ᓀ) 🐇 ✏️ Learnings added
|
The previous bound `unsafe impl<T: Copy> Send/Sync for AtomicCell<T>` was too broad: any external `unsafe impl Atom for SomeCopyButNotSendType` would auto-derive `AtomicCell<ThatType>: Send + Sync`, which would let `load`/`swap`/`compare_exchange` hand owned `T` values across threads through a shared `&AtomicCell<T>` reference. Audit witness in the CodeRabbit review of oven-sh#30924 (src/bun_core/atomic_cell.rs:65). Introduce a new marker trait: pub unsafe trait AtomSend: Atom {} and tighten the bounds to `<T: AtomSend>`. `AtomSend` separates "OK as an atomic representation" (which `Atom` already guaranteed) from "OK to hand owned `T` between threads via atomic load/store" — the latter is what AtomicCell's auto-traits actually need. - The `unsafe_impl_atom!` macro now emits both `Atom` and `AtomSend`, matching the macro's existing safety contract (caller asserts Self is fully POD-shaped and the cross-thread story matches the underlying atomic word's semantics). In-tree macro users (`bool`, `char`, the integer/float scalars, `Level`, `Winsize`) all keep `AtomicCell<Self>: Send + Sync` with no change. - The pointer specializations (`*mut U`, `*const U`, `Option<NonNull<U>>`) manually implement `AtomSend`, matching `AtomicPtr<U>: Send + Sync` unconditionally — the atomic op makes the address transfer well-defined, and dereferencing remains the caller's hazard. - Hand-rolled `unsafe impl Atom for FunkyType` (without using the macro) no longer auto-derives Send/Sync on AtomicCell. The implementor must now explicitly add `unsafe impl AtomSend for FunkyType` to opt in, forcing them to consider the cross-thread story separately from the atomic-representation story. Verified: - `cargo check --workspace` clean - `cargo check --tests -p bun_core` clean - Added compile-time test `atomic_cell_atomsend_marker_is_required_for_send` asserting the standard usages still pass through (`AtomicCell<u64>`, `AtomicCell<*mut u8>`, `AtomicCell<Option<NonNull<u8>>>`) - Standalone witness at `/tmp/atomicell-witness/`: a downstream `unsafe impl Atom for NotSend` (where `NotSend: Copy + !Send`) without a paired `AtomSend` impl now FAILS to compile through `assert_send::<AtomicCell<NotSend>>()` with `the trait bound NotSend: AtomSend is not satisfied`. Pre-fix the same code compiled, exposing the hole CodeRabbit flagged. Refs oven-sh#30903, oven-sh#30924. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Correction on my earlier reply (the one starting "Thanks for the careful review @coderabbitai…") — I said `deprecated.rs:113` was still deferred, but I missed that the relevant code was already deleted in `781811cbdd` bun_core: delete unused DoublyLinkedList (audit EXP-094, Miri SB UB)` two days prior. The deletion is a stronger fix than the comment cleanup you flagged: the test was hitting a Miri Stacked Borrows UB (raw-pointer tag invalidation when `&mut node` reborrows occurred between insertion and traversal), so removing the whole unused type closes the audit finding entirely rather than just tidying the rationale comment. So the actual status on this PR's three CodeRabbit findings:
Nothing actually deferred — apologies for the earlier mis-call. |
|
Tip For best results, initiate chat on the files or code changes.
Updated final status for all CodeRabbit findings on this PR:
Everything is resolved. 🐇 ✏️ Learnings added
|
2 participants
Follow-up to the UB audit artifacts in #30903. This PR contains source fixes only; no audit artifacts, beads, lockfile changes, or generated-output churn.
What this fixes:
DoublyLinkedList/DoublyLinkedNodecode whose in-tree unit test reproduced Stacked Borrows UB under Miri.SystemErrno; impossible raw errnos now fail closed instead of mapping to success.GuardedLock!Send/!Sync, matching sibling mutex guard behavior for OS backends that require unlock on the locking thread.StoreSlice<T>Send/Syncimpls onT, matching the sisterStoreRef<T>discipline.from_rawerrno constructors so invalid discriminants panic instead of constructing invalid repr enums.Moved out of this PR:
AtomicCell<T>auto-trait hardening) is intentionally no longer part of this mixed audit PR. Review found the initialT: Atombound was incomplete; the remaining issue is tracked in soundness: AtomicCell<T> unsafe impl Send/Sync missing T: Send bound #31089 / rust: AtomicCell<Pointer<T>> is used as AtomicPtr<T> replacement #30840 and handled by the focused follow-up bun_core: gate AtomicCell Send/Sync on AtomCrossThread marker #31090. This keeps this PR non-overlapping with that focused AtomicCell fix.Notes:
mainalready landed the UTF-16Vec<u8>->Vec<u16>layout fix in encoding: fix Vec<u8>->Vec<u16> layout UB in fs.readFile utf16le path #30926.mainalready deletedsrc/bun.rs, which removed the unusedunsafe_assert(bool)facade.Verification run locally after rebasing onto current
origin/main:cargo +nightly check -p bun_core -p bun_ast -p bun_errno -p bun_threadingcargo +nightly check -p bun_errno --target x86_64-pc-windows-msvccargo +nightly test -p bun_errno --lib system_errno_from_raw_rejects_invalid_safe_inputcargo +nightly test -p bun_errno --lib linux_raw_syscall_errno_rejects_invalid_tagcargo +nightly miri test -p bun_errno --libVerification after de-overlapping EXP-098 from this PR:
env CARGO_TARGET_DIR=/tmp/bun-pr30924-dedupe-target cargo +nightly check -p bun_core -p bun_ast -p bun_errno -p bun_threadinggit diff --check origin/main...HEAD