fix(runtime): a SharedArrayBuffer carries a real GcHeader — user bytes decided another SAB's type, and a brand check segfaulted (fixes #10925)

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.

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<Mutex<HashSet<usize>>> = OnceLock::new();
@@ -45,11 +46,21 @@ fn registry() -> &'static Mutex<HashSet<usize>> {
     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::<BufferHeader>() + size as usize;
+    let total = GC_HEADER_SIZE + std::mem::size_of::<BufferHeader>() + 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<HashSet<usize>> {
     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

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"
+    );
+}