napi: block worker shutdown on in-flight napi_async_work (UAF) - #36855
Closed
robobun wants to merge 6 commits into
Closed
napi: block worker shutdown on in-flight napi_async_work (UAF)#36855robobun wants to merge 6 commits into
robobun wants to merge 6 commits into
Claude / Claude Code Review
completed
Aug 3, 2026 in 23m 32s
Code review found 3 important issues
Found 3 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 3 |
| 🟡 Nit | 0 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/dispatch.rs:1334-1346 |
One-shot barrier: napi_queue_async_work from shutdown-drain complete() reintroduces the UAF |
| 🔴 Important | src/jsc/web_worker.rs:1306-1313 |
shutdown-drain complete() runs after NapiEnv::cleanup() — addon may access freed per-env state |
| 🔴 Important | src/runtime/napi/napi_body.rs:1832-1837 |
UAF: self.event_loop read after enqueue_task_concurrent may free napi_async_work |
Annotations
Check failure on line 1346 in src/runtime/dispatch.rs
claude / Claude Code Review
One-shot barrier: napi_queue_async_work from shutdown-drain complete() reintroduces the UAF
The new `NapiAsyncWork` arm runs the addon's `complete()` callback during the shutdown drain, but `complete()` (or JS it invokes via `napi_call_function`) may call `napi_queue_async_work` — which has no `is_shutting_down` guard and re-schedules onto the WorkPool after `wait_for_pending_work_pool_tasks()` has already returned. `WebWorker::shutdown` never re-checks `work_pool_pending` after the drain, so `teardownJSCVM`/`dealloc(vm_ptr)` proceed with a live pool task that will later `enqueue_task_
Check failure on line 1313 in src/jsc/web_worker.rs
claude / Claude Code Review
shutdown-drain complete() runs after NapiEnv::cleanup() — addon may access freed per-env state
The new barrier + `NapiAsyncWork` shutdown-drain arm run **after** `vm.on_exit()`, which has already executed `NapiEnv::cleanup()` (addon cleanup hooks, TSF abort, wrap finalizers, `instanceDataFinalizer`) via `rare_data.cleanup_hooks`. So the addon's `complete()` now runs against a torn-down env — e.g. `napi_get_instance_data()` returns the dangling pointer whose finalizer already freed it (napi.h:263-264 clears the finalizer but not `instanceData`). Before this PR the tag fell through to `_ =>
Check failure on line 1837 in src/runtime/napi/napi_body.rs
claude / Claude Code Review
UAF: self.event_loop read after enqueue_task_concurrent may free napi_async_work
`self.event_loop.work_pool_task_unref()` at napi_body.rs:1837 (and :1820 on the cancelled path) reads a field of `*self` after `enqueue_task_concurrent` has published `self_ptr` to the JS thread — which can immediately dispatch `run_from_js` → addon `complete` → `napi_delete_async_work` → `heap::take(self)`, freeing the box before the pool thread reads `self.event_loop`. Hoist `let el = self.event_loop;` (BackRef is Copy) before the enqueue and call `el.work_pool_task_unref()` after; before this
Loading