diff --git a/src/jsc/ConcurrentPromiseTask.rs b/src/jsc/ConcurrentPromiseTask.rs index 8fab41a8442f..bd40d12e44d4 100644 --- a/src/jsc/ConcurrentPromiseTask.rs +++ b/src/jsc/ConcurrentPromiseTask.rs @@ -48,10 +48,22 @@ pub struct ConcurrentPromiseTask<'a, Context: ConcurrentPromiseTaskContext> { bun_threading::intrusive_work_task!(['a, Context: ConcurrentPromiseTaskContext] ConcurrentPromiseTask<'a, Context>, task); -// SAFETY: `ConcurrentPromiseTask` is heap-allocated and only its address crosses -// threads via the intrusive `task` node and the concurrent queue. All access to -// `ctx` / `promise` / `global_this` is sequenced by the work-pool → on_finish → -// run_from_js hand-off; raw pointers are inert. +// SAFETY: `ConcurrentPromiseTask` is heap-allocated and only its *address* crosses +// threads, via the intrusive `task` node and the concurrent queue; it is never +// aliased on two threads at once. The hand-off is strictly sequenced: +// `schedule` (JS thread) → `run_from_thread_pool` (one worker, exclusive) → +// `on_finish` re-enqueues onto the JS event loop → `run_from_js` (JS thread). +// +// `Context` is deliberately NOT required to be `Send`: its implementors hold +// JS-thread-bound state — a `&JSGlobalObject` (opaque `!Sync` FFI handle) and +// raw pointers into the JS heap — that `then()` touches back on the JS thread. +// The only method the worker runs is `ctx.run()`, which operates on the +// thread-safe subset; any value that genuinely needs to cross to the worker is +// wrapped at the field level in `bun_jsc::ThreadSafe` (e.g. `TransformTask::input_code`), +// which protects the JS value and unprotects on drop. Requiring `C: Send` would +// reject this correct-by-construction design, so the invariant is discharged here +// rather than by an auto-trait bound — the same single-JS-thread contract that +// backs `JsCell` and `VirtualMachine`'s hand-written `unsafe impl`s. unsafe impl Send for ConcurrentPromiseTask<'_, C> {} impl Taskable for ConcurrentPromiseTask<'_, Context> { diff --git a/src/jsc/JSCell.rs b/src/jsc/JSCell.rs index 31347eb4070d..6e16bd02cbc0 100644 --- a/src/jsc/JSCell.rs +++ b/src/jsc/JSCell.rs @@ -119,11 +119,21 @@ pub struct JsCell(core::cell::UnsafeCell); // SAFETY: see type-level docs — `JsCell` is only dereferenced on the owning // JS thread; the `Sync` impl exists so `&'static VirtualMachine` (which // contains `JsCell` fields) satisfies `'static`-bound trait objects and -// `thread_local!` accessors without `T: Sync` cascading everywhere. It is NOT -// a license for cross-thread `get_mut()`. -unsafe impl Sync for JsCell {} -// SAFETY: same single-thread-owner invariant as `Sync` above. -unsafe impl Send for JsCell {} +// `thread_local!` accessors without hand-written `unsafe impl`s cascading +// everywhere. It is NOT a license for cross-thread `get_mut()`. +// +// The `T: Sync` / `T: Send` bounds are load-bearing: `get()` is a *safe* fn +// that hands out `&T`, so an unconditional `Sync` would let a `&JsCell` +// cross threads (via any `Sync` container) and surface a `&T` to a `!Sync` +// `T` (`Rc`, `Cell`, `RefCell`) on another thread — UB with zero `unsafe` at +// the call site. Gating on the inner bound keeps the thread-affinity escape +// hatch while making `JsCell`/`JsCell` correctly `!Sync`/`!Send`, +// so such a field can never silently make its container cross-thread-shareable. +unsafe impl Sync for JsCell {} +// SAFETY: same single-thread-owner invariant as `Sync` above; `T: Send` +// because `into_inner()` moves the owned `T` out across whatever thread holds +// the cell. +unsafe impl Send for JsCell {} impl JsCell { #[inline(always)] @@ -225,3 +235,40 @@ impl core::fmt::Debug for JsCell { self.get().fmt(f) } } + +// Compile-time proof that the `Send`/`Sync` bounds on `JsCell` are honored. +// +// The safe `get()` makes unconditional `Send`/`Sync` unsound (see the SAFETY +// note on the impls): a `&JsCell` must not be shareable across threads. +// Stable Rust has no negative bounds, so the `!Send`/`!Sync` direction uses the +// auto-trait-ambiguity trick — if `JsCell>` ever regains `Send`/`Sync`, +// both blanket impls below apply and the anonymous `const` fails to compile +// with "conflicting implementations" (same shape as `subproc.rs`'s +// `__pipe_reader_thread_confined`). +mod jscell_send_sync_proof { + use super::JsCell; + + // `JsCell` stays `Send`/`Sync` when `T` is — the escape hatch still works. + const _: fn() = || { + fn assert_send() {} + fn assert_sync() {} + assert_send::>(); + assert_sync::>(); + }; + + // `JsCell` is NOT `Send`/`Sync` when `T` is not (`Rc` is `!Send + !Sync`). + trait NotSend { + const OK: () = (); + } + impl NotSend<()> for T {} + impl NotSend for T {} + + trait NotSync { + const OK: () = (); + } + impl NotSync<()> for T {} + impl NotSync for T {} + + const _: () = > as NotSend<_>>::OK; + const _: () = > as NotSync<_>>::OK; +} diff --git a/src/jsc/WorkTask.rs b/src/jsc/WorkTask.rs index 9d3541f7ed3e..d385a5d6a450 100644 --- a/src/jsc/WorkTask.rs +++ b/src/jsc/WorkTask.rs @@ -51,10 +51,20 @@ pub struct WorkTask { bun_threading::intrusive_work_task!([Context: WorkTaskContext] WorkTask, task); -// SAFETY: `WorkTask` is moved into the thread pool's queue (intrusive `task` -// node) and back via the concurrent task queue. All access to `ctx` / -// `global_this` is sequenced by the work-pool → on_finish → run_from_js -// hand-off; raw pointers are inert. +// SAFETY: `WorkTask` is heap-allocated; only its *address* crosses threads, via +// the intrusive `task` node and the concurrent queue, and it is never aliased on +// two threads at once. The hand-off is strictly sequenced: `schedule` (JS thread) +// → `run_from_thread_pool` (one worker, exclusive) → `on_finish` re-enqueues onto +// the JS event loop → `run_from_js` (JS thread). +// +// `Context` is deliberately NOT required to be `Send`: `ctx` is a raw +// `*mut Context` and its implementors (e.g. `GetAddrInfoRequest`) hold +// JS-thread-bound raw pointers (`*mut DNSLookup`, `*mut Resolver`) that `then()` +// uses back on the JS thread. The worker only runs `Context::run`, which touches +// the thread-safe subset; the raw pointer is inert while the address is in flight. +// Requiring `C: Send` would reject this correct-by-construction design — the same +// single-JS-thread contract that backs `JsCell` and `VirtualMachine`'s +// hand-written `unsafe impl`s. unsafe impl Send for WorkTask {} impl Taskable for WorkTask { diff --git a/test/internal/jscell-send-sync-bounds.test.ts b/test/internal/jscell-send-sync-bounds.test.ts new file mode 100644 index 000000000000..580f505db5c8 --- /dev/null +++ b/test/internal/jscell-send-sync-bounds.test.ts @@ -0,0 +1,132 @@ +// `JsCell` (src/jsc/JSCell.rs) is a `#[repr(transparent)]` wrapper over +// `UnsafeCell` whose `get(&self) -> &T` is a SAFE function. Its `Send`/`Sync` +// impls must be gated on the inner type: +// +// unsafe impl Sync for JsCell {} +// unsafe impl Send for JsCell {} +// +// If they were unconditional (`unsafe impl ...`), a `&JsCell` would be +// shareable across threads via any `Sync` container, and the safe `get()` would +// then hand out a `&T` to a `!Sync` `T` (`Rc`, `Cell`, `RefCell`) on another +// thread — UB with zero `unsafe` at the call site (oven-sh/bun#31498). +// +// This is a type-level invariant with no runtime surface, so the compiler checks +// it: a throwaway crate depends on the real `bun_jsc` and asserts `JsCell>` +// is `!Send + !Sync` via the auto-trait-ambiguity trick (stable Rust has no +// negative bounds). `cargo check` succeeds iff the bounds are present — with the +// unconditional impls `JsCell>` is `Send + Sync`, both blanket impls apply, +// and the build fails with E0283. `cargo check` does not link, so no C++/JSC objects +// are needed; it only needs the codegen the debug build already emitted. +import { expect, test } from "bun:test"; +import { tempDir } from "harness"; +import { existsSync, readFileSync } from "node:fs"; +import path from "node:path"; + +const cargo = Bun.which("cargo"); +const repoRoot = path.resolve(import.meta.dir, "..", ".."); +const jscCrate = path.join(repoRoot, "src", "jsc"); + +// `bun_jsc`'s build.rs pulls generated Rust from here; emitted by `bun bd`. +const codegenReady = + existsSync(path.join(repoRoot, "build", "debug", "codegen", "cpp.rs")) && + existsSync(path.join(repoRoot, "build", "debug", "codegen", "generated_resolved_source_tag.rs")); + +// Pin the same toolchain `bun bd` uses (the temp crate lives outside the repo, so +// it wouldn't otherwise pick up rust-toolchain.toml). +function pinnedToolchain(): string | undefined { + try { + const toml = readFileSync(path.join(repoRoot, "rust-toolchain.toml"), "utf8"); + return toml.match(/channel\s*=\s*"([^"]+)"/)?.[1]; + } catch { + return undefined; + } +} +const toolchain = pinnedToolchain(); + +// Host triple, for `--target` so the build.rs codegen path lines up. +function hostTriple(): string | undefined { + if (!cargo) return undefined; + const out = Bun.spawnSync({ cmd: [cargo, "-vV"], env: process.env }).stdout.toString(); + return out.match(/host:\s*(\S+)/)?.[1]; +} +const triple = hostTriple(); + +const proof = ` +use bun_jsc::JsCell; +use std::rc::Rc; + +trait NotSend { const OK: () = (); } +impl NotSend<()> for T {} +impl NotSend for T {} + +trait NotSync { const OK: () = (); } +impl NotSync<()> for T {} +impl NotSync for T {} + +// JsCell> must be !Send and !Sync. If either impl is unconditional the +// type gains the trait, both blanket impls apply, and this fails to compile (E0283). +const _: () = > as NotSend<_>>::OK; +const _: () = > as NotSync<_>>::OK; + +// Positive direction: a Send/Sync inner type keeps the thread-affinity escape hatch. +const _: fn() = || { + fn assert_send() {} + fn assert_sync() {} + assert_send::>(); + assert_sync::>(); +}; + +fn main() {} +`; + +test.skipIf(!cargo || !codegenReady || !toolchain || !triple)( + "JsCell's Send/Sync impls require T: Send/Sync (issue #31498)", + async () => { + // `[workspace]` detaches this crate from the repo's workspace; the absolute + // path dep resolves `bun_jsc` regardless of cwd. The target dir lives inside + // the temp dir so the run is self-contained and leaves the build untouched. + using dir = tempDir("jscell-send-sync", { + "Cargo.toml": `[package] +name = "jscell_send_sync_proof" +version = "0.0.0" +edition = "2021" + +[[bin]] +name = "jscell_send_sync_proof" +path = "main.rs" + +[dependencies] +bun_jsc = { path = ${JSON.stringify(jscCrate)} } + +[workspace] +`, + "main.rs": proof, + }); + + await using proc = Bun.spawn({ + // --offline: every dep is already in the cargo cache from `bun bd`; never + // touch the network in CI. + cmd: [cargo!, "check", "--quiet", "--offline", "--target", triple!], + cwd: String(dir), + env: { + ...process.env, + RUSTUP_TOOLCHAIN: toolchain!, + CARGO_TARGET_DIR: path.join(String(dir), "target"), + CARGO_TERM_COLOR: "never", + }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + // `cargo check` must succeed — the negative assertions compile only when + // `JsCell>` is correctly `!Send + !Sync`. E0283 is the signature of + // the unconditional impls (both ambiguity blankets apply). + const output = stdout + stderr; + expect(output).not.toContain("E0283"); + expect(output).not.toMatch(/^error/m); + expect(exitCode).toBe(0); + }, + 300_000, +);