diff --git a/src/bundler/LinkerContext.rs b/src/bundler/LinkerContext.rs index bbe0441b4548..97e94b5b998b 100644 --- a/src/bundler/LinkerContext.rs +++ b/src/bundler/LinkerContext.rs @@ -586,8 +586,16 @@ impl<'a> LinkerContext<'a> { callback: SourceMapDataTask::run_quoted_source_contents, }, }; - batch.push(ThreadPoolLib::Batch::from(&raw mut line_offset.thread_task)); - second_batch.push(ThreadPoolLib::Batch::from(&raw mut quoted.thread_task)); + let line_offset = std::ptr::from_mut(line_offset); + let quoted = std::ptr::from_mut(quoted); + // SAFETY: field projections of the two live elements; the callbacks + // recover the whole `SourceMapDataTask` from these pointers. + unsafe { + batch.push(ThreadPoolLib::Batch::from( + &raw mut (*line_offset).thread_task, + )); + second_batch.push(ThreadPoolLib::Batch::from(&raw mut (*quoted).thread_task)); + } } // line offsets block sooner and are faster to compute, so we should schedule those first diff --git a/src/bundler/linker_context/generateChunksInParallel.rs b/src/bundler/linker_context/generateChunksInParallel.rs index fd5175c2aba4..c7f05ffe912f 100644 --- a/src/bundler/linker_context/generateChunksInParallel.rs +++ b/src/bundler/linker_context/generateChunksInParallel.rs @@ -98,7 +98,6 @@ pub(crate) fn generate_chunks_in_parallel( debug!(" START {} prepare CSS ast (total count)", total_count); - let mut batch = ThreadPoolLib::Batch::default(); let mut tasks: Vec = Vec::with_capacity(total_count); for chunk in chunks.iter_mut() { if chunk.content.is_css() { @@ -112,12 +111,18 @@ pub(crate) fn generate_chunks_in_parallel( // (raw ptr is invariant); `.cast()` erases the inner `'a` to satisfy it. linker: std::ptr::from_mut::(c).cast(), }); - // Capacity pre-reserved → push never reallocates → ptr stays stable. - let task = tasks.last_mut().unwrap(); - batch.push(ThreadPoolLib::Batch::from(&raw mut task.task)); } } debug_assert_eq!(tasks.len(), total_count); + let tasks_ptr = tasks.as_mut_ptr(); + let mut batch = ThreadPoolLib::Batch::default(); + for i in 0..tasks.len() { + // SAFETY: `i < tasks.len()`; the callback recovers the whole + // `PrepareCssAstTask` from this element pointer. + batch.push(ThreadPoolLib::Batch::from(unsafe { + &raw mut (*tasks_ptr.add(i)).task + })); + } // SAFETY: `parse_graph` is the `BundleV2.graph` backref (valid for // the link step); `pool` is the arena-allocated bundler ThreadPool. let worker_pool = c.worker_pool(); @@ -176,10 +181,7 @@ pub(crate) fn generate_chunks_in_parallel( debug_assert_eq!(chunks.len(), chunk_contexts.len()); debug!(" START {} compiling part ranges", total_count); - // Pre-reserved to `total_count` so pushes never reallocate; the - // batch holds raw pointers into this buffer. let mut combined_part_ranges: Vec = Vec::with_capacity(total_count); - let mut batch = ThreadPoolLib::Batch::default(); for (chunk, chunk_ctx) in chunks.iter_mut().zip(chunk_contexts.iter_mut()) { match &chunk.content { crate::chunk::Content::Javascript(js) => { @@ -200,9 +202,6 @@ pub(crate) fn generate_chunks_in_parallel( bun_ptr::detach_lifetime_ref::(chunk_ctx) }, }); - batch.push(ThreadPoolLib::Batch::from( - &raw mut combined_part_ranges.last_mut().unwrap().task, - )); } } crate::chunk::Content::Css(css) => { @@ -223,9 +222,6 @@ pub(crate) fn generate_chunks_in_parallel( bun_ptr::detach_lifetime_ref::(chunk_ctx) }, }); - batch.push(ThreadPoolLib::Batch::from( - &raw mut combined_part_ranges.last_mut().unwrap().task, - )); } } crate::chunk::Content::Html => { @@ -245,13 +241,19 @@ pub(crate) fn generate_chunks_in_parallel( bun_ptr::detach_lifetime_ref::(chunk_ctx) }, }); - batch.push(ThreadPoolLib::Batch::from( - &raw mut combined_part_ranges.last_mut().unwrap().task, - )); } } } debug_assert_eq!(combined_part_ranges.len(), total_count); + let part_ranges_ptr = combined_part_ranges.as_mut_ptr(); + let mut batch = ThreadPoolLib::Batch::default(); + for i in 0..combined_part_ranges.len() { + // SAFETY: `i < combined_part_ranges.len()`; the callbacks recover + // the whole `PendingPartRange` from this element pointer. + batch.push(ThreadPoolLib::Batch::from(unsafe { + &raw mut (*part_ranges_ptr.add(i)).task + })); + } // SAFETY: `parse_graph` is the `BundleV2.graph` backref (valid for // the link step); `pool` is the arena-allocated bundler ThreadPool. let worker_pool = c.worker_pool(); diff --git a/src/http/AsyncHTTP.rs b/src/http/AsyncHTTP.rs index e46ff76410e9..9fd8605c0be2 100644 --- a/src/http/AsyncHTTP.rs +++ b/src/http/AsyncHTTP.rs @@ -388,7 +388,9 @@ pub fn preconnect(url: URL<'static>, is_url_owned: bool) { )); async_http.client.flags.is_preconnect_only = true; - crate::HTTPThread::schedule(Batch::from(core::ptr::addr_of_mut!(async_http.task))); + let mut batch = Batch::default(); + async_http.schedule(&mut batch); + crate::HTTPThread::schedule(batch); } } @@ -527,7 +529,10 @@ impl<'a> AsyncHTTP<'a> { } pub fn schedule(&mut self, batch: &mut Batch) { - batch.push(Batch::from(core::ptr::addr_of_mut!(self.task))); + let this = core::ptr::from_mut(self); + // SAFETY: field projection of `self`; `HTTPThread::schedule` recovers + // the whole `AsyncHTTP` from this pointer. + batch.push(Batch::from(unsafe { &raw mut (*this).task })); } } diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index 4ad50745d1d0..6ba172afa35a 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -1604,12 +1604,14 @@ pub fn flush_patch_task_queue(this: &mut PackageManager) { while let Some(patch_task) = this.patch_task_fifo.read_item() { // SAFETY: fifo stores live `*mut PatchTask` pushed by // `enqueue_patch_task`; exclusive ownership transferred here. - let pt = unsafe { &mut *patch_task }; - pt.schedule(if matches!(pt.callback, PatchTaskCallback::Apply(_)) { + let is_apply = unsafe { (*patch_task).callback.is_apply() }; + let batch = if is_apply { &mut this.patch_apply_batch } else { &mut this.patch_calc_hash_batch - }); + }; + // SAFETY: as above. + unsafe { PatchTask::schedule(patch_task, batch) }; } } diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index 199bcc6bfd0a..3633ec6899a3 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -166,9 +166,12 @@ impl<'a> Installer<'a> { )); task.result = Result::None; + let task = std::ptr::from_mut(task); + // SAFETY: field projection of the live `tasks[entry_id]` slot; + // `Task::callback` recovers the whole `Task` from this pointer. manager .thread_pool - .schedule(thread_pool::Batch::from(&raw mut task.task)); + .schedule(thread_pool::Batch::from(unsafe { &raw mut (*task).task })); } pub(crate) fn on_package_extracted(&mut self, task_id: crate::package_manager_task::Id) { diff --git a/src/install/patch_install.rs b/src/install/patch_install.rs index 2b5addb0149b..48369234e674 100644 --- a/src/install/patch_install.rs +++ b/src/install/patch_install.rs @@ -707,8 +707,12 @@ impl PatchTask { Some(hasher.final_()) } - pub(crate) fn schedule(&mut self, batch: &mut Batch) { - batch.push(Batch::from(&raw mut self.task)); + /// # Safety + /// `this` must be the live heap `PatchTask` handed out by `enqueue_patch_task`. + pub(crate) unsafe fn schedule(this: *mut Self, batch: &mut Batch) { + // SAFETY: field projection of the live task; `run_from_thread_pool` + // recovers the whole task from this pointer. + batch.push(Batch::from(unsafe { &raw mut (*this).task })); } pub(crate) fn new_calc_patch_hash( diff --git a/src/threading/ThreadPool.rs b/src/threading/ThreadPool.rs index 2b8e5bbe15bc..2809279710fb 100644 --- a/src/threading/ThreadPool.rs +++ b/src/threading/ThreadPool.rs @@ -554,10 +554,11 @@ impl ThreadPool { ctx: bun_ptr::BackRef::new(&wait_context), }); } - // Push to the Vec first (no realloc: capacity reserved), then take - // stable addresses. - for runner_task in tasks.iter_mut() { - batch.push(Batch::from(ptr::addr_of_mut!(runner_task.task))); + let runner_tasks = tasks.as_mut_ptr(); + for i in 0..tasks.len() { + // SAFETY: `i < tasks.len()`; `call` recovers the whole `RunnerTask` + // from this element pointer. + batch.push(Batch::from(unsafe { &raw mut (*runner_tasks.add(i)).task })); } self.schedule(batch); self.wait_for_all(); diff --git a/test/internal/source-lints/thread-pool-batch-projection.test.ts b/test/internal/source-lints/thread-pool-batch-projection.test.ts new file mode 100644 index 000000000000..636ef9047ed8 --- /dev/null +++ b/test/internal/source-lints/thread-pool-batch-projection.test.ts @@ -0,0 +1,250 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// The `*mut Task` a `Batch::from(..)` hands to the thread pool must be +// projected out of a raw pointer to the object that embeds the task, not out +// of a reference to that object: +// +// Batch::from(&raw mut self.task) // `self: &mut Self` +// Batch::from(&raw mut task.task) // `task: &mut T` (a slot, an iter_mut item) +// Batch::from(addr_of_mut!(runner_task.task)) +// Batch::from(&raw mut combined_part_ranges.last_mut().unwrap().task) +// +// are banned; the accepted spellings project through a raw pointer: +// +// Batch::from(&raw mut (*this).task) // `this: *mut T`, or `ptr::from_mut(t)` +// Batch::from(&raw mut (*tasks.as_mut_ptr().add(i)).task) // a Vec of tasks, filled first +// Batch::from(addr_of_mut!((*task).task)) +// +// The pool calls back with exactly the pointer it was given, and every +// callback behind a `Batch::from` site (`from_task_ptr`, `from_field_ptr!`) +// container-ofs it back to the embedding object and then reads and writes the +// object's other fields through the result; `HTTPThread::schedule` does the +// container-of on the calling thread before the batch is even queued. +// `bun_core::container_of` documents that this needs a pointer derived from the +// object's pointer, and a raw borrow of a field taken through a reference does +// not give one: rustc retags `&raw mut ` unless the place is based on a +// raw pointer, and under Stacked Borrows that retag covers the field only, so +// the callback's first sibling-field access is UB ("created by a +// SharedReadWrite retag at offsets []"). The push-then- +// `last_mut()` loop is worse: each `last_mut()` reborrows the whole buffer and +// invalidates the pointers already in the batch, so `Batch::push` itself trips +// when it links the next task onto the previous one. `&raw mut (*p).f` with +// `p` raw is not retagged and keeps `p`'s range; `ptr::from_mut(t)` is a +// reborrow of the whole object, so projecting through it is equivalent. Tree +// Borrows, which is what `bun run rust:miri` runs, accepts the banned shapes, +// and none of the crates involved are in its crate set, so this lint is what +// reports a regression. The sites this was written for (install's +// `PatchTask::schedule` and isolated-install `start_task`, +// `AsyncHTTP::schedule` and `preconnect`, `ThreadPool::each`, the bundler's +// source-map and chunk batches) are the templates for the accepted shapes. +// +// Scope: the argument of every `Batch::from(` call (however the type is +// reached, including the `PoolBatch` / `ThreadPoolBatch` aliases), when it is +// `&raw mut` / `&raw const` / `addr_of_mut!` / `addr_of!` of a field path whose +// base is a binding rather than a `(*p)` deref. The `(*p)` spelling is taken +// to mean `p` is a raw pointer, which is the only reason to write it (a +// reference auto-derefs, and clippy's `explicit_auto_deref` rejects `(*r).f`), +// so a `(*r).f` through a reference would not be reported. A local holding a +// pointer projected elsewhere is out of scope, as is whether the pointer a +// function was handed is itself whole-object (that is its caller's business). The other +// ways of narrowing the same argument, a `&mut` formed in the argument, an +// accessor returning `&mut Task`, `ptr::from_mut(..)` of either, are banned by +// the sibling lint from #37865 and deliberately not repeated here, so the two +// never share an allowlist; `WorkPool::schedule(..)`, the other hand-over, is +// the subject of #37768's lint. Siblings: self-receiver-reclaim.test.ts, +// fn-long-mut-reborrow.test.ts, frozen-nonnull-reborrow.test.ts. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const rustSources = globAllSources().rust.filter(p => p.endsWith(".rs")); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). Same guard as +// dead-code-escapes.test.ts. +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +// `Batch::from(`, however the type is reached: `thread_pool::Batch::from(`, +// `ThreadPoolLib::Batch::from(`, and the `use .. Batch as PoolBatch` / +// `ThreadPoolBatch` aliases in the tree. +const BATCH_FROM = /\b\w*Batch::from\s*\(/g; + +// A place rooted at a binding and ending in a field: `self.task`, +// `task.task`, `self.inner.task`, `v.last_mut().unwrap().task`, +// `self.tasks[i].task`. A place rooted at a raw pointer is spelled `(*p).f` +// (or `(*p.add(i)).f`) and starts with `(`, so it never matches; neither does +// a bare deref such as `*self.task_ptr`, which is a pointer the object holds +// rather than a projection through the object. +const FIELD_PLACE = String.raw`\w+(?:\.\w+(?:\([^()]*\))?|\[[^\[\]]*\])*\.\w+`; + +const RAW_BORROW = new RegExp(String.raw`^&\s*raw\s+(?:mut|const)\s+${FIELD_PLACE}$`); +const ADDR_OF = new RegExp(String.raw`^(?:[\w:]+::)?addr_of(?:_mut)?!\s*\(\s*${FIELD_PLACE}\s*\)$`); + +/** The argument text of the call whose opening paren is at `open`, or null if unbalanced. */ +function argumentAt(source: string, open: number): string | null { + for (let depth = 0, i = open; i < source.length; i++) { + const c = source[i]; + if (c === "(" || c === "{" || c === "[") depth++; + else if (c === ")" || c === "}" || c === "]") { + if (--depth === 0) return source.slice(open + 1, i); + } + } + return null; +} + +function normalize(argument: string): string { + let arg = argument.replace(/\s+/g, " ").trim().replace(/,$/, "").trim(); + const unsafeBlock = /^unsafe\s*\{(.*)\}$/.exec(arg); + if (unsafeBlock) arg = unsafeBlock[1].trim(); + return arg; +} + +function isBanned(argument: string): boolean { + const arg = normalize(argument); + return RAW_BORROW.test(arg) || ADDR_OF.test(arg); +} + +/** Byte offsets (into `stripped`) of every banned `Batch::from` call in one file. */ +function findBannedBatches(stripped: string): number[] { + const hits: number[] = []; + for (const m of stripped.matchAll(BATCH_FROM)) { + const argument = argumentAt(stripped, m.index + m[0].length - 1); + if (argument !== null && isBanned(argument)) hits.push(m.index); + } + return hits; +} + +function lineOf(text: string, offset: number): number { + return text.slice(0, offset).split("\n").length; +} + +// Documented, ratcheted exceptions: files allowed to keep exactly N of the +// shape. Prefer converting over adding an entry here. +const ALLOW: Record = {}; + +const counts: Record = {}; +const offenders: string[] = []; +let scanned = 0; +let batchSites = 0; +for (const abs of rustSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + // `src/cli` is a symlink into `src/runtime/cli`; count each file once under + // its canonical path. + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + const content = await file(abs).text(); + // Strip full-line comments so prose mentions (including the in-tree comments + // describing this hazard) don't count. `[ \t]*`, not `\s*`: `\s` crosses + // newlines and would swallow blank lines, shifting the reported line numbers. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + batchSites += [...stripped.matchAll(BATCH_FROM)].length; + for (const offset of findBannedBatches(stripped)) { + counts[source] = (counts[source] ?? 0) + 1; + if (counts[source] > (ALLOW[source] ?? 0)) { + offenders.push(`${source}:${lineOf(stripped, offset)}`); + } + } +} + +test("scans a non-empty set of tracked Rust sources containing Batch::from calls", () => { + // Guards against the tracked/realpath filters (or the call pattern) failing + // to match anything, which would make the ban below pass vacuously. + expect(scanned).toBeGreaterThan(0); + expect(batchSites).toBeGreaterThan(0); +}); + +test("the classifier recognizes the spellings it claims to", () => { + const banned = [ + // The instances this was written for, as they were. + "&raw mut self.task", + "core::ptr::addr_of_mut!(self.task)", + "core::ptr::addr_of_mut!(async_http.task)", + "&raw mut task.task", + "ptr::addr_of_mut!(runner_task.task)", + "&raw mut line_offset.thread_task", + "\n &raw mut combined_part_ranges.last_mut().unwrap().task,\n ", + // Other spellings of the same projection. + "&raw const self.task", + "addr_of_mut!(this.task)", + "std::ptr::addr_of!(self.task)", + "&raw mut self.inner.task", + "&raw mut self.tasks[i].task", + "&raw mut tasks[i].task", + "&raw mut self.tasks.last_mut().unwrap().task", + "unsafe { &raw mut this.task }", + "unsafe {\n core::ptr::addr_of_mut!(self.task)\n}", + "core::ptr::addr_of_mut!(\n task.task\n)", + ]; + const allowed = [ + // The converted shapes. + "unsafe { &raw mut (*this).task }", + "unsafe { &raw mut (*task).task }", + "\n &raw mut (*line_offset).thread_task,\n ", + "unsafe { &raw mut (*runner_tasks.add(i)).task }", + "unsafe {\n &raw mut (*tasks_ptr.add(i)).task\n }", + // The raw projections that were already in the tree. + "&raw mut (*parse_task).task", + "&raw mut (*parse_task).io_task", + "unsafe { core::ptr::addr_of_mut!((*task).task) }", + "unsafe {\n core::ptr::addr_of_mut!((*task).task)\n }", + "core::ptr::addr_of_mut!(\n (*this.parse_task.as_ptr()).task\n )", + "core::ptr::addr_of_mut!(\n (*this).drain_task\n )", + "unsafe { TaskType::field_of(task) }", + // A pointer projected elsewhere, or one the object holds, is out of scope. + "task", + "queued", + "&raw mut *self.task", + // Narrowed through a reference or an accessor rather than a raw borrow: + // #37865's lint, not this one. + "&mut self.task", + "this.task()", + "unsafe { std::ptr::from_mut::((*task).task()) }", + "ptr::from_mut(&mut self.task)", + ]; + expect(banned.filter(s => !isBanned(s))).toEqual([]); + expect(allowed.filter(s => isBanned(s))).toEqual([]); +}); + +test("the call matcher finds every spelling of the call and takes its whole argument", () => { + const source = [ + "fn schedule(&mut self, batch: &mut Batch) {", + " batch.push(Batch::from(&raw mut self.task));", + " batch.push(thread_pool::Batch::from(unsafe { &raw mut (*task).task }));", + " batch.push(ThreadPoolLib::Batch::from(", + " &raw mut combined_part_ranges.last_mut().unwrap().task,", + " ));", + " crate::HTTPThread::schedule(Batch::from(core::ptr::addr_of_mut!(async_http.task)));", + " let batch = PoolBatch::from(unsafe { core::ptr::addr_of_mut!((*task).task) });", + " manager.task_batch.push(ThreadPoolBatch::from(queued));", + " batch.push(Batch::from(unsafe {", + " &raw mut (*runner_tasks.add(i)).task", + " }));", + " // batch.push(Batch::from(&raw mut self.task));", + "}", + ].join("\n"); + const stripped = source.replace(/^[ \t]*\/\/.*$/gm, ""); + expect(findBannedBatches(stripped).map(offset => lineOf(stripped, offset))).toEqual([2, 4, 7]); +}); + +test("no Batch::from call projects the task through a reference to the object", () => { + expect(offenders).toEqual([]); +}); + +test("allowlisted files still carry exactly their documented count", () => { + // Ratchet: once an allowlisted instance is converted, delete its entry so + // a new one cannot take its place. + for (const [f, n] of Object.entries(ALLOW)) { + expect({ file: f, count: counts[f] ?? 0 }).toEqual({ file: f, count: n }); + } +});