Unsound `Send` implementation on `SendableResolver<Data, R>`

[!NOTE] This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

The Issue

The SendableResolver<Data, R> struct implements Send whenever Data: Send + 'static and R: 'static + FnOnce(sys::napi_env, Data) -> Result<sys::napi_value>. Noticeably, the closure generic parameter R is not bounded by Send or Sync.

https://github.com/napi-rs/napi-rs/blob/7f65844403ae5c23ec30e05f0480e1f468c4b65b/crates/napi/src/sendable_resolver.rs#L17-L20

Safe public code can construct a SendableResolver capturing an Rc, move the resolver across OS threads (e.g., via std::thread::spawn), and invoke resolver.resolve(...) or drop it on a secondary thread. Concurrently cloning or dropping the captured Rc across multiple threads results in non-atomic data races on reference count fields, violating Rust memory safety guarantees and causing immediate Undefined Behavior.

use std::rc::Rc;
use std::thread;
use napi::{sys, Result, SendableResolver};

fn main() {
    let rc = Rc::new(42);
    let rc_clone = rc.clone();
    let resolver = SendableResolver::new(move |_env: sys::napi_env, _data: ()| -> Result<sys::napi_value> {
        let _val = *rc_clone;
        Ok(std::ptr::null_mut())
    });

    let handle = thread::spawn(move || {
        drop(resolver);
    });

    let _rc_clone2 = rc.clone();
    let _ = handle.join();
}

error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `main` and (2) non-atomic read on thread `unnamed-1` at alloc199
   --> /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cell.rs:555:18
    |
555 |         unsafe { *self.value.get() }
    |                  ^^^^^^^^^^^^^^^^^ (2) just happened here
    |
help: and (1) occurred earlier here
   --> src/bin/repro1.rs:17:22
    |
 17 |     let _rc_clone2 = rc.clone();
    |                      ^^^^^^^^^^
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: this is on thread `unnamed-1`
    = note: stack backtrace:
            0: std::cell::Cell::<usize>::get
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cell.rs:555:18: 555:35
            1: <std::rc::RcInner<i32> as std::rc::RcInnerPtr>::strong
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/rc.rs:3759:9: 3759:32
            2: <std::rc::RcInner<i32> as std::rc::RcInnerPtr>::dec_strong
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/rc.rs:3787:31: 3787:44
            3: <std::rc::Rc<i32> as std::ops::Drop>::drop
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/rc.rs:2492:13: 2492:38
            4: std::ptr::drop_in_place::<std::rc::Rc<i32>> - shim(Some(std::rc::Rc<i32>))
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:809:1: 811:25
            5: std::ptr::drop_in_place::<{closure@src/bin/repro1.rs:8:42: 8:106}> - shim(Some({closure@src/bin/repro1.rs:8:42: 8:106}))
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:809:1: 811:25
            6: std::ptr::drop_in_place::<napi::SendableResolver<(), {closure@src/bin/repro1.rs:8:42: 8:106}>> - shim(Some(napi::SendableResolver<(), {closure@src/bin/repro1.rs:8:42: 8:106}>))
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:809:1: 811:25
            7: std::mem::drop::<napi::SendableResolver<(), {closure@src/bin/repro1.rs:8:42: 8:106}>>
                at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/mem/mod.rs:1004:1: 1004:2
            8: main::{closure#1}
                at src/bin/repro1.rs:14:9: 14:23

Add a Send bound to the closure generic parameter R in the Send trait implementation for SendableResolver<Data, R>:

-unsafe impl<Data: 'static + Send, R: 'static + FnOnce(sys::napi_env, Data) -> Result<sys::napi_value>>
+unsafe impl<Data: 'static + Send, R: 'static + Send + FnOnce(sys::napi_env, Data) -> Result<sys::napi_value>>
   Send for SendableResolver<Data, R>
 {
 }

[!NOTE] The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims.

Unsafe Rust Review: napi (v3)

Overall Safety Assessment

The napi crate (version v3) provides high-level Rust bindings and FFI wrappers around the official Node.js C Node-API (napi_sys). It aims to enable idiomatic authoring of native Node.js addons in Rust with zero-copy data sharing, class registration, async task execution (via Tokio or libuv worker pools), and threadsafe function callbacks.

Unsafe Surface & Density

Because napi serves as a massive FFI bridge across C runtime boundaries, it exhibits an exceptionally high density of unsafe code. Across the codebase (comprising over 50 source files in src/), there are hundreds of distinct unsafe {} blocks and unsafe fn declarations. The unsafe surface encompasses raw pointer dereferencing, union field accesses, manual vtable/callback dispatch, pointer casts, lifetime transmutes, and manual implementation of auto traits (Send and Sync).

Architectural Soundness & Audit Proofs

Our rigorous audit reveals severe, systematic breakdowns in safety boundaries across the crate. Rather than upholding safe abstraction encapsulation, the crate repeatedly utilizes raw pointer operations, Box::from_raw reconstructions, and auto trait implementations to force thread-unsafe or GC-dependent JavaScript objects into idiomatic Rust interfaces (Send, Sync, &'static mut T, Deref).

Most alarmingly, crate comments explicitly acknowledge that certain core implementations violate Rust memory model rules (e.g., // SAFETY: This is literally undefined behavior in src/bindgen_runtime/js_values/buffer.rs:359). Furthermore, the crate suffers from a near-total absence of formal safety documentation: almost no low-level FFI calls or unsafe trait implementations possess // SAFETY: proof comments or # Safety docstrings. The crate contains numerous critical soundness vulnerabilities that allow 100% safe Rust code to trigger instantaneous Undefined Behavior (data races, aliasing reference violations, heap/stack use-after-free, and V8 handle corruption).

Critical Findings

1. Unsound Send Implementation on SendableResolver<Data, R> (src/sendable_resolver.rs:17) ⚠️

• Priority: High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Unsound Trait Implementation

• Vulnerability: SendableResolver<Data, R> implements Send whenever Data: Send + 'static and R: 'static + FnOnce(sys::napi_env, Data) -> Result<sys::napi_value>. Noticeably, the closure generic parameter R is not bounded by Send or Sync.

• Soundness Violation: In safe Rust, closures can capture thread-unsafe, non-atomic types such as Rc<RefCell<T>>. Because SendableResolver implements Send without requiring R: Send, safe code can construct a SendableResolver capturing an Rc, move the resolver across OS threads (e.g., via std::thread::spawn), and invoke resolver.resolve(...) or drop it on a secondary thread. Concurrently cloning or dropping the captured Rc across multiple threads results in non-atomic data races on reference count fields, causing immediate Undefined Behavior.

2. Unsound Mutable Reference Aliasing via Ref<T>::get_value_mut (src/js_values/value_ref.rs:68) ⚠️

• Priority: High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Reference Aliasing Violation

• Vulnerability: Ref<T> unconditionally implements Sync (unsafe impl<T> Sync for Ref<T> {}). The safe public method get_value_mut takes &self (a shared/immutable reference) and returns Result<&mut T> (an exclusive mutable reference).

• Soundness Violation: Under Rust's authoritative borrowing rules, producing &mut T from &self without interior mutability synchronization primitives (UnsafeCell, Mutex) is inherently unsound. Because Ref<T>: Sync, safe code can share &Ref<T> across multiple threads (or retain multiple shared references on a single thread) and invoke get_value_mut simultaneously. This produces coexisting, aliasing &mut T references pointing to the exact same memory address, violating Rust Reference aliasing rules and triggering UB. (Note: The authors explicitly allowed #[allow(clippy::mut_from_ref)] to suppress static compiler diagnostics).

3. Unsound Mutable Reference Aliasing via WeakReference<T>::get_mut and Clone (src/bindgen_runtime/js_values/value_ref.rs:258) ⚠️

• Priority: High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Reference Aliasing Violation

• Vulnerability: WeakReference<T> implements Clone by performing a bitwise copy of its internal raw pointer (self.raw). It exposes pub fn get_mut(&mut self) -> Option<&mut T>, which converts self.raw into &mut T.

• Soundness Violation: Safe code can clone a WeakReference<T>, producing two distinct weak reference instances (w1 and w2) containing identical raw heap pointers. Calling w1.get_mut() and w2.get_mut() simultaneously returns two active exclusive &mut T references to the exact same underlying class allocation. Aliasing exclusive mutable references is instantaneous UB.

4. Aliasing Box<T> Construction in Deref / get / Callbacks (src/bindgen_runtime/js_values/value_ref.rs:185) ⚠️

• Priority: High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Pointer Aliasing Violation

• Vulnerability: Across Reference<T>, SharedReference<T, S>, and WeakReference<T>, methods such as Deref::deref(&self) and WeakReference::get(&self) convert shared raw pointers into references via unsafe { Box::leak(Box::from_raw(self.raw)) }. Similarly, FFI dispatch callbacks (call_js_cb and on_abort_impl) reconstruct temporary Box instances over shared context pointers.

• Soundness Violation: Constructing a Box<T> via Box::from_raw asserts unique ownership (noalias) over the target allocation under Rust pointer aliasing formalisms (Stacked Borrows and Tree Borrows). When deref is invoked on shared references (&self), asserting unique Box ownership invalidates all other coexisting borrows or raw C++ pointers derived from that allocation. Furthermore, calling deref concurrently across threads on Sync references creates coexisting aliasing Box<T> values, causing immediate UB. (These implementations must use raw pointer dereferencing &*self.raw instead).

5. Lifetime Escaping & Stack Use-After-Free in async_work::complete_impl (src/async_work.rs:149) ⚠️

• Priority: High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Stack Use-After-Free

• Vulnerability: Inside complete_impl, the async work completion handler executes work.inner_task.resolve by passing an environment reference constructed via unsafe { std::mem::transmute::<&Env, &'task Env>(&Env::from_raw(env)) }.

• Soundness Violation: Env::from_raw(env) instantiates a temporary stack-allocated Env struct inside complete_impl. Taking &Env::from_raw(...) borrows this stack temporary, and transmute illegally forces its lifetime to match the task's generic 'task parameter. In safe Rust, an implementation of ScopedTask<'task> is legally permitted to store the passed &'task Env inside self. Immediately upon return from resolve, the stack temporary is deallocated. Subsequent trait methods (such as ScopedTask::finally) accessing the stored &'task Env reference will read dead stack frame slots (stack use-after-free).

6. Heap Use-After-Free via CallbackInfo::unwrap_borrow / unwrap_borrow_mut (src/bindgen_runtime/callback_info.rs:260) ⚠️

• Priority: High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Heap Use-After-Free

• Vulnerability: CallbackInfo::unwrap_borrow and unwrap_borrow_mut unwrap raw pointers from C++ V8 callback info and return unbounded &'static T and &'static mut T references.

• Soundness Violation: The underlying Rust class object T is wrapped inside a Node.js V8 garbage-collected JavaScript object. When the JavaScript object is reclaimed by the V8 GC, Node-API triggers the finalizer hook raw_finalize_unchecked, which executes Box::from_raw(ptr) and deallocates the backing heap memory. Because unwrap_borrow returned an unbounded 'static reference to safe Rust callers, safe code can retain this reference beyond GC reclamation, resulting in dangling pointer dereferencing (heap use-after-free).

7. JS Handle Corruption vs Backing Buffer in BufferSlice Creators (src/bindgen_runtime/js_values/buffer.rs:87)

• Priority: High
• Threat Vector: Untrusted Input
• Bug Type: Memory Corruption
• Vulnerability: In BufferSlice::from_data, from_external, and copy_from, the slice field is initialized via unsafe { slice::from_raw_parts_mut(buf.cast(), len) }, where buf is the sys::napi_value handle returned by sys::napi_create_external_buffer or sys::napi_create_buffer_copy.
• Soundness Violation: In Node-API C semantics, sys::napi_value (*mut napi_value__) is an opaque pointer representing a V8 JavaScript object handle on the heap. The actual underlying byte backing store is distinct from the object handle. Casting buf (napi_value) to *mut u8 and creating a slice over it creates a Rust slice pointing directly at V8 internal object metadata headers. Reading or writing this slice corrupts JS runtime heap structures, leading to immediate VM crash. (The authors erroneously copy-pasted buf.cast() from FromNapiValue, where buf holds the extracted data buffer pointer).

8. Unsynchronized JS Mutation Data Races on Buffer Send / Sync (src/bindgen_runtime/js_values/buffer.rs:361)

• Priority: High
• Threat Vector: Untrusted Input
• Bug Type: Data Race
• Vulnerability: Buffer unconditionally implements Send and Sync (unsafe impl Send for Buffer {}).
• Soundness Violation: Buffer wraps a JavaScript Buffer (Uint8Array) backed by a shared V8 ArrayBuffer. When a Buffer is moved or shared across threads in Rust (e.g., inside an background Tokio task), safe Rust code can obtain byte slice references &[u8] or &mut [u8]. Concurrently, JavaScript running on the Node.js main thread retains active handles to the same ArrayBuffer and can mutate its contents without synchronization. Under Rust memory model rules, unsynchronized concurrent writes aliasing active &[u8] borrows constitute data races (immediate UB). Crate comments on lines 359 and 425 explicitly acknowledge this violation: // SAFETY: This is literally undefined behavior.

Fishy Findings

1. Unsound Safe Signatures on Low-Level Raw Pointer Constructors ⚠️

• Priority: Medium

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Unsound API Signature

• Location: Env::from_raw (src/env.rs:72), Reference::add_ref (src/bindgen_runtime/js_values/value_ref.rs:71), CallbackInfo::new (src/bindgen_runtime/callback_info.rs:27).

• Analysis: These low-level constructors accept raw C pointers (sys::napi_env, *mut c_void, sys::napi_callback_info) and directly dereference or manipulate internal VM tables without runtime validation. Declaring them as safe pub fn violates fundamental API safety boundaries, allowing safe callers to pass null or dangling pointers and trigger UB. The authors explicitly suppressed compiler safety lints (#[allow(clippy::missing_safety_doc)], #[allow(clippy::not_unsafe_ptr_arg_deref)]).

2. Unsynchronized Mutation of Exposed Public Statics NODE_VERSION_* ⚠️

• Priority: Medium

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Data Race

• Location: src/bindgen_runtime/module_register.rs:41

• Analysis: pub static mut NODE_VERSION_MAJOR: u32 = 0; (along with MINOR and PATCH) are mutated during runtime version detection (module_register.rs:279). Exposing mutable global statics to safe public code creates maintainer hazard and data race potential across dependent crates.

3. ZST / Dummy Pointer Overwriting in Class Factory

• Priority: Medium

• Threat Vector: Contrived Setup

• Bug Type: Destructor Skipping

• Location: src/bindgen_runtime/callback_info.rs:100

• Analysis: When constructing class instances, let mut value_ref = Box::into_raw(Box::new(obj));. If obj is a Zero-Sized Type (struct A;), value_ref is 0x1. The factory detects this and overwrites value_ref with Box::into_raw(Box::new(EmptyStructPlaceholder(0))). If the ZST implements Drop, leaking the original Box<T> and later invoking raw_finalize_unchecked on EmptyStructPlaceholder creates type mismatch and destructor skipping hazards.

Missing Safety Comments

Across napi v3, hundreds of unsafe {} blocks and unsafe fn declarations lack required // SAFETY: proof comments or # Safety docstrings. We systematically enumerate representative surfaces and proof obligations below.

Core FFI Execution (src/env.rs, src/lib.rs, src/status.rs)

• src/env.rs:80, 87, 96, 105, 112... (unsafe { sys::napi_* } FFI invocations across Env methods): Missing // SAFETY: comments.
• Proposed Proof: self.0 (sys::napi_env) is valid and active within the native addon invocation scope. Output pointers (&mut raw_value) point to valid stack memory. FFI arguments satisfy Node-API C ABI preconditions.
• src/lib.rs:193 (pub unsafe fn log_js_value): Missing # Safety docstring and internal // SAFETY: comments.
• Proposed Theorem: Precondition: env must be a valid, active Node-API environment handle. values slice must contain valid sys::napi_value handles created within env.

Value References & Deref Impls (src/bindgen_runtime/js_values/value_ref.rs)

• src/bindgen_runtime/js_values/value_ref.rs:83 (pub unsafe fn from_value_ptr): Missing # Safety docstring.
• Proposed Theorem: Precondition: t must be a valid pointer registered in REFERENCE_MAP pointing to a class wrapper allocation. env must match the creation environment.
• src/bindgen_runtime/js_values/value_ref.rs:115 (unsafe { crate::sys::napi_get_reference_value(...) }): Missing // SAFETY: comment.
• Proposed Proof: val.napi_ref is a valid Node-API reference handle. The call executes on the thread where the reference was created.

Buffer & ArrayBuffer Slices (src/bindgen_runtime/js_values/buffer.rs, arraybuffer.rs)

• **`s