Skip to content

fix(runtime): harden EXP-041 shared-provenance cluster#30972

Open
Dicklesworthstone wants to merge 2 commits into
oven-sh:mainfrom
Dicklesworthstone:claude/ub-fixes-demo-2
Open

fix(runtime): harden EXP-041 shared-provenance cluster#30972
Dicklesworthstone wants to merge 2 commits into
oven-sh:mainfrom
Dicklesworthstone:claude/ub-fixes-demo-2

Conversation

@Dicklesworthstone
Copy link
Copy Markdown

@Dicklesworthstone Dicklesworthstone commented May 18, 2026

References the UB audit PR: #30903

This is a focused follow-up fix for audit EXP-041.

What changed:

  • WebSocketServerContext::Handler.active_connections no longer mutates a plain usize through a pointer derived from &self. It is now an AtomicUsize, with relaxed saturating updates and relaxed reads. The value is only a count; no synchronization contract is attached to it.
  • The sibling hand-written as_ctx_ptr(&self) -> *mut Self { ... } helper bodies called out in the EXP-041 cluster now route through the existing bun_ptr::AsCtxPtr helper, so the shared-provenance contract lives in one audited place instead of being repeated across runtime wrappers.
  • Interpreter::as_ctx_ptr intentionally remains an inherent method. Box<Interpreter> callers must produce *mut Interpreter, not *mut Box<Interpreter>; its body now uses ptr::from_ref(self).cast_mut().

Important scope note:

  • This PR does not claim to implement every remediation from audit: add UB-exorcism audit workspace + executive guide #30903.
  • Within EXP-041, it fixes the witnessed WebSocket mutable-counter UB and cleans up the sibling context-pointer helper pattern.
  • The sibling helpers are not converted to atomics because they are pointer-handoff helpers, not counters. Their contract is shared provenance plus interior mutability at the eventual mutation sites.

Verification run locally:

  • bun run fmt:rust
  • cargo check -p bun_runtime
  • bun run rust:check-all -> 10 ok, 0 failed
  • BUN_DEBUG_QUIET_LOGS=1 bun bd -e ... smoke test
  • BUN_DEBUG_QUIET_LOGS=1 bun bd test test/js/bun/websocket/websocket-server.test.ts -t sendText
  • git diff --check origin/main..HEAD
  • Targeted rg search for the old WebSocket raw-write shape and old self as *const Self helper bodies

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 18, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 16a43866-9928-4004-95b7-2dd62615f668

📥 Commits

Reviewing files that changed from the base of the PR and between 3b592ec and 7ff8ba2.

📒 Files selected for processing (2)
  • src/runtime/server/WebSocketServerContext.rs
  • src/runtime/server/mod.rs

Walkthrough

Handler.active_connections changes from plain usize to core::cell::Cell with helper methods updated to use Cell::get()/set() for saturating updates. Many modules drop local as_ctx_ptr helpers and import bun_ptr::AsCtxPtr; Interpreter tweaks raw-pointer construction.

Changes

WebSocket Cell connection tracking

Layer / File(s) Summary
Handler Cell field and operations
src/runtime/server/WebSocketServerContext.rs
Adds core::cell::Cell import; Handler.active_connections is now Cell<usize>; active_connections_saturating_add/_sub use Cell::get()/saturating_add/saturating_sub and Cell::set(); Handler::from_js initializes with Cell::new(0).
Reader integration
src/runtime/server/mod.rs
active_sockets_count() updated to read the handler counter via the new representation and convert to u32 for the return value.

AsCtxPtr trait migration and pointer adjustments

Layer / File(s) Summary
Trait import and inherent-method removals
src/runtime/api/JSTranspiler.rs, src/runtime/api/bun/Terminal.rs, src/runtime/api/bun/h2_frame_parser.rs, src/runtime/api/bun/subprocess.rs, src/runtime/api/cron.rs, src/runtime/dns_jsc/dns.rs, src/runtime/socket/socket_body.rs
These modules add use bun_ptr::AsCtxPtr; and remove local as_ctx_ptr helper methods so as_ctx_ptr() is provided by the AsCtxPtr trait; docs/comments updated where relevant.
Stat/FS watcher adjustments
src/runtime/node/node_fs_stat_watcher.rs, src/runtime/node/node_fs_watcher.rs
StatWatcher and FSWatcher drop file-local as_ctx_ptr helpers and rely on the trait; StatWatcher keeps ctx_el_ctx() helper for EventLoopCtx derivation.
Interpreter pointer construction tweak
src/runtime/shell/interpreter.rs
Interpreter::as_ctx_ptr now constructs the mutable raw pointer using core::ptr::from_ref::<Self>(self).cast_mut() instead of casting from *const Self.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(runtime): harden EXP-041 shared-provenance cluster' clearly and specifically describes the main change—addressing a UB finding (EXP-041) by hardening shared-provenance context-pointer handling and fixing mutable-counter issues. It is concise and directly related to the changeset.
Description check ✅ Passed The PR description covers both required sections: it explains what changed (active_connections refactored, as_ctx_ptr helpers consolidated, Interpreter::as_ctx_ptr updated) and how it was verified (rustfmt, cargo check, rust:check-all, smoke tests, websocket test, diff checks, targeted searches). Context and scope are also clearly documented.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

This should be Cell<usize> instead of AtomicUsize, and the comment about using Cell should be deleted.

Replace the WebSocket active_connections shared-reference write with AtomicUsize and centralize the sibling context-pointer helper pattern identified by the UB audit.

The counter uses relaxed saturating updates because the value is only a count; no synchronization contract is attached to it.

Most sibling sites now use the existing bun_ptr::AsCtxPtr helper instead of repeating hand-written self-as-const-self casts. Interpreter keeps an inherent as_ctx_ptr intentionally so Box<Interpreter> callers still produce *mut Interpreter instead of *mut Box<Interpreter>.

Refs oven-sh#30903.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Dicklesworthstone Dicklesworthstone force-pushed the claude/ub-fixes-demo-2 branch from 94707bb to 3b592ec Compare May 18, 2026 20:28
@Dicklesworthstone Dicklesworthstone changed the title fix(server): atomic-counter WebSocket active_connections (audit R-EXP-041) fix(runtime): harden EXP-041 shared-provenance cluster May 18, 2026
…view

Per Jarred-Sumner's review on oven-sh#30972: replace AtomicUsize with Cell<usize>
for WebSocket Handler::active_connections. The Handler struct is already
!Send/!Sync (holds *mut c_void, JSValue, BackRef), and the counter is only
touched on the JS thread, so Cell is the right primitive — no synchronization
contract was attached to the original counter.

Drops the read-modify-write fetch_update calls in favor of get + set,
and the load(Relaxed) in active_sockets_count for plain get.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Dicklesworthstone
Copy link
Copy Markdown
Author

Thanks for the review @Jarred-Sumner — addressed in 7ff8ba2.

  • pub active_connections: AtomicUsizeCell<usize> in src/runtime/server/WebSocketServerContext.rs
  • Dropped the fetch_update RMW pair in favor of .set(self.active_connections.get().saturating_add/sub(n))
  • Removed the TODO(port) comment about converting to Cell (it's no longer aspirational)
  • Updated the one consumer in src/runtime/server/mod.rs::active_sockets_count from .load(Ordering::Relaxed) to .get()

Handler was already !Send + !Sync (it holds *mut c_void, raw JSValues, and BackRefs), so dropping the atomics doesn't loosen anything — and the counter is only touched on the JS thread per your earlier note.

cargo check --workspace clean.

@Dicklesworthstone
Copy link
Copy Markdown
Author

@Jarred-Sumner quick follow-up on your requested change here: this branch now uses Cell<usize> for active_connections, removes the stale Cell-related comment, and updates the reader to .get().

Verified with cargo check --workspace.

Does this match the shape you wanted, or would you prefer any further adjustment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants