Follow up on #388 review comments by szegedi · Pull Request #391 · DataDog/pprof-nodejs

szegedi · 2026-08-10T14:59:31Z

Three review comments landed on #388 after it merged (@nsavoire). All three are valid; this addresses them.

1. Duplicated internal-field accessors

These functions are duplicated both here and in wall.cc.

Correct, and it was deliberate but temporary: #388 introduced internal-field.hh and left wall.cc's copies alone to avoid conflicting with #387, which was in flight at the time, with a note to fold them in afterwards. #387 has landed, so wall.cc now includes the header and its copies are gone. Same namespace and same names, so every call site is untouched.

2. Register the hook in `Init()`, drop `g_drain_hook_registered`

nit: register the environment cleanup hook in Init() and remove g_drain_hook_registered

Better, and it removes state rather than adding it. Module initialisation always runs with a context entered, so AddEnvironmentCleanupHook's own CHECK is satisfied there just as it was in Wrap(), and Init() runs exactly once per isolate — which is the lifetime the hook should match.

The flag existed only to make the lazy registration idempotent and to re-arm after an isolate was torn down and recreated on the same thread. Init() running again on the new isolate covers the second case by construction, so both reasons evaporate.

3. Clear the internal field before deleting the wrap

nit: clear JS object internal field before deleting the wrap object

Applied, and I'd argue it is more than a nit. That slot is exactly what the out-of-process OTEP-4947 reader walks to reach record_, so leaving it pointing at freed memory is a loaded gun aimed at a consumer we do not control. #388's comment claimed the dangling pointer "stays inert because nothing reads it" — true of our own code, but not of the reader.

The drain hook now nulls slot 0 on its way through the list. Being on the live list means V8 has not collected the holder, so reading the handle there is safe. The WeakCallback path cannot do this and does not need to: there the holder is the object being collected.

Verification

	result
Node 26 `npm test`	exit 0, 165 passing, 0 aborts
Node 24 `test:js-asan`	exit 0, 165 passing, 0 leaks, 0 aborts
Node 20 `test:js-asan`	exit 0, 101 passing, 0 leaks, 0 aborts
teardown regression test	passing on 24 and 26 (the OTEP block skips on 20, no ACF)
original repro, N=3000 / 10000	exit 0
published `native_wrap_fields_offset`	still 0

Local suite 115 passing, gts check 0 errors, clang-format -Werror clean.

The zero leak counts remain the load-bearing check: they confirm the drain hook still does what node::ObjectWrap's per-instance hook used to, now that it is registered from a different place.

github-actions · 2026-08-10T15:00:23Z

Overall package size

Self size: 2.48 MB
Deduped: 3.18 MB
No deduping: 3.18 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.1 | 504.33 kB | 504.33 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

Ports two review nits raised on DataDog/pprof-nodejs#388, which vendors this file, and addressed there in DataDog/pprof-nodejs#391. The third comment on that PR was about deduplicating helpers against a file that has no counterpart here, so it does not apply. Register the drain hook in Init() rather than lazily on first Wrap(), which removes g_drain_hook_registered entirely. Module initialisation always runs with a context entered, so AddEnvironmentCleanupHook's CHECK is satisfied there too, and Init() runs exactly once per isolate — the lifetime the hook should match. The flag existed only to make the lazy registration idempotent and to re-arm after an isolate was torn down and recreated on the same thread; Init() running again on the new isolate covers that by construction. Clear the holder's internal field before freeing the CtxWrap it points at. That slot is exactly what the out-of-process OTEP-4947 reader walks to reach record_, so leaving it pointing at freed memory aims a dangling pointer at a consumer we do not control. Being on the live list means V8 has not collected the holder, so reading the handle there is safe; the WeakCallback path cannot do this and does not need to, since there the holder is the object being collected. Verified on Node 22, 24 and 26: 49/49 tests pass on each, and the teardown repro stays clean at N=3000.

Three review nits from #388, all valid. Deduplicate the internal-field accessors. wall.cc had its own copies of GetAlignedPointerFromInternalField / SetAlignedPointerInInternalField; #388 added the same pair in internal-field.hh and deliberately left wall.cc alone to avoid conflicting with #387, which was in flight. #387 has landed, so wall.cc now includes the header and its copies are gone. Same namespace and names, so every call site is unchanged. Register the drain hook in Init() rather than lazily on first Wrap(), which removes g_drain_hook_registered entirely. Module initialisation always runs with a context entered, so AddEnvironmentCleanupHook's CHECK is satisfied there too, and Init() runs exactly once per isolate — which is the lifetime the hook should match. The flag existed only to make the lazy registration idempotent and to re-arm after an isolate was torn down and recreated on the same thread; Init() running again on the new isolate covers that by construction. Clear the holder's internal field before freeing the CtxWrap it points at. The drain hook now nulls slot 0 on its way through the list. This matters more than a tidiness nit: that slot is exactly what the out-of-process OTEP-4947 reader walks, so leaving it pointing at freed memory is a loaded gun aimed at a consumer we do not control. Being on the live list means V8 has not collected the holder, so reading the handle there is safe; the WeakCallback path cannot do this and does not need to, since there the holder is the object being collected. Verified on Node 20, 24 and 26: ASAN exit 0 with zero leaks and zero aborts on 20 and 24, 165 passing on 24 and 26, the teardown regression test passing where the OTEP block runs, the original repro clean at N=3000 and N=10000, and the published native_wrap_fields_offset still 0.

nsavoire · 2026-08-10T16:07:31Z

+    // means V8 has not collected the holder, so the handle is safe to read
+    // here; the WeakCallback path cannot do this and does not need to,
+    // since there the holder is the thing being collected.
+    if (!p->handle_.IsEmpty()) {


nit: maybe do the same for PCP ?

yeah, why not. It might still be observed through a signal handler probably. I'll guard it additionally on current isolate not being null, just in case.

nsavoire · 2026-08-10T16:10:14Z

  memcpy(&new_rec->attrs_data[current_used], appended.data(), appended.size());
  new_rec->attrs_data_size = static_cast<uint16_t>(new_used);
  // The copy should've preserved valid=1 from the source record.
  assert(new_rec->valid == 1);


AI flagged that this assert will fire in debug builds when doing invalidate() followed by large enough Append(...)

This is valid; I'll fix it in a follow-up PR as it's a separate concern.

@nsavoire

…392) Append's reallocate path asserted that the record it had just copied was valid: memcpy(new_rec.get(), self->record_, ...); ... assert(new_rec->valid == 1); invalidate() sets that byte to 0, and appending afterwards is supported — there is a test for it — so the assert fires on any append too large to fit in place: Assertion `new_rec->valid == 1' failed. Aborted (exit 134) This is not debug-only. NDEBUG is never defined for this addon, so assert() is live in Release too; both configurations abort. Reproduced through the public API on Linux with invalidate() followed by a 200-byte attribute. Only the reallocate path is affected: an append that fits the current capacity is written in place and never copies the header. A fresh record has 36 bytes of attrs_data capacity, so an attribute over ~34 bytes on a fresh record is enough. The existing 'appendAttributes after invalidate' test appends 6 bytes, takes the in-place path, and so never reached the copy — right behaviour, wrong size. Assert what the check was actually for — that the memcpy carried the header across intact — by capturing the source's valid byte first and comparing against that. Still catches a genuine copy bug, such as shortening the memcpy so it no longer covers the header, and is correct whether the record is valid or not. The regression test forks, since the failure is an abort that would otherwise take the whole mocha run down. Verified it bites: against the pre-fix binding it reports signal=SIGABRT with the assertion above, and passes after. Reported by @nsavoire on #391.

@nsavoire

…392) Append's reallocate path asserted that the record it had just copied was valid: memcpy(new_rec.get(), self->record_, ...); ... assert(new_rec->valid == 1); invalidate() sets that byte to 0, and appending afterwards is supported — there is a test for it — so the assert fires on any append too large to fit in place: Assertion `new_rec->valid == 1' failed. Aborted (exit 134) This is not debug-only. NDEBUG is never defined for this addon, so assert() is live in Release too; both configurations abort. Reproduced through the public API on Linux with invalidate() followed by a 200-byte attribute. Only the reallocate path is affected: an append that fits the current capacity is written in place and never copies the header. A fresh record has 36 bytes of attrs_data capacity, so an attribute over ~34 bytes on a fresh record is enough. The existing 'appendAttributes after invalidate' test appends 6 bytes, takes the in-place path, and so never reached the copy — right behaviour, wrong size. Assert what the check was actually for — that the memcpy carried the header across intact — by capturing the source's valid byte first and comparing against that. Still catches a genuine copy bug, such as shortening the memcpy so it no longer covers the header, and is correct whether the record is valid or not. The regression test forks, since the failure is an abort that would otherwise take the whole mocha run down. Verified it bites: against the pre-fix binding it reports signal=SIGABRT with the assertion above, and passes after. Reported by @nsavoire on #391. (cherry picked from commit cfa8cd1)

* Follow up on #388 review comments Three review nits from #388, all valid. Deduplicate the internal-field accessors. wall.cc had its own copies of GetAlignedPointerFromInternalField / SetAlignedPointerInInternalField; #388 added the same pair in internal-field.hh and deliberately left wall.cc alone to avoid conflicting with #387, which was in flight. #387 has landed, so wall.cc now includes the header and its copies are gone. Same namespace and names, so every call site is unchanged. Register the drain hook in Init() rather than lazily on first Wrap(), which removes g_drain_hook_registered entirely. Module initialisation always runs with a context entered, so AddEnvironmentCleanupHook's CHECK is satisfied there too, and Init() runs exactly once per isolate — which is the lifetime the hook should match. The flag existed only to make the lazy registration idempotent and to re-arm after an isolate was torn down and recreated on the same thread; Init() running again on the new isolate covers that by construction. Clear the holder's internal field before freeing the CtxWrap it points at. The drain hook now nulls slot 0 on its way through the list. This matters more than a tidiness nit: that slot is exactly what the out-of-process OTEP-4947 reader walks, so leaving it pointing at freed memory is a loaded gun aimed at a consumer we do not control. Being on the live list means V8 has not collected the holder, so reading the handle there is safe; the WeakCallback path cannot do this and does not need to, since there the holder is the object being collected. Verified on Node 20, 24 and 26: ASAN exit 0 with zero leaks and zero aborts on 20 and 24, 165 passing on 24 and 26, the teardown regression test passing where the OTEP block runs, the original repro clean at N=3000 and N=10000, and the published native_wrap_fields_offset still 0. * Add the zero-out-internal-field logic to PCP too (cherry picked from commit 6c74108)

szegedi requested review from IlyasShabi, nsavoire and r1viollet as code owners August 10, 2026 14:59

This comment has been minimized.

Sign in to view

szegedi force-pushed the szegedi/ctxwrap-review-followups branch from c9f9fee to 8dfa9bb Compare August 10, 2026 15:18

szegedi added the semver-patch Bug or security fixes, mainly label Aug 10, 2026

nsavoire reviewed Aug 10, 2026

View reviewed changes

Add the zero-out-internal-field logic to PCP too

4597fb1

szegedi mentioned this pull request Aug 11, 2026

fix(otel-thread-ctx): don't assert a record is valid when growing it #392

Merged

nsavoire approved these changes Aug 11, 2026

View reviewed changes

szegedi merged commit 6c74108 into main Aug 11, 2026
70 checks passed

szegedi deleted the szegedi/ctxwrap-review-followups branch August 11, 2026 09:03

This was referenced Aug 11, 2026

Don't derive CtxWrap from node::ObjectWrap polarsignals/custom-labels#20

Open

v5.18.0 #394

Merged

Navigation Menu

Uh oh!

Follow up on #388 review comments - #391

Follow up on #388 review comments#391
szegedi merged 2 commits into
mainfrom
szegedi/ctxwrap-review-followups

szegedi commented Aug 10, 2026

Uh oh!

github-actions Bot commented Aug 10, 2026

Uh oh!

This comment has been minimized.

nsavoire Aug 10, 2026

Uh oh!

szegedi Aug 11, 2026

Uh oh!

nsavoire Aug 10, 2026

Uh oh!

szegedi Aug 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

szegedi commented Aug 10, 2026

1. Duplicated internal-field accessors

2. Register the hook in Init(), drop g_drain_hook_registered

3. Clear the internal field before deleting the wrap

Verification

Uh oh!

github-actions Bot commented Aug 10, 2026

Overall package size

Uh oh!

This comment has been minimized.

nsavoire Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

szegedi Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

nsavoire Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

szegedi Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

2. Register the hook in `Init()`, drop `g_drain_hook_registered`