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

sreehariannam · 2026-08-10T14:58:55Z

Summary

CleanupHookThunkRun() read thunk->isolate / thunk->fun / thunk->arg
from the CleanupHookThunk after invoking thunk->fun(). For every
node::ObjectWrap still alive at teardown, thunk->fun is
ObjectWrap::CleanupHook, which deletes the wrap. Since #63642,
~ObjectWrap() calls RemoveEnvironmentCleanupHook() itself, which erases
the CleanupHookThunk from cleanup_hook_registry and frees the node it
lives in. The subsequent read to make the (now redundant) second
RemoveEnvironmentCleanupHook() call was therefore a use-after-free — this
is now the ordinary teardown path for every ObjectWrap-based addon, not an
edge case.

The fix caches isolate/fun/arg before invoking the hook, so nothing is
read from thunk once it may have already been freed.

Root cause and fix as diagnosed in the issue.

Fixes: #65195

Test plan

test/addons/worker-addon-exit (built with node-gyp, run under
valgrind) is the existing repro described in the issue.

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. Fixes: nodejs#65195

sreehariannam · 2026-08-10T15:04:18Z

cc @addaleax (you wrote both #63642 and #63985, whose interaction causes this) and @legendecas (reviewed #63642) — would appreciate a look when you have a chance.

sreehariannam · 2026-08-10T15:05:24Z

cc @nsavoire — thanks for the thorough root-cause writeup and repro in the issue, it made this a straightforward fix to apply. If you get a chance to re-run your valgrind repro (test/addons/worker-addon-exit) against this branch, that would help confirm it before a maintainer looks at it.

aduh95 · 2026-08-13T09:21:20Z

#65195 claims the issue is 100% deterministic, shouldn't we be adding a test? Also, there's a commit linter failure to fix here

codecov · 2026-08-13T10:49:03Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.30%. Comparing base (404b0cf) to head (893ed58).
⚠️ Report is 48 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65196      +/-   ##
==========================================
- Coverage   90.32%   90.30%   -0.03%     
==========================================
  Files         760      760              
  Lines      248525   248528       +3     
  Branches    46894    46895       +1     
==========================================
- Hits       224488   224430      -58     
- Misses      15469    15522      +53     
- Partials     8568     8576       +8

Files with missing lines	Coverage Δ
src/api/hooks.cc	`89.44% <100.00%> (+0.20%)`	⬆️

... and 32 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.

8월 15일부터 CI가 7연속 빨간불이었다. Node 22는 늘 통과했고 24만 죽었다. 8월 12일 마지막 성공 런의 러너는 24.18.0이었고 첫 실패부터 24.19.0이다. 그 사이 우리 커밋은 룰북과 스킬 문서뿐이라 네이티브 쪽을 건드리지 않았다. 24.19.0이 node::ObjectWrap에 정리 훅을 붙이면서(nodejs/node#63642) better-sqlite3 11.x의 Database 소멸자가 RemoveEnvironmentCleanupHook의 CHECK_NOT_NULL(env)에서 abort한다. 스택이 정확히 그 경로다. Node 쪽 수정(nodejs/node#65196)은 아직 미머지고 24.19.0 뒤로 24.x 릴리즈도 없어서 기다릴 수가 없다. better-sqlite3 13.x가 N-API로 옮겨가며 이 경로를 없앴지만 darwin-arm64에서 Node 20/22가 깨지는 회귀(#1514)가 열려 있어 검증 없이는 못 올린다. 우선 러너를 묶어 불을 끄고 업그레이드는 따로 본다. 커버리지 업로드 조건도 함께 고친다 — 24로 비교하던 자리라 핀을 넣으면 영영 안 걸린다. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Node 24.19.0 regressed process teardown with a use-after-free in CleanupHookThunkRun, which crashes better-sqlite3 11.10.0 AVA workers in RemoveEnvironmentCleanupHook (Assertion failed: (env) != nullptr). The floating 24.x matrix advanced 24.18.0 -> 24.19.0 mid-cycle and turned 'test (24.x, ubuntu-latest)' red across unrelated PRs. Pin the four 24.x matrix entries to 24.18.0 (last known-good) so the float stops. Lift once the Node fix ships. Refs: nodejs/node#65195 Refs: nodejs/node#65196

everett1992 · 2026-08-20T15:47:34Z

I added a test and fixed the lint error - I'm happy to open new PR but didn't want to step on @sreehariannam's contribution:
main...everett1992:node:test-cleanup-hook-uaf-regression-65196
You're free to take that test, or we can add it as a follow up, or I can open the PR.

I'm seeing wide spread crashes in node v24.19.0 that should be fixed by #65042 which is blocked by this

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. Fixes: nodejs#65195 Refs: nodejs#65196 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, but the existing test/addons/worker-addon-exit does not report the use-after-free even under ASan, so exercising the self-removal directly is what catches it. 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: the test reports heap-use-after-free in CleanupHookThunkRun() without the preceding commit and passes 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>

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>

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 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 10, 2026

kriscendobot mentioned this pull request Aug 14, 2026

feat(ascii): add strict decoding and adopt for OCapN endojs/endo-but-for-bots#980

Merged

tienne mentioned this pull request Aug 15, 2026

chore(deps): better-sqlite3 13.x 업그레이드 + CI arm64 추가 tienne/gestalt#1

Merged

This was referenced Aug 16, 2026

ci: pin 24.x matrix to 24.18.0 to dodge node 24.19.0 worker crash endojs/endo-but-for-bots#999

Closed

ci: pin Node 24.x matrix to 24.18.0 (worker-teardown crash) endojs/endo-but-for-bots#1000

Closed

everett1992 mentioned this pull request Aug 25, 2026

Test cleanup hook uaf regression 65196 #65546

Closed

everett1992 mentioned this pull request Aug 28, 2026

Test cleanup hook uaf regression 65196 #65630

Open

Navigation Menu

Uh oh!

Uh oh!

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

src: fix use-after-free in CleanupHookThunkRun#65196
sreehariannam wants to merge 1 commit into
nodejs:mainfrom
sreehariannam:fix-cleanup-hook-thunk-uaf

sreehariannam commented Aug 10, 2026 •

edited

Loading

Uh oh!

sreehariannam commented Aug 10, 2026

Uh oh!

sreehariannam commented Aug 10, 2026

Uh oh!

aduh95 commented Aug 13, 2026

Uh oh!

codecov Bot commented Aug 13, 2026

Uh oh!

everett1992 commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Summary

Test plan

Uh oh!

sreehariannam commented Aug 10, 2026

Uh oh!

sreehariannam commented Aug 10, 2026

Uh oh!

aduh95 commented Aug 13, 2026

Uh oh!

codecov Bot commented Aug 13, 2026

Codecov Report

Uh oh!

everett1992 commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sreehariannam commented Aug 10, 2026 •

edited

Loading