diff --git a/src/runtime/dispatch.rs b/src/runtime/dispatch.rs index df0996b0438a..16b20ae22927 100644 --- a/src/runtime/dispatch.rs +++ b/src/runtime/dispatch.rs @@ -370,7 +370,10 @@ pub(crate) fn run_task( // ── napi ───────────────────────────────────────────────────────── task_tag::NapiAsyncWork => { - cast!(napi_async_work).run_from_js(vm, global); + // SAFETY: §Dispatch — tag identifies the pointee; the addon's + // `complete` callback usually frees the work, so it takes the raw + // pointer (no `&mut` at this boundary). + unsafe { napi_async_work::run_from_js(cast_ptr!(napi_async_work), vm, global) }; } task_tag::ThreadSafeFunction => { ThreadSafeFunction::on_dispatch(cast_ptr!(ThreadSafeFunction)); diff --git a/src/runtime/napi/napi_body.rs b/src/runtime/napi/napi_body.rs index 4ad87609e8e7..e78fe2bdd3f3 100644 --- a/src/runtime/napi/napi_body.rs +++ b/src/runtime/napi/napi_body.rs @@ -50,7 +50,7 @@ impl Taskable for napi_async_work { let vm = VirtualMachine::get().as_mut(); let global = vm.global(); // SAFETY: fn contract — the addon's live work object the pool posted. - unsafe { (*this).run_from_js(vm, global) }; + unsafe { napi_async_work::run_from_js(this, vm, global) }; } } impl Taskable for ThreadSafeFunction { @@ -1754,11 +1754,18 @@ pub(super) enum AsyncWorkStatus { Cancelled = 3, } -/// must be globally allocated +/// Heap-allocated; owned and freed by the addon. While queued it is shared +/// between the JS thread (which may still cancel or re-queue it, and whose +/// `complete` usually frees it the moment `run` has posted it) and the pool +/// thread, so the entry points below take the addon's pointer and go through it +/// field by field instead of forming a reference to the whole work. The field +/// docs say which thread writes what; unmarked fields are immutable after +/// [`Self::new`]. pub(crate) struct napi_async_work { + /// The pool's while the work is queued. pub task: WorkPoolTask, + /// Written by the pool thread as it hands the work back. pub(crate) concurrent_task: ConcurrentTask, - // Note: BackRef — `enqueue_task` needs `&mut EventLoop`; reborrowed at use sites. /// How the pool thread delivers completion / cancellation to the VM. pub(crate) loop_handle: bun_jsc::LoopHandle, /// JS thread only. @@ -1767,8 +1774,11 @@ pub(crate) struct napi_async_work { pub(crate) execute: napi_async_execute_callback, pub(crate) complete: Option, pub(crate) data: *mut c_void, - pub(crate) status: AtomicU32, // AsyncWorkStatus + /// [`AsyncWorkStatus`]; the one field both threads write. + pub(crate) status: AtomicU32, + /// JS thread only. pub(crate) scheduled: bool, + /// JS thread only. pub poll_ref: KeepAlive, } @@ -1811,111 +1821,152 @@ impl napi_async_work { drop(unsafe { bun_core::heap::take(this) }); } - pub(crate) fn schedule(&mut self) { - if self.scheduled { - return; + /// `napi_queue_async_work`; JS thread. + /// + /// # Safety + /// `this` is a live work. + pub(crate) unsafe fn schedule(this: *mut Self) { + // SAFETY: fn contract. The first call hands the pool its field on the + // last line and touches JS-thread and immutable fields before that + // (see the struct); a repeat call while the pool has the work only + // reads `scheduled`. + unsafe { + if (*this).scheduled { + return; + } + (*this).scheduled = true; + (*this).poll_ref.ref_(bun_io::js_vm_ctx()); + // The work object belongs to the addon and `execute` receives this + // env: counted, so the VM waits for it (Node likewise settles its + // threadpool requests before an environment is freed). + (*this).loop_handle.embedded_work_scheduled(); + WorkPool::schedule(&raw mut (*this).task); } - self.scheduled = true; - self.poll_ref.ref_(bun_io::js_vm_ctx()); - // The work object belongs to the addon and `execute` receives this - // env: counted, so the VM waits for it (Node likewise settles its - // threadpool requests before an environment is freed). - self.loop_handle.embedded_work_scheduled(); - WorkPool::schedule(&raw mut self.task); } pub(crate) unsafe fn run_from_thread_pool(task: *mut WorkPoolTask) { - // SAFETY: `task` is the `task` field of a live heap `napi_async_work`, - // exclusively owned by the work pool for this callback's duration. - unsafe { (*napi_async_work::from_task_ptr(task)).run() }; + // SAFETY: `task` is the field `schedule` handed to the pool, projected + // from the work pointer; the pool runs it once. + unsafe { Self::run(napi_async_work::from_task_ptr(task)) }; } - fn run(&mut self) { - let self_ptr: *mut Self = self; - let handle = self.loop_handle.clone(); + /// Pool thread. The JS thread may `cancel` at any point, and frees the + /// work (`complete`) as soon as the post below lands. + /// + /// # Safety + /// `this` is the live work `schedule` handed to the pool. + unsafe fn run(this: *mut Self) { + // SAFETY: fn contract; immutable fields here, and below only the pool + // thread's fields and the atomic (see the struct). + let (handle, execute, env, data) = unsafe { + ( + (*this).loop_handle.clone(), + (*this).execute, + (*this).env.get(), + (*this).data, + ) + }; // A VM that is already stopping cancels work it has not started, as // Node's environment cleanup does (uv_cancel); otherwise `execute` runs // with the VM held open. let vm = handle.borrow_if_running(); let started = vm.is_some() - && match self.status.compare_exchange( - AsyncWorkStatus::Pending as u32, - AsyncWorkStatus::Started as u32, - Ordering::SeqCst, - Ordering::SeqCst, - ) { + // SAFETY: as above. + && match unsafe { + (*this).status.compare_exchange( + AsyncWorkStatus::Pending as u32, + AsyncWorkStatus::Started as u32, + Ordering::SeqCst, + Ordering::SeqCst, + ) + } { Ok(_) => true, Err(state) => state != AsyncWorkStatus::Cancelled as u32, }; if started { - (self.execute)(self.env.get(), self.data); - self.status - .store(AsyncWorkStatus::Completed as u32, Ordering::SeqCst); - } else { - let _ = self.cancel(); + execute(env, data); } + // The queue takes the embedded task (and with it the work) from here + // on. Counted work, so the VM has not closed its handle; a VM tearing + // down runs `complete` from its queue release (status cancelled if + // `execute` never ran), as Node does at environment cleanup. + // SAFETY: as above; filling in the task is the pool thread's last + // access to `*this`. + let ct = unsafe { + if started { + (*this) + .status + .store(AsyncWorkStatus::Completed as u32, Ordering::SeqCst); + } else { + let _ = Self::cancel(this); + } + core::ptr::NonNull::from((*this).concurrent_task.from(this, AutoDeinit::ManualDeinit)) + }; drop(vm); - self.post_to_js_thread(self_ptr); - // `self` may already be freed by the JS thread; the handle is ours. - handle.embedded_work_finished(); - } - - /// Pool thread → JS thread: run `complete` there. `concurrent_task` is the - /// live inline field of this heap work; the queue takes ownership of its - /// `next` link. Counted work, so the VM has not closed its handle; a VM - /// tearing down runs `complete` from its queue release (status cancelled - /// if `execute` never ran), as Node does at environment cleanup. - fn post_to_js_thread(&mut self, self_ptr: *mut Self) { - let ct = core::ptr::NonNull::from( - self.concurrent_task - .from(self_ptr, AutoDeinit::ManualDeinit), - ); - let bun_jsc::vm_handle::Posted::Queued = self.loop_handle.post_task(ct) else { + let bun_jsc::vm_handle::Posted::Queued = handle.post_task(ct) else { unreachable!("VM handle closed with napi async work outstanding"); }; + handle.embedded_work_finished(); } - pub(crate) fn cancel(&mut self) -> bool { - self.status - .compare_exchange( + /// # Safety + /// `this` is a live work (any thread; only the atomic is touched). + pub(crate) unsafe fn cancel(this: *mut Self) -> bool { + // SAFETY: fn contract. + unsafe { + (*this).status.compare_exchange( AsyncWorkStatus::Pending as u32, AsyncWorkStatus::Cancelled as u32, Ordering::SeqCst, Ordering::SeqCst, ) - .is_ok() + } + .is_ok() } - pub(crate) fn run_from_js(&mut self, vm: &mut VirtualMachine, global: &JSGlobalObject) { - // Note: the "this" value here may already be freed by the user in `complete` - // Note: KeepAlive is not `Copy`, so move it out (the original slot may - // be freed under us by `complete`). - let mut poll_ref = core::mem::take(&mut self.poll_ref); + /// JS thread, from the task queue or from its release at VM teardown. + /// `complete` usually frees the work, so `this` is not used after it. + /// + /// # Safety + /// `this` is the live work [`Self::run`] posted. + pub(crate) unsafe fn run_from_js( + this: *mut Self, + vm: &mut VirtualMachine, + global: &JSGlobalObject, + ) { + // SAFETY: fn contract; the pool thread is done with the work. + let (mut poll_ref, complete, env, status, data) = unsafe { + ( + core::mem::take(&mut (*this).poll_ref), + (*this).complete, + (*this).env.get(), + (*this).status.load(Ordering::SeqCst), + (*this).data, + ) + }; // KeepAlive::unref needs an event-loop ctx so it cannot impl Drop // generically; this is a genuine one-off cleanup. scopeguard::defer! { poll_ref.unref(bun_io::js_vm_ctx()); } // https://github.com/nodejs/node/blob/a2de5b9150da60c77144bb5333371eaca3fab936/src/node_api.cc#L1201 - let Some(complete) = self.complete else { + let Some(complete) = complete else { return; }; - let env = self.env.get(); - // SAFETY: env is held alive by NapiEnvRef for the duration of this call. + // SAFETY: `global` (live for this call) holds a ref on every env made + // for it (`GlobalObject::m_napiEnvs`), so `env` outlives the work's own + // ref, which `complete` usually drops by freeing the work. let env_ref = unsafe { &*env }; let _hs = NapiHandleScope::open_scoped(env_ref); - let status: NapiStatus = - if self.status.load(Ordering::SeqCst) == AsyncWorkStatus::Cancelled as u32 { - NapiStatus::cancelled - } else { - NapiStatus::ok - }; + let status: NapiStatus = if status == AsyncWorkStatus::Cancelled as u32 { + NapiStatus::cancelled + } else { + NapiStatus::ok + }; - complete(env, status as napi_status, self.data); + complete(env, status as napi_status, data); - // SAFETY: env is valid for the duration of this call. - let env_ref = unsafe { &*env }; if let Some(exception) = env_ref.get_and_clear_pending_exception() { let _ = vm.uncaught_exception(global, exception, false); } else if global.has_exception() { @@ -2172,11 +2223,13 @@ extern "C" fn napi_create_async_work( extern "C" fn napi_delete_async_work(env_: napi_env, work_: *mut napi_async_work) -> napi_status { bun_output::scoped_log!(napi, "napi_delete_async_work"); let env = get_env!(env_); - // SAFETY: `work_` is null or the `napi_async_work` we allocated in `napi_create_async_work`. - let Some(work) = (unsafe { work_.as_mut() }) else { + if work_.is_null() { return env.invalid_arg(); - }; - debug_assert!(core::ptr::eq(env.to_js(), work.global.as_ptr())); + } + // SAFETY: non-null `work_` is the work `napi_create_async_work` allocated. + debug_assert!(core::ptr::eq(env.to_js(), unsafe { + (*work_).global.as_ptr() + })); napi_async_work::destroy(work_); env.ok() } @@ -2185,12 +2238,15 @@ extern "C" fn napi_delete_async_work(env_: napi_env, work_: *mut napi_async_work extern "C" fn napi_queue_async_work(env_: napi_env, work_: *mut napi_async_work) -> napi_status { bun_output::scoped_log!(napi, "napi_queue_async_work"); let env = get_env!(env_); - // SAFETY: `work_` is null or the `napi_async_work` we allocated in `napi_create_async_work`. - let Some(work) = (unsafe { work_.as_mut() }) else { + if work_.is_null() { return env.invalid_arg(); - }; - debug_assert!(core::ptr::eq(env.to_js(), work.global.as_ptr())); - work.schedule(); + } + // SAFETY: non-null `work_` is the work `napi_create_async_work` allocated. + debug_assert!(core::ptr::eq(env.to_js(), unsafe { + (*work_).global.as_ptr() + })); + // SAFETY: as above. + unsafe { napi_async_work::schedule(work_) }; env.ok() } @@ -2198,12 +2254,16 @@ extern "C" fn napi_queue_async_work(env_: napi_env, work_: *mut napi_async_work) extern "C" fn napi_cancel_async_work(env_: napi_env, work_: *mut napi_async_work) -> napi_status { bun_output::scoped_log!(napi, "napi_cancel_async_work"); let env = get_env!(env_); - // SAFETY: `work_` is null or the `napi_async_work` we allocated in `napi_create_async_work`. - let Some(work) = (unsafe { work_.as_mut() }) else { + if work_.is_null() { return env.invalid_arg(); - }; - debug_assert!(core::ptr::eq(env.to_js(), work.global.as_ptr())); - if work.cancel() { + } + // SAFETY: non-null `work_` is the work `napi_create_async_work` allocated; + // the pool thread may be running it, so only individual fields are touched. + debug_assert!(core::ptr::eq(env.to_js(), unsafe { + (*work_).global.as_ptr() + })); + // SAFETY: as above. + if unsafe { napi_async_work::cancel(work_) } { return env.ok(); } diff --git a/test/internal/source-lints/self-receiver-intrusive-post.test.ts b/test/internal/source-lints/self-receiver-intrusive-post.test.ts new file mode 100644 index 000000000000..0e0644ad5932 --- /dev/null +++ b/test/internal/source-lints/self-receiver-intrusive-post.test.ts @@ -0,0 +1,399 @@ +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 method must not post its own receiver through the intrusive task it +// embeds. Inside a `&self` / `&mut self` method, the receiver's address +// +// self.concurrent_task.from(std::ptr::from_mut(self), AutoDeinit::ManualDeinit) +// let p: *mut Self = self; ... self.concurrent_task.from(p, ..) +// fn post(&mut self, p: *mut Self) { self.concurrent_task.from(p, ..) } // p is self, by contract +// +// as the first argument of the intrusive `.from(..)` initializer +// (`ConcurrentTask::from`, `AnyTaskWithExtraContext::from`: "make the task +// embedded in this object carry this object") is banned. +// +// The `.from(..)` is the start of the hand-over: the task it fills in is +// posted next, and from the moment the post lands the consumer owns the +// object. For the pool-thread completions that use this form the consumer is +// the JS thread, and what it does with the object is usually to free it +// (`napi_async_work`: the addon's `complete` callback calls +// `napi_delete_async_work`; the S3 and shell tasks `heap::take` themselves), +// while the `&mut self` of the method that posted is still a live argument. +// A reference argument is protected for the duration of its call, and +// writing to or deallocating protected memory is UB under both aliasing +// models whether or not the method touches `self` again: Tree Borrows (what +// `bun run rust:miri` uses) reports "deallocation through is forbidden +// ... the strongly protected tag disallows deallocations", pointing at the +// receiver, and Stacked Borrows reports "deallocating while item [Unique] is +// strongly protected". Codegen relies on the same thing: the argument is +// annotated dereferenceable for the whole call. `napi_async_work::run(&mut +// self)` / `post_to_js_thread(&mut self, self_ptr: *mut Self)` were the +// instance this was written for: every async work completion posted the work +// from inside two such frames. +// +// The object was a raw pointer before it became `self` (the work-pool task +// pointer, the callback ctx), so the fix is to keep it one: the function that +// posts takes `this: *mut Self`, reads what it needs through statement-scoped +// `(*this).field` accesses, clones the handle it posts through out of the +// object (posting through `(*this).loop_handle` would protect a reference +// into the allocation for the duration of the post, the same bug one level +// down), and ends with `(*this).concurrent_task.from(this, ..)`. Templates: +// `napi_async_work::run` in src/runtime/napi/napi_body.rs, +// `S3HttpSimpleTask::http_callback` in src/runtime/webcore/s3/simple_request.rs, +// `ShellAsyncTask` in src/runtime/shell/states/Async.rs. +// +// Scope: the `.from(` initializer with the receiver's address as its first +// argument, where "the receiver's address" is one of the spellings below +// (applied to `self` or to a reborrow of it), a local of the same function +// bound to one, or a parameter of a method that also has a reference receiver +// and is typed as a raw pointer to the method's own type, spelled `Self` or by +// the enclosing impl's name (the only thing such a parameter can be is the +// receiver, handed in separately because the reference cannot be posted). The +// spelling list is the same one self-receiver-reclaim.test.ts uses; a change +// to one belongs in the other too. The heap-task constructors +// (`Task::init`, `ConcurrentTask::create_from`, `from_callback`) are the same +// hazard in a different spelling and a separate population (#37723); a +// pointer produced by a helper (`self.as_ptr()`) and reference parameters +// other than `self` are outside this 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)); +})(); + +// The intrusive initializer: a method call, so `Foo::from(x)` (a `From` impl) +// does not count. `\s*` around the paren so a rustfmt-wrapped call still +// matches. +const POST = String.raw`\.\s*from\s*\(\s*`; + +// Pointer-to-pointer conversions that keep the address: `.cast()`, +// `.cast_mut()`, `.as_ptr()` (how a `NonNull` binding is passed). +const SAME_ADDRESS = String.raw`(?:\s*\.\s*(?:cast(?:_mut|_const)?(?:::<[^>]*>)?|as_ptr)\(\))*`; + +// `self` or a reborrow of it (`&mut *self`, `&*self`), as the operand of the +// pointer conversions below. `(?!\s*\.)` keeps `&mut *self.field` (something +// the receiver owns) out. +const SELF_OPERAND = String.raw`(?:&\s*(?:mut\s+)?\*\s*)?self\b(?!\s*\.)`; + +// The conversions that turn the receiver into its address. Shared between the +// inline and the `let`-bound forms below; the self-test at the bottom pins +// each spelling. `(?!\s*\.)` after the reborrow forms keeps `&raw mut +// *self.field` out, as above. +const ADDRESS_OF_SELF = [ + String.raw`(?:[\w:]+::)?from_(?:mut|ref)(?:::<[^>]*>)?\(\s*${SELF_OPERAND}\s*\)`, + String.raw`(?:[\w:]+::)?NonNull::from\(\s*${SELF_OPERAND}\s*\)`, + String.raw`(?:[\w:]+::)?addr_of(?:_mut)?!\s*\(\s*\*\s*self\s*\)`, + String.raw`&\s*(?:raw\s+(?:mut|const)|mut)\s+\*\s*self\b(?!\s*\.)`, +]; + +// Inline: one of the conversions, `self as *mut _`, or bare `self` (a `&mut +// Self` coerces to `*mut Self` at the call; it needs the `,` / `)` so +// `self.field` does not match). +const SELF_AS_POINTER = [...ADDRESS_OF_SELF, String.raw`self\s+as\s+\*(?:mut|const)\b`, String.raw`self\s*[,)]`].join( + "|", +); + +const DIRECT = new RegExp(`${POST}(?:${SELF_AS_POINTER})`, "g"); + +// A local bound to the receiver's address: `let p = ptr::from_mut(self);`, +// `let p = NonNull::from(&mut *self);`, `let p = self as *mut Self;`, or the +// coercion spelling `let p: *mut Self = self;` (which needs the annotation; +// without it `let p = self;` is just another reference). +const BINDING_HEAD = String.raw`let\s+(?:mut\s+)?(\w+)\s*`; +const SELF_POINTER_BINDINGS = [ + new RegExp( + BINDING_HEAD + + String.raw`(?::[^=;]*)?=\s*(?:` + + [...ADDRESS_OF_SELF, String.raw`self\s+as\s+\*(?:mut|const)\b[^;.]*`].join("|") + + String.raw`)${SAME_ADDRESS}\s*;`, + "g", + ), + new RegExp(BINDING_HEAD + String.raw`:\s*\*(?:mut|const)\b[^=;]*=\s*self\s*;`, "g"), +]; + +// A function item, however qualified. Also where the previous function's +// body is taken to end (a closure inside a function is part of it for this +// purpose). Two instances because a `g` regex carries its position between +// uses: `FN_ITEMS` is only ever iterated with `matchAll`, `FN_ITEM` only used +// with `search`. +const FN_ITEM_SOURCE = String.raw`^[ \t]*(?:pub(?:\([^)]*\))?\s+)?(?:(?:const|async|unsafe|extern\s+"[^"]*")\s+)*fn\s+\w+`; +const FN_ITEM = new RegExp(FN_ITEM_SOURCE, "m"); +const FN_ITEMS = new RegExp(FN_ITEM_SOURCE, "gm"); +const RECEIVER_PARAM = /^\s*(?:&(?:'\w+\s+)?(?:mut\s+)?self\b|(?:mut\s+)?self\s*:\s*(?:&|Pin<))/; + +// `impl Foo`, `impl Foo`, `impl Trait for Foo`: the type `Self` stands +// for in the methods that follow, so a parameter typed `*mut Foo` inside them +// counts like `*mut Self`. A `trait` item starts a region where only `Self` +// is known. +const IMPL_OR_TRAIT_ITEMS = + /^[ \t]*(?:pub(?:\([^)]*\))?\s+)?(?:unsafe\s+)?(?:impl(?:<[^>]*>)?\s+(?:[\w:]+(?:<[^>]*>)?\s+for\s+)?([\w:]+)|trait\s+\w+)/gm; + +/** A parameter typed `*mut Self` / `*const Self` (or the enclosing impl's type), capturing its name. */ +function selfPointerParams(implType: string | null): RegExp { + const types = implType === null ? String.raw`Self\b(?!\s*::)` : String.raw`(?:Self\b(?!\s*::)|${implType}\b)`; + return new RegExp(String.raw`\b(\w+)\s*:\s*\*(?:mut|const)\s+${types}`, "g"); +} + +function postOf(name: string): RegExp { + return new RegExp(POST + String.raw`\b${name}\b${SAME_ADDRESS}\s*[,)]`); +} + +/** The text after `offset` up to the next function item. */ +function restOfFunction(stripped: string, offset: number): string { + const rest = stripped.slice(offset); + const end = rest.search(FN_ITEM); + return end === -1 ? rest : rest.slice(0, end); +} + +/** For each `impl` / `trait` item: where it starts and the implementing type's last path segment (null for traits). */ +function implRegions(stripped: string): { start: number; implType: string | null }[] { + return [...stripped.matchAll(IMPL_OR_TRAIT_ITEMS)].map(m => ({ + start: m.index, + implType: m[1] === undefined ? null : m[1].slice(m[1].lastIndexOf(":") + 1), + })); +} + +/** + * For every method with a reference receiver and a parameter that is a raw + * pointer to its own type: the parameter names and the offset of the + * method's body. + */ +function* receiverTwice(stripped: string): Generator<{ names: string[]; bodyStart: number }> { + const regions = implRegions(stripped); + for (const item of stripped.matchAll(FN_ITEMS)) { + const region = regions.findLast(r => r.start < item.index); + let i = item.index + item[0].length; + // Skip the generic parameter list, which may itself contain parens + // (`fn f u8>(`); `->` inside it is not a closing angle. + if (/^\s*" && stripped[i - 1] !== "-" && --depth === 0) { + i++; + break; + } + } + } + const paramsOpen = stripped.indexOf("(", i); + if (paramsOpen === -1 || stripped.slice(i, paramsOpen).trim() !== "") continue; + // Parameter types may contain parens too (`cb: fn(*mut Task)`), so find + // the list's own closing paren by depth. + let close = -1; + for (let depth = 0, j = paramsOpen; j < stripped.length; j++) { + const c = stripped[j]; + if (c === "(") depth++; + else if (c === ")" && --depth === 0) { + close = j; + break; + } + } + if (close === -1) continue; + const params = stripped.slice(paramsOpen + 1, close); + if (!RECEIVER_PARAM.test(params)) continue; + const names = [...params.matchAll(selfPointerParams(region?.implType ?? null))].map(m => m[1]); + if (names.length > 0) yield { names, bodyStart: close + 1 }; + } +} + +/** Byte offsets (into `stripped`) of every banned post in one file. */ +function findPosts(stripped: string): number[] { + const hits = new Set(); + for (const m of stripped.matchAll(DIRECT)) hits.add(m.index); + for (const pattern of SELF_POINTER_BINDINGS) { + for (const binding of stripped.matchAll(pattern)) { + const start = binding.index + binding[0].length; + const post = restOfFunction(stripped, start).search(postOf(binding[1])); + if (post !== -1) hits.add(start + post); + } + } + for (const { names, bodyStart } of receiverTwice(stripped)) { + const body = restOfFunction(stripped, bodyStart); + for (const name of names) { + const post = body.search(postOf(name)); + if (post !== -1) hits.add(bodyStart + post); + } + } + return [...hits].sort((a, b) => a - b); +} + +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, with a stated reason. Empty by design: the whole tree is at zero. +// Prefer converting over adding an entry here. +const ALLOW: Record = {}; + +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 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, ""); + for (const offset of findPosts(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", () => { + // 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 match the banned spellings and nothing else", () => { + const banned = [ + // `napi_async_work::run` / `post_to_js_thread` as they were: the address + // is taken in one method and posted by the other, which receives it next + // to its own `&mut self`. One hit, at the post. + [ + " fn run(&mut self) {", + " let self_ptr: *mut Self = self;", + " let handle = self.loop_handle.clone();", + " (self.execute)(self.env.get(), self.data);", + " self.post_to_js_thread(self_ptr);", + " handle.embedded_work_finished();", + " }", + "", + " fn post_to_js_thread(&mut self, self_ptr: *mut Self) {", + " let ct = core::ptr::NonNull::from(", + " self.concurrent_task", + " .from(self_ptr, AutoDeinit::ManualDeinit),", + " );", + " let Posted::Queued = self.loop_handle.post_task(ct) else { unreachable!() };", + " }", + ].join("\n"), + // The same two methods with the parameter typed by the impl's name + // instead of `Self`. + [ + "impl napi_async_work {", + " fn run(&mut self) {", + " let self_ptr: *mut napi_async_work = self;", + " self.post_to_js_thread(self_ptr);", + " }", + "", + " fn post_to_js_thread(&mut self, work: *mut napi_async_work) {", + " let ct = NonNull::from(self.concurrent_task.from(work, AutoDeinit::ManualDeinit));", + " }", + "}", + ].join("\n"), + "impl Worker {\n fn post(&mut self, me: *const Worker) {\n self.ct.from(me.cast_mut(), AutoDeinit::ManualDeinit);\n }\n}", + "unsafe impl crate::Postable for shell::RmTask {\n fn post(&mut self, me: *mut RmTask) {\n self.ct.from(me, AutoDeinit::ManualDeinit);\n }\n}", + // The same thing inlined into one method. + "fn run(&mut self) {\n let self_ptr: *mut Self = self;\n let ct = NonNull::from(self.concurrent_task.from(self_ptr, AutoDeinit::ManualDeinit));\n}", + // Taking the address of a reborrow of the receiver is taking the + // receiver's address. + "fn run(&mut self) {\n let self_ptr = NonNull::from(&mut *self);\n ct.from(self_ptr.as_ptr(), AutoDeinit::ManualDeinit);\n}", + "fn run(&mut self) {\n let p = core::ptr::from_mut(&mut *self);\n ct.from(p, AutoDeinit::ManualDeinit);\n}", + "fn run(&mut self) {\n let p = std::ptr::from_ref(&*self).cast_mut();\n ct.from(p, AutoDeinit::ManualDeinit);\n}", + "fn run(&mut self) {\n let p = &mut *self;\n ct.from(p, AutoDeinit::ManualDeinit);\n}", + "ct.from(&mut *self, AutoDeinit::ManualDeinit)", + "ct.from(std::ptr::from_mut(&mut *self), AutoDeinit::ManualDeinit)", + "ct.from(NonNull::from(&mut *self).as_ptr(), AutoDeinit::ManualDeinit)", + "fn run(&mut self) {\n let this = std::ptr::from_mut::(self);\n unsafe { (*this).concurrent_task.from(this, AutoDeinit::ManualDeinit) };\n}", + "fn run(&mut self) {\n let p = self as *mut Self;\n self.task.from(p, Self::run_from_main_thread_mini);\n}", + "fn run(&mut self) {\n let p = std::ptr::from_ref(self).cast_mut();\n ct.from(p, AutoDeinit::ManualDeinit);\n}", + "fn run(&mut self) {\n let p = NonNull::from(self);\n ct.from(p.as_ptr(), AutoDeinit::ManualDeinit);\n}", + "fn run(&mut self) {\n let p = &raw mut *self;\n if done {\n return;\n }\n ct.from(p, AutoDeinit::ManualDeinit);\n}", + // Spelled inline. `ct` is a task reached some other way, since borrowck + // rejects `self.concurrent_task.from(self, ..)`. + "ct.from(self, AutoDeinit::ManualDeinit)", + "ct.from(std::ptr::from_mut(self), AutoDeinit::ManualDeinit)", + "ct.from(core::ptr::from_mut::(self), AutoDeinit::ManualDeinit)", + "ct.from(self as *mut Self, AutoDeinit::ManualDeinit)", + "ct.from(&raw mut *self, AutoDeinit::ManualDeinit)", + "ct.from(core::ptr::addr_of_mut!(*self), AutoDeinit::ManualDeinit)", + "ct.from(NonNull::from(self).as_ptr(), AutoDeinit::ManualDeinit)", + "at.from(\n std::ptr::from_mut(self),\n Self::run_from_main_thread_mini,\n)", + // A reference receiver plus a pointer parameter, in the other receiver + // and header spellings, posting the parameter. + "fn post(&self, this: *mut Self) {\n self.task.with_mut(|ct| ct.from(this, AutoDeinit::ManualDeinit));\n}", + "pub(crate) unsafe fn post u8>(\n &mut self,\n f: F,\n work: *const Self,\n) -> bool {\n self.ct.from(work.cast_mut(), AutoDeinit::ManualDeinit);\n true\n}", + "fn post(&'a mut self, cb: fn(*mut Task), this: *mut Self) {\n self.ct.from(this, cb);\n}", + "fn post(self: Pin<&mut Self>, this: *mut Self) {\n self.ct.from(this, AutoDeinit::AutoDeinit);\n}", + ]; + const allowed = [ + // The converted shape: the pointer comes in as the only handle on the + // object. + "unsafe fn run(this: *mut Self) {\n let handle = unsafe { (*this).loop_handle.clone() };\n let ct = NonNull::from(unsafe { (*this).concurrent_task.from(this, AutoDeinit::ManualDeinit) });\n handle.post_task(ct);\n}", + "pub(crate) fn http_callback(this: *mut Self, result: Result<'_>) {\n unsafe { (*this).concurrent_task.from(this, AutoDeinit::ManualDeinit) };\n}", + "fn run(task: *mut Self) {\n let ct = unsafe { (*task).concurrent_task.from(task, AutoDeinit::ManualDeinit) };\n}", + "EventLoopTask::Mini(at) => at.from(this, Self::run_from_main_thread_mini),", + // Posting something the receiver owns or points at is not this shape. + "self.concurrent_task.from(self.child, AutoDeinit::ManualDeinit)", + "ct.from(&raw mut *self.inner, AutoDeinit::ManualDeinit)", + "ct.from(&mut *self.inner, AutoDeinit::ManualDeinit)", + "ct.from(std::ptr::from_mut(&mut *self.inner), AutoDeinit::ManualDeinit)", + "fn run(&mut self) {\n let p = NonNull::from(&mut *self.inner);\n ct.from(p.as_ptr(), AutoDeinit::ManualDeinit);\n}", + "ct.from(self.as_ptr(), AutoDeinit::ManualDeinit)", + // A pointer to some other type, or to a type that is only the impl's + // type by name in a different region of the file. + "impl Scheduler {\n fn post(&mut self, job: *mut Job) {\n self.ct.from(job, AutoDeinit::ManualDeinit);\n }\n}", + "impl Job {\n fn id(&self) -> u32 {\n self.id\n }\n}\n\ntrait Poster {\n fn post(&mut self, job: *mut Job) {\n self.ct().from(job, AutoDeinit::ManualDeinit);\n }\n}", + // `from` methods that are not the task initializer, and `From` impls. + ".reader()\n .from(buffered_reader, ctx_ptr.cast::());", + "let s = String::from(self.name);", + "Self::from(self)", + '"