src: fix use-after-free in CleanupHookThunkRun by everett1992 · Pull Request #65630 · nodejs/node

everett1992 · 2026-08-28T23:51:00Z

This PR is a continuation of @sreehariannam's work in

I've added a regression test and fixed the lint issue. I'm only opening this because there's no activity on that PR, and this bug is blocking the backport of #65042 which fixes a crash with better-sqlite3 on node v24.19.0

Fixes: #65195

codecov · 2026-08-29T18:10:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.07%. Comparing base (f9ab994) to head (fd325a7).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #65630   +/-   ##
=======================================
  Coverage   90.07%   90.07%           
=======================================
  Files         754      754           
  Lines      256395   256398    +3     
  Branches    48494    48495    +1     
=======================================
+ Hits       230947   230954    +7     
+ Misses      16563    16561    -2     
+ Partials     8885     8883    -2

Files with missing lines	Coverage Δ
src/api/hooks.cc	`88.19% <100.00%> (-0.41%)`	⬇️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

CleanupHookThunkRun() read thunk->isolate/fun/arg from the CleanupHookThunk after invoking thunk->fun(). For every node::ObjectWrap alive at teardown, thunk->fun is ObjectWrap::CleanupHook, which deletes the wrap; ~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, erasing the CleanupHookThunk from the registry and freeing the node it lives in. The subsequent read of thunk->isolate/fun/arg to make the (now redundant) second RemoveEnvironmentCleanupHook() call was therefore a use-after-free. Cache the fields before running the hook so nothing is read from `thunk` once it may have been freed. Taken over from nodejs#65196, which has been inactive; the original change is unmodified apart from the added comment. This also unblocks nodejs#65042, the backport of the cleanup hook registry to v24.x. Without that registry ~ObjectWrap() asserts during garbage collection, so every 24.x runtime aborts for ObjectWrap addons (nodejs#65446), as do 26.x runtimes before 26.4.0 when used with newer headers (nodejs#65262). Fixes: nodejs#65195 Refs: nodejs#65196 Refs: nodejs#65042 Refs: nodejs#65446 Refs: nodejs#65262 Assisted-by: a closed-source coding agent Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com>

Add a cctest that registers an environment cleanup hook which removes itself while the cleanup queue is drained. It exercises CleanupHookThunkRun(), which must not read the CleanupHookThunk after invoking the hook, because the hook has already erased and freed it. The hook is registered directly rather than through node::ObjectWrap. ObjectWrap is what makes this reachable for addons since nodejs#63642, because its destructor removes its own hook, and nodejs#65195 reproduces the fault that way with test/addons/worker-addon-exit. That reproducer needs an addon build and depends on when the wrapper is collected, whereas this test drives the self-removal directly. The use-after-free is silent in ordinary builds and is caught by the ASan/Valgrind CI, which is how the original assertion (nodejs#63923) surfaced. Verified locally with an ASan build: without the preceding commit both this test and test/addons/worker-addon-exit report heap-use-after-free in CleanupHookThunkRun(); both are clean with it. Refs: nodejs#65195 Refs: nodejs#65196 Assisted-by: a closed-source coding agent Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Co-authored-by: nsavoire <19255994+nsavoire@users.noreply.github.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com>

nodejs-github-bot · 2026-09-02T05:32:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/76871/

nodejs-github-bot · 2026-09-04T16:56:45Z

Landed in 03e2b9b

The registry behind `AddEnvironmentCleanupHook()` is keyed on {isolate, fun, arg} and asserts that every insertion is unique. Two Environments on one isolate that register the same hook, which the Node-API documentation allows per environment, abort the process on the second `napi_add_env_cleanup_hook()`. Key the registry on `arg` only and tell entries apart by Environment: adding the same hook to one Environment twice still aborts as documented, and removal prefers the current Environment's registration, falling back to a matching one from another Environment when there is no current context. Because the entry to remove after a hook has run can no longer be found by {isolate, fun, arg} alone, `CleanupHookThunkRun()` marks its entry as running and erases exactly that entry afterwards; a removal of a running entry (a hook removing itself, as `~ObjectWrap()` does) is a no-op, which keeps the use-after-free fixed by nodejs#65630 fixed. Refs: nodejs#63985 Refs: nodejs#65630 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

CleanupHookThunkRun() read thunk->isolate/fun/arg from the CleanupHookThunk after invoking thunk->fun(). For every node::ObjectWrap alive at teardown, thunk->fun is ObjectWrap::CleanupHook, which deletes the wrap; ~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, erasing the CleanupHookThunk from the registry and freeing the node it lives in. The subsequent read of thunk->isolate/fun/arg to make the (now redundant) second RemoveEnvironmentCleanupHook() call was therefore a use-after-free. Cache the fields before running the hook so nothing is read from `thunk` once it may have been freed. Taken over from #65196, which has been inactive; the original change is unmodified apart from the added comment. This also unblocks #65042, the backport of the cleanup hook registry to v24.x. Without that registry ~ObjectWrap() asserts during garbage collection, so every 24.x runtime aborts for ObjectWrap addons (#65446), as do 26.x runtimes before 26.4.0 when used with newer headers (#65262). Fixes: #65195 Refs: #65196 Refs: #65042 Refs: #65446 Refs: #65262 Assisted-by: a closed-source coding agent Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com> PR-URL: #65630 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>

The registry behind `AddEnvironmentCleanupHook()` is keyed on {isolate, fun, arg} and asserts that every insertion is unique. Two Environments on one isolate that register the same hook, which the Node-API documentation allows per environment, abort the process on the second `napi_add_env_cleanup_hook()`. Key the registry on `arg` only and tell entries apart by Environment: adding the same hook to one Environment twice still aborts as documented, and removal prefers the current Environment's registration, falling back to a matching one from another Environment when there is no current context. Because the entry to remove after a hook has run can no longer be found by {isolate, fun, arg} alone, `CleanupHookThunkRun()` marks its entry as running and erases exactly that entry afterwards; a removal of a running entry (a hook removing itself, as `~ObjectWrap()` does) is a no-op, which keeps the use-after-free fixed by nodejs#65630 fixed. Refs: nodejs#63985 Refs: nodejs#65630 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

CleanupHookThunkRun() read thunk->isolate/fun/arg from the CleanupHookThunk after invoking thunk->fun(). For every node::ObjectWrap alive at teardown, thunk->fun is ObjectWrap::CleanupHook, which deletes the wrap; ~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, erasing the CleanupHookThunk from the registry and freeing the node it lives in. The subsequent read of thunk->isolate/fun/arg to make the (now redundant) second RemoveEnvironmentCleanupHook() call was therefore a use-after-free. Cache the fields before running the hook so nothing is read from `thunk` once it may have been freed. Taken over from nodejs#65196, which has been inactive; the original change is unmodified apart from the added comment. This also unblocks nodejs#65042, the backport of the cleanup hook registry to v24.x. Without that registry ~ObjectWrap() asserts during garbage collection, so every 24.x runtime aborts for ObjectWrap addons (nodejs#65446), as do 26.x runtimes before 26.4.0 when used with newer headers (nodejs#65262). Fixes: nodejs#65195 Refs: nodejs#65196 Refs: nodejs#65042 Refs: nodejs#65446 Refs: nodejs#65262 Assisted-by: a closed-source coding agent Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com> PR-URL: nodejs#65630 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>

The registry behind `AddEnvironmentCleanupHook()` is keyed on {isolate, fun, arg} and asserts that every insertion is unique. Two Environments on one isolate that register the same hook, which the Node-API documentation allows per environment, abort the process on the second `napi_add_env_cleanup_hook()`. Key the registry on `arg` only and tell entries apart by Environment: adding the same hook to one Environment twice still aborts as documented, and removal prefers the current Environment's registration, falling back to a matching one from another Environment when there is no current context. Because the entry to remove after a hook has run can no longer be found by {isolate, fun, arg} alone, `CleanupHookThunkRun()` marks its entry as running and erases exactly that entry afterwards; a removal of a running entry (a hook removing itself, as `~ObjectWrap()` does) is a no-op, which keeps the use-after-free fixed by nodejs#65630 fixed. Refs: nodejs#63985 Refs: nodejs#65630 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

CleanupHookThunkRun() read thunk->isolate/fun/arg from the CleanupHookThunk after invoking thunk->fun(). For every node::ObjectWrap alive at teardown, thunk->fun is ObjectWrap::CleanupHook, which deletes the wrap; ~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, erasing the CleanupHookThunk from the registry and freeing the node it lives in. The subsequent read of thunk->isolate/fun/arg to make the (now redundant) second RemoveEnvironmentCleanupHook() call was therefore a use-after-free. Cache the fields before running the hook so nothing is read from `thunk` once it may have been freed. Taken over from #65196, which has been inactive; the original change is unmodified apart from the added comment. This also unblocks #65042, the backport of the cleanup hook registry to v24.x. Without that registry ~ObjectWrap() asserts during garbage collection, so every 24.x runtime aborts for ObjectWrap addons (#65446), as do 26.x runtimes before 26.4.0 when used with newer headers (#65262). Fixes: #65195 Refs: #65196 Refs: #65042 Refs: #65446 Refs: #65262 Assisted-by: a closed-source coding agent Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com> PR-URL: #65630 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>

* chore: squash the perfetto backports into one Node.js patch The perfetto trace agent came in as six separate cherry-picks of nodejs/node#64565 and nodejs/node#64721. Both are marked dont-land-on-v24.x upstream, so they stay for as long as Electron ships Node.js 24; carry them as one patch. Likewise fold the CleanupHookThunkRun regression test into the patch for its fix, matching nodejs/node#65630. No change to the patched tree. * chore: keep the test commit's trailers on the folded cleanup-hook patch

…#53990) * chore: squash the perfetto backports into one Node.js patch The perfetto trace agent came in as six separate cherry-picks of nodejs/node#64565 and nodejs/node#64721. Both are marked dont-land-on-v24.x upstream, so they stay for as long as Electron ships Node.js 24; carry them as one patch. Likewise fold the CleanupHookThunkRun regression test into the patch for its fix, matching nodejs/node#65630. No change to the patched tree. * chore: keep the test commit's trailers on the folded cleanup-hook patch

The registry behind `AddEnvironmentCleanupHook()` is keyed on {isolate, fun, arg} and asserts that every insertion is unique. Two Environments on one isolate that register the same hook, which the Node-API documentation allows per environment, abort the process on the second `napi_add_env_cleanup_hook()`. Key the registry on `arg` only and tell entries apart by Environment: adding the same hook to one Environment twice still aborts as documented, and removal prefers the current Environment's registration, falling back to a matching one from another Environment when there is no current context. Because the entry to remove after a hook has run can no longer be found by {isolate, fun, arg} alone, `CleanupHookThunkRun()` marks its entry as running and erases exactly that entry afterwards; a removal of a running entry (a hook removing itself, as `~ObjectWrap()` does) is a no-op, which keeps the use-after-free fixed by nodejs#65630 fixed. Refs: nodejs#63985 Refs: nodejs#65630 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

CleanupHookThunkRun() read thunk->isolate/fun/arg from the CleanupHookThunk after invoking thunk->fun(). For every node::ObjectWrap alive at teardown, thunk->fun is ObjectWrap::CleanupHook, which deletes the wrap; ~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, erasing the CleanupHookThunk from the registry and freeing the node it lives in. The subsequent read of thunk->isolate/fun/arg to make the (now redundant) second RemoveEnvironmentCleanupHook() call was therefore a use-after-free. Cache the fields before running the hook so nothing is read from `thunk` once it may have been freed. Taken over from nodejs#65196, which has been inactive; the original change is unmodified apart from the added comment. This also unblocks nodejs#65042, the backport of the cleanup hook registry to v24.x. Without that registry ~ObjectWrap() asserts during garbage collection, so every 24.x runtime aborts for ObjectWrap addons (nodejs#65446), as do 26.x runtimes before 26.4.0 when used with newer headers (nodejs#65262). Fixes: nodejs#65195 Refs: nodejs#65196 Refs: nodejs#65042 Refs: nodejs#65446 Refs: nodejs#65262 Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com> PR-URL: nodejs#65630 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com> Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex

The registry behind `AddEnvironmentCleanupHook()` is keyed on {isolate, fun, arg} and asserts that every insertion is unique. Two Environments on one isolate that register the same hook, which the Node-API documentation allows per environment, abort the process on the second `napi_add_env_cleanup_hook()`. Key the registry on `arg` only and tell entries apart by Environment: adding the same hook to one Environment twice still aborts as documented, and removal prefers the current Environment's registration, falling back to a matching one from another Environment when there is no current context. Because the entry to remove after a hook has run can no longer be found by {isolate, fun, arg} alone, `CleanupHookThunkRun()` marks its entry as running and erases exactly that entry afterwards; a removal of a running entry (a hook removing itself, as `~ObjectWrap()` does) is a no-op, which keeps the use-after-free fixed by #65630 fixed. Refs: #63985 Refs: #65630 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com> PR-URL: #65777 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

CleanupHookThunkRun() read thunk->isolate/fun/arg from the CleanupHookThunk after invoking thunk->fun(). For every node::ObjectWrap alive at teardown, thunk->fun is ObjectWrap::CleanupHook, which deletes the wrap; ~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, erasing the CleanupHookThunk from the registry and freeing the node it lives in. The subsequent read of thunk->isolate/fun/arg to make the (now redundant) second RemoveEnvironmentCleanupHook() call was therefore a use-after-free. Cache the fields before running the hook so nothing is read from `thunk` once it may have been freed. Taken over from #65196, which has been inactive; the original change is unmodified apart from the added comment. This also unblocks #65042, the backport of the cleanup hook registry to v24.x. Without that registry ~ObjectWrap() asserts during garbage collection, so every 24.x runtime aborts for ObjectWrap addons (#65446), as do 26.x runtimes before 26.4.0 when used with newer headers (#65262). Fixes: #65195 Refs: #65196 Refs: #65042 Refs: #65446 Refs: #65262 Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com> PR-URL: #65630 Backport-PR-URL: #66128 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 28, 2026

everett1992 marked this pull request as ready for review August 29, 2026 16:32

trivikr changed the title ~~Test cleanup hook uaf regression 65196~~ src: fix use-after-free in CleanupHookThunkRun Aug 31, 2026

trivikr mentioned this pull request Aug 31, 2026

src: fix use-after-free in CleanupHookThunkRun #65196

Closed

trivikr approved these changes Sep 1, 2026

View reviewed changes

trivikr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 1, 2026

trivikr requested a review from aduh95 September 1, 2026 02:05

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 1, 2026

This comment was marked as outdated.

Sign in to view

everett1992 and others added 2 commits September 1, 2026 09:53

everett1992 force-pushed the test-cleanup-hook-uaf-regression-65196 branch from bb42c6d to fd325a7 Compare September 1, 2026 16:55

trivikr approved these changes Sep 1, 2026

View reviewed changes

trivikr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 1, 2026

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 1, 2026

This comment was marked as outdated.

Sign in to view

trivikr added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 2, 2026

trivikr added commit-queue-squash PRs the Commit Queue should land as one squashed commit. commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 2, 2026

nodejs-github-bot added the lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. label Sep 2, 2026

codebytere approved these changes Sep 4, 2026

View reviewed changes

nodejs-github-bot merged commit 03e2b9b into nodejs:main Sep 4, 2026
84 checks passed

nodejs-github-bot removed commit-queue PRs queued for automated landing through the Commit Queue. lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. labels Sep 4, 2026

codebytere mentioned this pull request Sep 4, 2026

src: let Environments on one isolate share a cleanup hook #65777

Merged

everett1992 mentioned this pull request Sep 5, 2026

[v24.x backport] src: keep global list of addon-provided cleanup hooks #65042

Closed

github-actions Bot mentioned this pull request Sep 7, 2026

Version 26.9.0 (Current) #65881

Merged

everett1992 mentioned this pull request Sep 9, 2026

[v24.x backport] src: keep global list of addon-provided cleanup hooks #65943

Closed

codebytere mentioned this pull request Sep 16, 2026

build: squash the perfetto backports into one Node.js patch electron/electron#53990

Merged

2 tasks

panva mentioned this pull request Sep 20, 2026

[v24.x backport] crypto, sqlite, http2, and addon fixes and features #66128

Closed

panva added the backport-open-v24.x Indicate that the PR has an open backport label Sep 20, 2026

panva added backported-to-v24.x PRs backported to the v24.x-staging branch. and removed backport-open-v24.x Indicate that the PR has an open backport labels Sep 21, 2026

Navigation Menu

Uh oh!

src: fix use-after-free in CleanupHookThunkRun - #65630

src: fix use-after-free in CleanupHookThunkRun#65630
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
everett1992:test-cleanup-hook-uaf-regression-65196

everett1992 commented Aug 28, 2026 •

edited by trivikr

Loading

Uh oh!

codecov Bot commented Aug 29, 2026 •

edited

Loading

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Sep 2, 2026

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

everett1992 commented Aug 28, 2026 • edited by trivikr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Sep 2, 2026

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

everett1992 commented Aug 28, 2026 •

edited by trivikr

Loading

codecov Bot commented Aug 29, 2026 •

edited

Loading