diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index df0996b0438a..f1b2545d85c2 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -301,14 +301,15 @@ pub(crate) fn run_task( .run(); } task_tag::AsyncCpTask => { - // SAFETY: posted by `on_subtask_done` with the count at zero (exclusive). - unsafe { (*task.ptr.cast::()).run_from_js_thread()? }; + // SAFETY: boxed by `schedule_new`; posted once by `on_subtask_done` with + // the count at zero; the arm consumes it. + unsafe { bun_core::heap::take(cast_ptr!(crate::node::fs::AsyncCpTask)) } + .run_from_js_thread()?; } task_tag::ShellAsyncCpTask => { // SAFETY: as above. - unsafe { - (*task.ptr.cast::()).run_from_js_thread()? - }; + unsafe { bun_core::heap::take(cast_ptr!(crate::node::fs::ShellAsyncCpTask)) } + .run_from_js_thread()?; } task_tag::StatWatcherHop => { // SAFETY: posted by `StatWatcher::post_to_js_thread` with a ref held. @@ -429,7 +430,10 @@ pub(crate) fn run_task( for_each_fs_uv_op!(__fs_pat) => { macro_rules! __fs_run { ($($tag:ident $ty:ident;)*) => { match task.tag { - $(task_tag::$tag => cast!(fs_async::$ty).run_from_js_thread()?,)* + // SAFETY: §Dispatch — tag identifies the pointee: the box + // `UVFSRequest::create` leaked, enqueued once; the arm consumes it. + $(task_tag::$tag => unsafe { bun_core::heap::take(cast_ptr!(fs_async::$ty)) } + .run_from_js_thread()?,)* // SAFETY: outer arm guard proves one of the table tags matched. _ => unsafe { core::hint::unreachable_unchecked() }, }}; diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index f0941840d3d2..971f8d0b57b6 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -3,7 +3,7 @@ // The top-level functions assume the arguments are already validated use bun_paths::strings; -use core::ffi::{c_char, c_int, c_uint, c_void}; +use core::ffi::{c_char, c_int, c_uint}; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; @@ -648,6 +648,13 @@ mod _async_tasks { pub(crate) tracker: AsyncTaskTracker, } + #[cfg(windows)] + impl Drop for UVFSRequest { + fn drop(&mut self) { + self.r#ref.unref(bun_io::js_vm_ctx()); + } + } + #[cfg(windows)] impl UVFSRequest where @@ -681,10 +688,7 @@ mod _async_tasks { r#ref: KeepAlive::default(), tracker: AsyncTaskTracker::init(vm), }); - // Transfer ownership to libuv: the box outlives the async request and is - // reclaimed in `destroy()` (run_from_js_thread → scopeguard). `heap::release` - // names that hand-off — it is `Box::leak` under the hood; the reclaim - // happens in `destroy()`, not in this scope. + // Reclaimed by the task-queue arm (`run_from_js_thread`) or `release_unrun`. let task: &mut Self = bun_core::heap::release(task); // KeepAlive::ref_ now takes the type-erased aio EventLoopCtx; the JS // event loop is the only one that owns AsyncFSTask/UVFSRequest. @@ -693,7 +697,7 @@ mod _async_tasks { task.tracker.did_schedule(global_object); let loop_ = uv::Loop::get(); - task.req.data = core::ptr::from_mut::(task).cast::(); + task.req.data = core::ptr::from_mut::(task).cast(); // The match resolves at compile time (`F` is a const generic), but // each arm's body needs `A` re-asserted to its concrete `args::*` @@ -942,15 +946,13 @@ mod _async_tasks { .enqueue_task(bun_jsc::Task::init(this_ptr)); } - pub(crate) fn run_from_js_thread(&mut self) -> Result<(), bun_jsc::JsTerminated> { - // SAFETY: self was Box::leak'd in create(); destroy() runs exactly once on scope exit - let _deinit = - scopeguard::guard(core::ptr::from_mut(self), |p| unsafe { Self::destroy(p) }); - // Move `result` out so the `global_object()` `&self` borrow can coexist - // with `&mut result` below; the sentinel left behind is dropped in `destroy()`. + /// Settles the promise; the request (and its keep-alive) is released on return. + #[allow(clippy::boxed_local, reason = "reclaim point for the boxed task")] + pub(crate) fn run_from_js_thread(mut self: Box) -> Result<(), bun_jsc::JsTerminated> { + // Moved out: `fs_to_js` needs `&mut` while `global_object()` borrows `self`. let mut result = core::mem::replace(&mut self.result, Err(sys::Error::default())); let global_object = self.global_object(); - let success = matches!(result, Ok(_)); + let success = result.is_ok(); let promise_value = self.promise.value(); let promise = self.promise.get(); let result = match &mut result { @@ -978,15 +980,6 @@ mod _async_tasks { } Ok(()) } - - /// SAFETY: `this` must be the pointer Box::leak'd in `create()`; called exactly once. - pub(crate) unsafe fn destroy(this: *mut Self) { - // SAFETY: caller guarantees `this` is the live Box-leaked allocation; - // reclaim ownership (paired with the Box::leak in create()). - let mut task = unsafe { bun_core::heap::take(this) }; - // `bun_sys::Error` frees its path on Drop. - task.r#ref.unref(bun_io::js_vm_ctx()); - } } // ────────────────────────────────────────────────────────────────────────── @@ -1220,10 +1213,10 @@ mod _async_tasks { { const TAG: bun_event_loop::TaskTag = F.task_tag(); /// A libuv fs request that completed into the queue after the last - /// tick: destroy releases its promise handle and keep-alive. + /// tick: dropping it releases its promise handle and keep-alive. unsafe fn release_unrun(this: *mut Self) { - // SAFETY: fn contract — `Box::leak`'d in `UVFSRequest::create`. - unsafe { Self::destroy(this) } + // SAFETY: fn contract — leaked in `UVFSRequest::create`, reclaimed once. + unsafe { bun_core::heap::destroy(this) } } } @@ -1394,6 +1387,14 @@ mod _async_tasks { pub(crate) shelltask: Option>, } + impl Drop for NewAsyncCpTask { + fn drop(&mut self) { + if !IS_SHELL { + self.r#ref.unref(event_loop_handle_to_ctx(self.evtloop)); + } + } + } + bun_threading::intrusive_work_task!([const IS_SHELL: bool] NewAsyncCpTask, task); /// This task is used by `AsyncCpTask/fs.promises.cp` to copy a single file. @@ -1403,7 +1404,7 @@ mod _async_tasks { /// subtask via the `subtask_count` refcount (see `on_subtask_done`). Stored /// as `ParentRef` (constructed from the `*mut` with `Box::leak` provenance) /// so shared reads are safe-projected and `as_mut_ptr()` round-trips the - /// original write provenance for `on_subtask_done`'s `&mut` promotion. + /// original write provenance that `on_subtask_done`'s completion frees with. pub(crate) cp_task: bun_ptr::ParentRef, bun_ptr::Mut>, /// Single owned allocation laid out as `\0\0`. Ownership is /// encoded directly as `Box<[OSPathChar]>` and @@ -1454,9 +1455,7 @@ mod _async_tasks { } fn run_owned(self: Box) { - // `ParentRef` preserves the `Box::leak` mutable provenance so - // `on_subtask_done` may later promote it to `&mut` via `as_mut_ptr()` - // once the refcount reaches zero. + // `ParentRef` keeps the `Box::leak` provenance the completion frees with. let cp_task = self.cp_task; // Shared borrow only — other workpool threads (and the directory-scan // thread) may hold `&Self` to the same parent concurrently; `ParentRef` @@ -1507,11 +1506,11 @@ mod _async_tasks { } else { bun_event_loop::task_tag::AsyncCpTask }; - /// A finished fs.cp whose completion will not run: destroy releases + /// A finished fs.cp whose completion will not run: dropping it releases /// its promise handle, protected arguments and keep-alive. unsafe fn release_unrun(this: *mut Self) { // SAFETY: fn contract — posted by `on_subtask_done` with the count at zero. - unsafe { Self::destroy(this) } + unsafe { bun_core::heap::destroy(this) } } } @@ -1550,7 +1549,9 @@ mod _async_tasks { tracker, core::ptr::null_mut(), ); - // SAFETY: `schedule_new` returns a Box::leak'd pointer; valid until destroy() + // SAFETY: `schedule_new` returns the leaked box, which only the completion + // posted to this thread reclaims, so it is live until we return to the + // event loop; the pool side never touches `promise`. unsafe { &*task }.promise.value() } @@ -1644,10 +1645,7 @@ mod _async_tasks { /// drops to zero) enqueues `runFromJSThread`, which resolves the promise /// and destroys `this`. /// - /// Takes a raw `*mut Self` (not `&self`) so the pointer retains the - /// mutable provenance from the original `Box::leak`; the JS-thread - /// callback later materializes `&mut *this`, which would be UB if the - /// pointer were derived from a shared reference. + /// `*mut Self`, not `&self`: the completion frees the box through this pointer. fn on_subtask_done(this: *mut Self) { // SAFETY: `this` is a live Box-leaked task; shared access only here — // other workpool threads may concurrently hold `&Self` until the @@ -1666,9 +1664,7 @@ mod _async_tasks { this_ref.result.set(Ok(())); } - // Count reached zero ⇒ exclusive access. `this` carries mutable - // provenance from `Box::leak`, so the enqueued callback may safely - // form `&mut *this` on the JS thread. + // Count reached zero ⇒ exclusive access; the completion frees `*this`. let poster = this_ref.poster.clone(); if poster.is_js() { let ct = ConcurrentTask::ConcurrentTask::create(bun_jsc::Task::init(this)); @@ -1677,13 +1673,13 @@ mod _async_tasks { unreachable!("VM handle closed with an fs.cp outstanding"); }; } else { - let at = AnyTaskWithExtraContext::from_callback_auto_deinit( - this, - |p: *mut Self, ctx| { - // SAFETY: subtask count hit zero ⇒ exclusive access to the leaked task. - unsafe { (*p).run_from_js_thread_mini(ctx) } - }, - ); + let at = + AnyTaskWithExtraContext::from_callback_auto_deinit(this, |p: *mut Self, _| { + debug_assert!(IS_SHELL, "only the shell posts cp tasks to a mini loop"); + // SAFETY: count hit zero ⇒ sole pointer to the box `schedule_new` leaked. + let task = unsafe { bun_core::heap::take(p) }; + let _ = task.run_from_js_thread(); // shell tasks settle no promise: always `Ok` + }); // `from_callback_auto_deinit` heap-allocates; never null. poster.post_mini(core::ptr::NonNull::new(at).expect("heap task")); } @@ -1691,22 +1687,16 @@ mod _async_tasks { poster.embedded_work_finished(); } - pub(crate) fn run_from_js_thread_mini(&mut self, _: *mut c_void) { - let _ = self.run_from_js_thread(); // TODO: properly propagate exception upwards - } - - pub(crate) fn run_from_js_thread(&mut self) -> Result<(), bun_jsc::JsTerminated> { + /// Settles the promise (shell: continues the `ShellCpTask`); the task is freed on return. + #[allow(clippy::boxed_local, reason = "reclaim point for the boxed task")] + pub(crate) fn run_from_js_thread(self: Box) -> Result<(), bun_jsc::JsTerminated> { + // `Maybe` (= `Maybe<()>`) has a cheap `Ok(())` placeholder. + let mut result = self.result.replace(Ok(())); if IS_SHELL { - // SAFETY: shelltask is set by create_for_shell and outlives this task - // Move the result out — `Maybe` (= `Maybe<()>`) has a cheap - // `Ok(())` placeholder. - let result = core::mem::replace(self.result.get_mut(), Ok(())); let shelltask = self.shelltask.expect("IS_SHELL ⇒ shelltask").as_mut_ptr(); // SAFETY: shelltask is non-null in the IS_SHELL specialization and - // outlives this task; `cp_on_finish` enqueues it concurrently. + // outlives this task; `cp_on_finish` continues it in place. unsafe { ShellCpTask::cp_on_finish(shelltask, result) }; - // SAFETY: self was Box::leak'd in create*(); destroyed exactly once here - unsafe { Self::destroy(std::ptr::from_mut::(self)) }; return Ok(()); } let go_ptr = self.evtloop.global_object(); @@ -1717,28 +1707,20 @@ mod _async_tasks { } // SAFETY: non-null erased *mut JSGlobalObject from the JS event loop vtable. let global_object: &JSGlobalObject = unsafe { &*go_ptr.cast::() }; - let success = (*self.result.get_mut()).is_ok(); + let success = result.is_ok(); let promise_value = self.promise.value(); - // Captured as a raw pointer because `Self::destroy(self)` runs *before* the - // resolve/reject. The `JSPromise` itself lives on the JS heap - // and is kept alive past `destroy` by `promise_value.ensure_still_alive()`. - let promise: *mut bun_jsc::JSPromise = self.promise.get(); - let result = match self.result.get_mut() { - // SAFETY: `promise` is the sole live reference to the heap `JSPromise`. - Err(err) => match err.to_js_with_async_stack(global_object, unsafe { &*promise }) { + let promise = self.promise.get(); + let result = match &mut result { + Err(err) => match err.to_js_with_async_stack(global_object, promise) { Ok(v) => v, Err(e) => { - // SAFETY: `promise` points at a GC-rooted JS heap cell; sole live - // reference on this thread (see comment above `let promise`). - return unsafe { &mut *promise }.reject(global_object, Err(e)); + return promise.reject(global_object, Err(e)); } }, Ok(res) => match FsReturn::fs_to_js(res, global_object) { Ok(v) => v, Err(e) => { - // SAFETY: `promise` points at a GC-rooted JS heap cell; sole live - // reference on this thread (see comment above `let promise`). - return unsafe { &mut *promise }.reject(global_object, Err(e)); + return promise.reject(global_object, Err(e)); } }, }; @@ -1746,31 +1728,14 @@ mod _async_tasks { let _dispatch = self.tracker.dispatch(global_object); - // SAFETY: self was Box::leak'd in create*(); destroyed exactly once here - unsafe { Self::destroy(std::ptr::from_mut::(self)) }; if success { - bun_jsc::JSPromise::opaque_mut(promise).resolve(global_object, result)?; + promise.resolve(global_object, result)?; } else { - bun_jsc::JSPromise::opaque_mut(promise).reject(global_object, Ok(result))?; + promise.reject(global_object, Ok(result))?; } Ok(()) } - /// SAFETY: `this` must be the pointer returned by Box::leak in - /// `schedule_new()`; called exactly once. - pub(crate) unsafe fn destroy(this: *mut Self) { - // SAFETY: caller guarantees `this` is the live Box-leaked allocation; - // reclaim ownership (paired with the Box::leak in - // schedule_new()). - let mut task = unsafe { bun_core::heap::take(this) }; - if !IS_SHELL { - let ctx = event_loop_handle_to_ctx(task.evtloop); - task.r#ref.unref(ctx); - } - // `Drop for ThreadSafe` releases the `protect()` taken by - // `to_thread_safe()` when `src`/`dest` are Buffers, so nothing leaks here. - } - /// Directory scanning + clonefile will block this thread, then each individual file copy (what the sync version /// calls "copy_single_file_sync") will be dispatched as a separate task. pub(crate) fn cp_async(nodefs: &mut NodeFS, this: *mut Self) { diff --git a/test/internal/source-lints/self-receiver-teardown.test.ts b/test/internal/source-lints/self-receiver-teardown.test.ts new file mode 100644 index 000000000000..b835833b45c3 --- /dev/null +++ b/test/internal/source-lints/self-receiver-teardown.test.ts @@ -0,0 +1,321 @@ +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"; + +// A `&self` / `&mut self` method must not hand its own receiver to the type's +// teardown routine: `Self::destroy(ptr::from_mut(self))`, `Self::deinit(self as +// *mut _)`, `Self::finalize(ptr::from_mut(self))`, or the deferred forms +// `scopeguard::guard(ptr::from_mut(self), |p| Self::destroy(p))` and +// `scopeguard::guard(ptr::from_mut(self), Self::destroy)` (which free in the +// epilogue, while the receiver argument is still live) are banned. +// +// `destroy(this: *mut Self)` / `deinit(this: *mut Self)` / `finalize(this: *mut +// Self)` are the names this tree's "reclaim the Box" routines (`heap::take` / +// `heap::destroy` inside) go by. Under Stacked and Tree Borrows a reference +// argument is protected for the whole call, and deallocating protected memory +// is UB regardless of whether the reference is used again afterwards (Miri: +// "deallocating while item is strongly protected" under Stacked Borrows, "the +// strongly protected tag disallows deallocations" under Tree Borrows, the model +// `bun run rust:miri` checks; the protector is the model's counterpart of the +// `dereferenceable` attribute rustc puts on reference arguments, so this is not +// only a Miri concern). The method has to own the allocation instead: the +// caller that holds the raw pointer (a dispatch arm, a C callback) reclaims +// the box (`heap::take`) and calls a `self: Box` method, with the +// teardown in `Drop`, so every return path frees it; see +// `UVFSRequest::run_from_js_thread` / `NewAsyncCpTask::run_from_js_thread` in +// src/runtime/node/node_fs.rs and the arms that call them in +// src/runtime/dispatch.rs. Where the pointer cannot be turned back into a box +// at the entry point, the fallback is a `this: *mut Self` function that +// reborrows per statement and frees through `this` (see the comment on +// `deinit(this: *mut Self)` in src/sql_jsc/postgres/PostgresSQLConnection.rs). +// +// Scope: the single-expression shapes above, with the callee literally named +// `destroy`, `deinit` or `finalize` (`heap::destroy` counts as a `destroy` +// spelling). The name list is the enforcement boundary: a reclaim routine under +// another name is not caught until its name is added here. Deliberately +// outside it: +// - the other reclaim primitives (`heap::take(from_mut(self))`, +// `Box::from_raw(self as *mut _)`), a separate population one layer down +// (#37672 adds the lint for them); +// - refcount releases (`Self::deref(from_mut(self))`), which only free on the +// last count, so each site needs its own argument about who else holds one; +// - in-place finalizers and UFCS forwarding (`Self::finalize(self)`), which +// take no address; a self-derived pointer stashed in a local and freed +// later; reference parameters (`fn f(this: &mut T)` freeing `this`); and a +// guard closure whose first expression is not the teardown call (whatever +// follows that call is not looked at). +// +// Sibling guards: fn-long-mut-reborrow.test.ts, frozen-nonnull-reborrow.test.ts, +// unsound-erased-box.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)); +})(); + +// An optional `::<..>`, allowing one level of nesting (`::>`). +const TURBOFISH = String.raw`(?:::<(?:[^<>]|<[^<>]*>)*>)?`; + +// `self` (or a reborrow of it) as the sole argument of a pointer constructor. +const SELF_ARG = String.raw`\(\s*(?:&(?:mut\s+)?\*\s*)?self\s*\)`; + +// The ways of spelling "`self`, as a raw pointer", optionally parenthesized and +// optionally followed by pointer-to-pointer conversions that keep it the same +// address (`.cast()`, `.cast_mut()`, `.as_ptr()`). `(?!\s*\.)` after +// `&raw mut *self` keeps a field's address (`&raw mut *self.inner`) out of it. +const SELF_AS_POINTER = + String.raw`\(?\s*(?:` + + [ + String.raw`(?:[\w:]+::)?from_(?:mut|ref)${TURBOFISH}${SELF_ARG}`, + String.raw`(?:[\w:]+::)?NonNull::from${SELF_ARG}`, + String.raw`&raw\s+(?:mut|const)\s+\*\s*self\b(?!\s*\.)`, + String.raw`(?:[\w:]+::)?addr_of(?:_mut)?!\s*\(\s*\*\s*self\s*\)`, + String.raw`self\s+as\s+\*(?:mut|const)\b[^,()]*`, + ].join("|") + + String.raw`)\s*\)?(?:\s*\.\s*(?:cast(?:_mut|_const)?${TURBOFISH}|as_ptr)\(\))*`; + +// A teardown routine by path (`destroy`, `Self::deinit`, `bun_core::heap::destroy`, +// `Worker::deinit::`), and the same followed by its opening paren (`\s*` +// after it so a rustfmt-wrapped argument list still matches). +const TEARDOWN_FN = String.raw`\b(?:[\w:]+::)?(?:destroy|deinit|finalize)${TURBOFISH}`; +const TEARDOWN = String.raw`${TEARDOWN_FN}\s*\(\s*`; + +const DIRECT = new RegExp(`${TEARDOWN}${SELF_AS_POINTER}\\s*[,)]`, "g"); + +// `scopeguard::guard(, |p| { unsafe { Self::destroy(p) } })` or +// `scopeguard::guard(, Self::destroy)`: the guard's callback +// must be the teardown routine, applied (in the closure form, via the +// back-reference, as the closure's first expression; anything after it still +// runs after the free and does not rescue the guard) to the guarded pointer +// itself. A guard over `self`'s address that does something else with it +// (src/runtime/ffi/ffi_body.rs frees a field) does not count. +const DEFERRED = new RegExp( + String.raw`scopeguard::guard\(\s*${SELF_AS_POINTER}\s*,\s*(?:` + + String.raw`(?:move\s+)?\|\s*(\w+)\s*\|\s*(?:\{\s*)?(?:unsafe\s*\{\s*)?${TEARDOWN}\1\s*\)` + + String.raw`|${TEARDOWN_FN}\s*\))`, + "g", +); + +/** Every banned occurrence in one file's text, in line order. */ +function scanText(content: string): { line: number; text: string }[] { + // Strip full-line comments so prose mentions don't count and a SAFETY comment + // inside a guard's closure doesn't break the match. `[ \t]*`, not `\s*`: `\s` + // crosses newlines and would swallow blank lines, shifting the reported line + // numbers. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + return [DIRECT, DEFERRED] + .flatMap(re => [...stripped.matchAll(re)]) + .map(m => ({ line: stripped.slice(0, m.index).split("\n").length, text: m[0].replace(/\s+/g, " ") })) + .sort((a, b) => a.line - b.line); +} + +// Documented, ratcheted exceptions: a file may keep exactly `count` hits whose +// (whitespace-collapsed) text is exactly `text` while their conversion is in +// flight; any other spelling in the file, or one more of this one, is an +// offender. Delete an entry when its file is converted; never raise one. +const ALLOW: Record = { + // `LifecycleScriptSubprocess::handle_exit` / `deinit_and_delete_package` + // (`&mut self`) free the subprocess at five sites; #37551 turns them into a + // disposition that the raw-pointer thunks act on. + "src/install/lifecycle_script_runner.rs": { + count: 5, + text: "Self::destroy(std::ptr::from_mut::(self))", + }, + // `Worker::deinit_soon` (`&mut self`) frees itself inline when the worker + // was created off the pool; #37685 converts it. + "src/bundler/ThreadPool.rs": { count: 1, text: "Self::deinit(std::ptr::from_mut::(self))" }, + // `CopyFileWindows::throw` / `resolve_promise` and `ReadFileUV::on_finish` + // (`&mut self`) free the task they run on, reached through further + // `&mut self` frames; #37705 converts them. + "src/runtime/webcore/blob/copy_file.rs": { count: 2, text: "Self::destroy(core::ptr::from_mut(self))" }, + "src/runtime/webcore/blob/read_file.rs": { count: 1, text: "Self::finalize(core::ptr::from_mut(self))" }, +}; + +/** + * Applies one file's allowlist entry: `documented` counts the hits with the + * documented text (the ratchet), and everything that is not one of the first + * `count` of those is an offender. + */ +function triage( + source: string, + hits: { line: number; text: string }[], + allow: { count: number; text: string } | undefined, +): { documented: number; offenders: string[] } { + let documented = 0; + const offenders: string[] = []; + for (const { line, text } of hits) { + if (allow !== undefined && text === allow.text && documented++ < allow.count) continue; + offenders.push(`${source}:${line}: ${text}`); + } + return { documented, offenders }; +} + +const counts: Record = {}; +const offenders: string[] = []; +let scanned = 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 result = triage(source, scanText(await file(abs).text()), ALLOW[source]); + if (result.documented > 0) counts[source] = result.documented; + offenders.push(...result.offenders); +} + +const matches = (snippet: string): boolean => scanText(snippet).length > 0; + +test("scans a non-empty set of tracked Rust sources", () => { + // Guards against the tracked/realpath filters above over-firing and leaving + // nothing to scan, which would make the ban below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("the patterns recognize the spellings they claim to", () => { + const banned = [ + // The node_fs.rs lines this lint was written for. + "unsafe { Self::destroy(std::ptr::from_mut::(self)) };", + "let _deinit =\n scopeguard::guard(core::ptr::from_mut(self), |p| unsafe { Self::destroy(p) });", + // Other callees and other spellings of the receiver's address. + "unsafe { Self::destroy(core::ptr::from_mut(self)) };", + "unsafe { Self::deinit(std::ptr::from_mut::(self)) };", + "Self::finalize(core::ptr::from_mut(self));", + "unsafe { Worker::deinit(self as *mut Self) };", + "unsafe { destroy(self as *const Self as *mut Self) }", + "unsafe { Self::destroy(&raw mut *self) }", + "unsafe { Self::destroy(core::ptr::addr_of_mut!(*self)) }", + "unsafe { Self::destroy(ptr::from_ref(self).cast_mut()) }", + "unsafe { Self::destroy(NonNull::from(self).as_ptr()) }", + "unsafe { Self::destroy(ptr::from_mut(&mut *self)) }", + "unsafe { Self::destroy((self as *mut Self).cast::()) }", + "unsafe { Self::destroy::(std::ptr::from_mut(self)) }", + "unsafe { Self::destroy(std::ptr::from_mut::>(self)) }", + "unsafe { crate::node::fs::AsyncCpTask::destroy(std::ptr::from_mut(self)) }", + "unsafe { bun_core::heap::destroy(std::ptr::from_mut::(self)) };", + // Extra arguments after the pointer, and a rustfmt-wrapped call. + "unsafe { Self::deinit(std::ptr::from_mut(self), allocator) }", + "unsafe {\n Self::destroy(\n std::ptr::from_mut::(self),\n )\n}", + // Deferred: block body with a SAFETY comment, `move`, and the fn-value form. + "let _g = scopeguard::guard(std::ptr::from_mut::(self), |this| {\n // SAFETY: leaked in new(); freed exactly once here.\n unsafe { Self::destroy(this) }\n});", + "let _g = scopeguard::guard(self as *mut Self, move |p| unsafe { Self::deinit(p) });", + 'let _g = scopeguard::guard(core::ptr::from_mut(self), |p| { unsafe { Self::destroy(p) }; log!("freed"); });', + "let _g = scopeguard::guard(core::ptr::from_mut(self), Self::destroy);", + "let _g = scopeguard::guard(self as *mut Self, Worker::deinit);", + ]; + const allowed = [ + // The converted shapes: the caller reclaims the box, or the pointer comes + // in as a parameter. + "unsafe { bun_core::heap::take(cast_ptr!(crate::node::fs::AsyncCpTask)) }.run_from_js_thread()?;", + "unsafe { Self::destroy(this) }", + "let _deinit = scopeguard::guard(this, |p| unsafe { Self::destroy(p) });", + "let _guard = scopeguard::guard(this_ptr, Self::deinit);", + "unsafe { FSWatchTask::deinit(t) };", + // Freeing something the receiver owns is fine. + "unsafe { Self::destroy(self.child) }", + "unsafe { Worker::deinit(self.worker.as_ptr()) }", + "unsafe { Self::destroy(&raw mut *self.inner) }", + "unsafe { TCC::State::destroy(s.as_ptr()) };", + // By-value teardown and UFCS forwarding of the reference itself take no + // address. + "self.deinit();", + "self.io_request.deinit();", + "Self::deinit(self)", + "Self::deinit(self, id)", + "Self::finalize(self)", + // Producing the receiver's address for something other than teardown. + "self.req.data = core::ptr::from_mut(self).cast::();", + "unsafe { Self::deref_(std::ptr::from_mut::(self)) };", + "unsafe { Self::teardown(core::ptr::from_mut(self), Teardown::MainThreadExit) };", + // The other primitives are a separate population (see the scope note). + "unsafe { drop(bun_core::heap::take(std::ptr::from_mut::(self))) };", + // A guard over the receiver's address whose callback frees a field, + // releases a refcount, or frees some other pointer is out of scope. + "let _guard = scopeguard::guard(std::ptr::from_mut::(self), |this_ptr| {\n // SAFETY: this_ptr is self for the duration of compile().\n if let Some(s) = unsafe { (*this_ptr).state.take() } {\n unsafe { TCC::State::destroy(s.as_ptr()) };\n }\n});", + "let _g = scopeguard::guard(std::ptr::from_mut::(self), |s| {\n unsafe { Self::deref_(s) }\n});", + "let _g = scopeguard::guard(std::ptr::from_mut::(self), Self::deref_);", + "let _g = scopeguard::guard(std::ptr::from_mut::(self), |_p| unsafe { Self::destroy(other) });", + // Not the receiver. + "unsafe { Self::destroy(std::ptr::from_mut::(self_)) };", + "unsafe { Self::destroy(ptr::from_mut(task)) }", + // Prose. + "// Unlike `Self::destroy(std::ptr::from_mut::(self))`, this takes the box.", + ]; + expect(banned.filter(s => !matches(s))).toEqual([]); + expect(allowed.filter(matches)).toEqual([]); +}); + +test("a file is scanned with comments stripped and hits attributed to their lines", () => { + // The three shapes `main` had in node_fs.rs, behind a comment that mentions + // one of them and with a SAFETY comment inside the guard's closure. Unlike the + // ratchet below, this keeps exercising the whole per-file pipeline once the + // allowlist is empty. + const fixture = [ + "// `Self::destroy(std::ptr::from_mut::(self))` is what this replaces.", + "impl Task {", + " fn uv(&mut self) {", + " let _deinit =", + " scopeguard::guard(core::ptr::from_mut(self), |p| unsafe { Self::destroy(p) });", + " }", + " fn cp(&mut self) {", + " unsafe { Self::destroy(std::ptr::from_mut::(self)) };", + " unsafe { Self::destroy(self.child) };", + " }", + " fn guarded(&mut self) {", + " let _g = scopeguard::guard(std::ptr::from_mut::(self), |this| {", + " // SAFETY: leaked in new(); freed exactly once here.", + " unsafe { Self::deinit(this) }", + " });", + " }", + "}", + ].join("\n"); + expect(scanText(fixture)).toEqual([ + { line: 5, text: "scopeguard::guard(core::ptr::from_mut(self), |p| unsafe { Self::destroy(p)" }, + { line: 8, text: "Self::destroy(std::ptr::from_mut::(self))" }, + { line: 12, text: "scopeguard::guard(std::ptr::from_mut::(self), |this| { unsafe { Self::deinit(this)" }, + ]); +}); + +test("an allowlist entry exempts only its documented spelling, and only that many of it", () => { + const destroy = "Self::destroy(core::ptr::from_mut(self))"; + const deinit = "Self::deinit(core::ptr::from_mut(self))"; + const hits = [ + { line: 10, text: destroy }, + { line: 20, text: destroy }, + { line: 30, text: deinit }, + ]; + expect(triage("x.rs", hits, { count: 1, text: destroy })).toEqual({ + documented: 2, + offenders: [`x.rs:20: ${destroy}`, `x.rs:30: ${deinit}`], + }); + expect(triage("x.rs", hits, undefined)).toEqual({ + documented: 0, + offenders: [`x.rs:10: ${destroy}`, `x.rs:20: ${destroy}`, `x.rs:30: ${deinit}`], + }); +}); + +test("no method hands its own receiver to destroy/deinit/finalize", () => { + expect(offenders).toEqual([]); +}); + +test("allowlisted files still carry exactly their documented sites", () => { + // Ratchet: once an allowlisted file is converted, delete its entry so a new + // instance cannot take the old one's place. + const actual = Object.fromEntries(Object.keys(ALLOW).map(f => [f, counts[f] ?? 0])); + const documented = Object.fromEntries(Object.entries(ALLOW).map(([f, { count }]) => [f, count])); + expect(actual).toEqual(documented); +});