diff --git a/changelog.d/10925-sab-gc-header.md b/changelog.d/10925-sab-gc-header.md new file mode 100644 index 0000000000..5091242650 --- /dev/null +++ b/changelog.d/10925-sab-gc-header.md @@ -0,0 +1,19 @@ +`SharedArrayBuffer` no longer lets one buffer's contents decide another +buffer's type. A SAB was handed to JavaScript as the address of a block with no +GC header in front of it, and several runtime paths read the eight bytes before +a value as its header. Those bytes are usually the tail of the previous SAB's +data, which a program can write through an ordinary `Uint8Array`. So writing a +byte into one SAB's own memory could make `Array.isArray` answer `true` for a +different SAB, and could make a `Map.prototype.get.call(sab, …)` brand check +follow fabricated pointers and crash. The answers also depended on how the +binary happened to be linked. + +A SAB's backing now carries a real GC header, so every one of those reads gets +the honest kind and takes the ordinary buffer path. `Array.isArray(sab)` is +`false`, a collection method called on a SAB throws the `TypeError` node throws, +and none of it depends on neighbouring memory. + +Sharing is unchanged: the backing is still one process-global, never-freed +allocation, two views over a SAB still alias the same bytes, a worker still sees +writes through a captured or module-level SAB, and `Atomics.wait` / `notify` +still rendezvous across agents on the same physical address. diff --git a/crates/perry-runtime/src/shared_sab.rs b/crates/perry-runtime/src/shared_sab.rs index d6e7292a73..8363b2a604 100644 --- a/crates/perry-runtime/src/shared_sab.rs +++ b/crates/perry-runtime/src/shared_sab.rs @@ -26,6 +26,7 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Mutex, OnceLock}; use crate::buffer::BufferHeader; +use crate::gc::{GcHeader, GC_FLAG_PINNED, GC_FLAG_TENURED, GC_HEADER_SIZE, GC_TYPE_BUFFER}; /// Set of `BufferHeader` addresses that back a `SharedArrayBuffer`. static SHARED_SAB_REGISTRY: OnceLock>> = OnceLock::new(); @@ -45,11 +46,21 @@ fn registry() -> &'static Mutex> { SHARED_SAB_REGISTRY.get_or_init(|| Mutex::new(HashSet::new())) } -/// Header + data layout for a SAB of `size` data bytes. 8-byte alignment so the -/// data region (which begins immediately after the 8-byte `BufferHeader`) is -/// itself 8-aligned — required for `BigInt64Array` / `Float64` atomic slots. +/// Layout for a SAB of `size` data bytes: +/// `[GcHeader:8][BufferHeader:8][data:size]`, 8-byte aligned. +/// +/// #340/#341 / #10925: the leading `GcHeader` is what makes a SAB an honest +/// pointer. Before it, the JS value (the `BufferHeader` address) had no header, +/// and every `*(addr - 8)` type probe read whatever `alloc_zeroed` block sat in +/// front of it — the tail of another SAB's user-writable data — so writing a +/// SAB's own bytes could flip `Array.isArray` on another and crash a brand +/// check (#10925). With the header, `BufferHeader` and the data region keep +/// their exact offsets (the returned pointer still points at the `BufferHeader`, +/// so `buffer_data` == `buf + 8` is unchanged), and `buf - 8` is a real +/// `GC_TYPE_BUFFER` header. 8-byte alignment keeps the data region 8-aligned for +/// `BigInt64Array` / `Float64` atomic slots. fn sab_layout(size: u32) -> Layout { - let total = std::mem::size_of::() + size as usize; + let total = GC_HEADER_SIZE + std::mem::size_of::() + size as usize; Layout::from_size_align(total, 8).expect("shared SAB layout") } @@ -72,9 +83,29 @@ pub fn alloc_shared_sab(size: u32) -> *mut BufferHeader { if raw.is_null() { handle_alloc_error(layout); } - let buf = raw as *mut BufferHeader; - // SAFETY: `buf` points at a fresh `BufferHeader`-sized-and-aligned block. + // The `GcHeader` sits at `raw`; the JS-visible value is the `BufferHeader` + // one header down, so `buf - GC_HEADER_SIZE` reads back this header. + let buf = unsafe { raw.add(GC_HEADER_SIZE) } as *mut BufferHeader; + let total = layout.size(); + // SAFETY: `raw` owns `total` zeroed, 8-aligned bytes; the header and the + // BufferHeader both fit within the first `GC_HEADER_SIZE + 8` of them. unsafe { + let header = raw as *mut GcHeader; + (*header).obj_type = GC_TYPE_BUFFER; + // PINNED + TENURED and NOT `GC_FLAG_ARENA`: this block is a raw, + // process-global `alloc_zeroed`, not an arena or a gc_malloc cell. The + // collector recognises a SAB by process-global registry membership + // (`is_shared_sab`), never by this header, and — proven by the + // header-write audit in the PR — no collector path (mark, scavenge, + // sweep, remembered-set) reaches an object outside its own thread's + // arena/tracked set, so this header is only ever READ by the collector, + // never written. It carries the honest kind for the mutator-side + // `*(addr - 8)` probes (`Array.isArray`, the collection-thunk brand, + // `JSON.stringify`), which is what #10925 needed. + (*header).gc_flags = GC_FLAG_PINNED | GC_FLAG_TENURED; + (*header)._reserved = 0; + // Total block size, for honesty; a non-arena object is never block-walked. + (*header).size = total.min(u32::MAX as usize) as u32; (*buf).length = size; (*buf).capacity = size; } @@ -128,6 +159,81 @@ pub(crate) fn snapshot_shared_sabs() -> Option> { registry().lock().ok().map(|r| r.clone()) } +#[cfg(test)] +mod header_survival_tests { + use super::*; + + /// #10925, the precondition for putting a `GcHeader` in front of + /// process-global memory: **no collector may WRITE it.** Two threads' + /// collectors setting a mark or forwarding bit on one header would be a + /// data race that shows up as rare corruption rather than a clean failure. + /// + /// The argument is the source audit (plan L15.7): every mark, scavenge, + /// sweep and remembered-set write gates on THIS thread's arena or + /// malloc-tracked membership — a set a process-global SAB is in on no + /// thread — and the moving paths classify by arena range before they read + /// a header at all. This test is the empirical backstop for that argument, + /// not a proof of it: it snapshots the header word, drives several minor + /// and major collections on this thread AND on two others while all three + /// hold the SAB, and requires the word to come back unchanged. + /// + /// It can fail: point `alloc_shared_sab` at the arena, or drop the + /// membership gate in front of any mark write, and the mark bit lands in + /// this word. + #[test] + fn no_collector_writes_a_shared_sab_header() { + let buf = alloc_shared_sab(64); + let header_addr = (buf as usize) - GC_HEADER_SIZE; + // Read as one 64-bit word: obj_type, gc_flags, _reserved and size + // together, so a write to ANY of them is caught. + let snapshot = unsafe { std::ptr::read_volatile(header_addr as *const u64) }; + + // The header must actually say what the fix intends, or "unchanged" + // would be vacuous. + let header = header_addr as *const GcHeader; + assert_eq!(unsafe { (*header).obj_type }, GC_TYPE_BUFFER); + assert_eq!( + unsafe { (*header).gc_flags }, + GC_FLAG_PINNED | GC_FLAG_TENURED + ); + + fn churn() { + for _ in 0..8 { + for _ in 0..2000 { + let o = crate::object::js_object_alloc(0, 0); + std::hint::black_box(o); + } + crate::gc::js_gc_collect(); + } + } + + let workers: Vec<_> = (0..2) + .map(|_| { + let addr = buf as usize; + std::thread::spawn(move || { + // Touch the shared bytes the way an Atomics user would, + // so the SAB is live across this thread's collections. + let data = unsafe { crate::buffer::buffer_data(addr as *const BufferHeader) }; + for i in 0..64u8 { + unsafe { std::ptr::write_volatile((data as *mut u8).add(i as usize), i) }; + } + churn(); + }) + }) + .collect(); + churn(); + for w in workers { + w.join().expect("a collecting thread panicked"); + } + + let after = unsafe { std::ptr::read_volatile(header_addr as *const u64) }; + assert_eq!( + after, snapshot, + "a collector wrote the process-global SAB header: {snapshot:#018x} -> {after:#018x}" + ); + } +} + /// Test-only: pretend `addr` is a process-global SAB backing. /// /// A real backing has no `GcHeader`, so the GC's dead-buffer scan can only diff --git a/crates/perry/tests/sab_header_read.rs b/crates/perry/tests/sab_header_read.rs new file mode 100644 index 0000000000..3bcd1a8b56 --- /dev/null +++ b/crates/perry/tests/sab_header_read.rs @@ -0,0 +1,152 @@ +//! #10925 -- a `SharedArrayBuffer` must not have its kind decided by the bytes +//! that happen to sit in front of it. +//! +//! A SAB was handed to JS as the address of a header-less `alloc_zeroed` +//! block, and several paths read `addr - 8` as a `GcHeader` for it. The bytes +//! there are, in the allocator layout observed on Linux x86_64, the tail of +//! the PREVIOUS SAB's data -- user-writable through an ordinary typed-array +//! view. So writing a byte into one SAB's own memory changed `Array.isArray` +//! on another, and made `Map.prototype.get.call` on it dereference fabricated +//! pointers (SIGSEGV). A type confusion driven by user bytes. +//! +//! MUST-FAIL: committed BEFORE the fix. On the unfixed runtime the first test +//! prints `true` and the second segfaults (the harness reports the signal); +//! the expected strings are node 26.8.1's. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run(dir: &std::path::Path, source: &str) -> String { + let entry = dir.join("main.ts"); + let output = dir.join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + let compile = Command::new(perry_bin()) + .current_dir(dir) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output) + .current_dir(dir) + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed (a signal here is #10925's segfault)\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).into_owned() +} + +/// Wrong value, no crash: kind byte 1 (`GC_TYPE_ARRAY`) planted in the tail of +/// the first SAB made the second answer `Array.isArray(b) === true`. +#[test] +fn a_byte_written_into_one_sab_does_not_change_another_sabs_kind() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + r#" +const sabs: any[] = []; +for (let i = 0; i < 8; i++) sabs.push(new SharedArrayBuffer(24)); +for (const s of sabs) new Uint8Array(s)[16] = 1; +console.log("isArray", sabs.map((s) => Array.isArray(s)).join(",")); +console.log("brand", Object.prototype.toString.call(sabs[3])); +console.log("json", JSON.stringify(sabs[3])); +"#, + ); + assert_eq!( + stdout, + "isArray false,false,false,false,false,false,false,false\n\ + brand [object SharedArrayBuffer]\n\ + json {}\n" + ); +} + +/// The segfault: kind byte 7 (`GC_TYPE_ERROR`) routed a collection thunk's +/// incompatible-receiver message into `js_error_get_name` on fabricated +/// pointers. node throws a `TypeError` for every one of these receivers. +#[test] +fn a_collection_brand_check_on_a_sab_throws_instead_of_crashing() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + r#" +const sabs: any[] = []; +for (let i = 0; i < 8; i++) sabs.push(new SharedArrayBuffer(24)); +for (const s of sabs) { const u = new Uint8Array(s); u[16] = 7; u[20] = 64; } +let threw = 0; +for (const s of sabs) { + try { Map.prototype.get.call(s, 1); } catch (e: any) { if (e instanceof TypeError) threw++; } +} +console.log("threw", threw); +console.log("keys", sabs.map((s) => Object.keys(s).length).join(",")); +"#, + ); + // Deliberately NOT asserting `String(sab)`: it returns the buffer bytes + // (not `[object SharedArrayBuffer]`) for a plain `new ArrayBuffer(n)` too, + // so that divergence is not a header read and would keep this test red + // after the fix for a reason it does not name. Tracked separately. + assert_eq!( + stdout, + "threw 8\n\ + keys 0,0,0,0,0,0,0,0\n" + ); +} + +/// The sharing semantics the fix must NOT regress: two views over one SAB see +/// each other's writes, and so does a worker the SAB is handed to -- both by +/// closure capture and as a module-level binding (the escape hatch in +/// `closure_analysis.rs` that reads a top-level SAB in place from a worker). +#[test] +fn sab_bytes_are_shared_across_views_and_threads() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + r#" +import { spawn } from "perry/thread"; +const top = new SharedArrayBuffer(16); +const topView = new Int32Array(top); +topView[0] = 7; +async function main() { + const local = new SharedArrayBuffer(16); + const a = new Int32Array(local); + const b = new Uint8Array(local); + a[0] = 0x01020304; + console.log("views", b[0], b[3]); + const fromCapture = await spawn(() => { + const v = new Int32Array(local); + Atomics.add(v, 1, 5); + return Atomics.load(v, 0); + }); + console.log("capture", fromCapture, Atomics.load(a, 1)); + const fromTop = await spawn(() => { + const v = new Int32Array(top); + Atomics.store(v, 1, 99); + return Atomics.load(v, 0); + }); + console.log("module-level", fromTop, Atomics.load(topView, 1)); +} +main(); +"#, + ); + assert_eq!( + stdout, + "views 4 1\n\ + capture 16909060 5\n\ + module-level 7 99\n" + ); +} diff --git a/crates/perry/tests/sab_header_survives_gc.rs b/crates/perry/tests/sab_header_survives_gc.rs new file mode 100644 index 0000000000..64db4d8e12 --- /dev/null +++ b/crates/perry/tests/sab_header_survives_gc.rs @@ -0,0 +1,107 @@ +//! #10925 smoke test (NOT a proof): the SAB's `GcHeader` must survive heavy +//! multi-thread collection. +//! +//! The source audit (PR body / plan L15.7) shows no collector path WRITES a +//! SAB header — every mark/move/sweep gates on this-thread arena or +//! malloc-tracked membership, which a process-global SAB is in on no thread. +//! This backs that empirically: the main thread and two workers each allocate +//! enough to force several minor and major collections while all three hold +//! the same SAB and run `Atomics` traffic on it. If any collector wrote the +//! header (a mark bit, a stale forward), the bytes would move and a later +//! typed-array read over the SAB would see corruption or the program would +//! crash. A clean, node-matching run across many collections is the signal. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +#[test] +fn a_sab_header_survives_heavy_multithread_collection() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + let output = dir.path().join("main_bin"); + std::fs::write( + &entry, + r#" +import { spawn } from "perry/thread"; +const sab = new SharedArrayBuffer(64); +const cell = new Int32Array(sab); +cell[0] = 0; +function churn(rounds: number): void { + for (let r = 0; r < rounds; r++) { + let junk: any[] = []; + for (let i = 0; i < 20000; i++) junk.push({ a: i, b: [i, i + 1], c: "s" + i }); + junk = []; + Atomics.add(cell, 0, 1); + } +} +async function main() { + const w1 = spawn(() => { + const v = new Int32Array(sab); + for (let r = 0; r < 40; r++) { + let j: any[] = []; + for (let i = 0; i < 20000; i++) j.push({ x: i, y: "" + i }); + j = []; + Atomics.add(v, 1, 1); + } + return Atomics.load(v, 1); + }); + const w2 = spawn(() => { + const v = new Int32Array(sab); + for (let r = 0; r < 40; r++) { + let j: any[] = []; + for (let i = 0; i < 20000; i++) j.push([i, i, i]); + j = []; + Atomics.add(v, 2, 1); + } + return Atomics.load(v, 2); + }); + churn(40); + const a = await w1; + const b = await w2; + // Every worker's and the main thread's Atomics counters landed in the one + // shared buffer, and the buffer is still a SharedArrayBuffer afterwards. + console.log("main", Atomics.load(cell, 0)); + console.log("w1", a, "w2", b); + console.log("shared-w1", Atomics.load(cell, 1), "shared-w2", Atomics.load(cell, 2)); + console.log("brand", Object.prototype.toString.call(sab)); + console.log("isArray", Array.isArray(sab)); + console.log("len", sab.byteLength); +} +main(); +"#, + ) + .unwrap(); + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .output() + .expect("compile"); + assert!( + compile.status.success(), + "compile failed\n{}", + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).current_dir(dir.path()).output().expect("run"); + assert!( + run.status.success(), + "a signal here would be a collector writing the shared SAB header\nstatus {:?}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stderr) + ); + assert_eq!( + String::from_utf8_lossy(&run.stdout), + "main 40\n\ + w1 40 w2 40\n\ + shared-w1 40 shared-w2 40\n\ + brand [object SharedArrayBuffer]\n\ + isArray false\n\ + len 64\n" + ); +}