n-api: handle weak no-finalizer refs correctly #34839

gabrielschulhof · 2020-08-19T06:39:22Z

When deleting a weak reference that has no finalizer we must not defer
deletion until the non-existent finalizer gets called.

Fixes: #34731

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2020-08-19T06:39:26Z

Review requested:

@nodejs/n-api

addaleax · 2020-08-19T14:20:15Z

src/js_native_api_v8.cc

@@ -242,6 +242,8 @@ class RefBase : protected Finalizer, RefTracker {
  static inline void Delete(RefBase* reference) {
    reference->Unlink();
    if ((reference->RefCount() != 0) ||
+        (reference->RefCount() == 0 &&


RefCount() is unsigned, so reference->RefCount() == 0 is always true here, right?

That's true. This can be simplified to the second half of the &&.

gabrielschulhof · 2020-08-19T14:39:46Z

@addaleax I removed the always-true condition and I also updated the comment above the function to include an explanation of the new condition (i.e. that we want to delete when the ref is weak but has no finalizer).

When deleting a weak reference that has no finalizer we must not defer deletion until the non-existent finalizer gets called. Fixes: nodejs#34731 Signed-off-by: Gabriel Schulhof <[email protected]>

gabrielschulhof · 2020-08-19T14:47:56Z

Added Signed-off-by header.

gabrielschulhof · 2020-08-19T18:03:07Z

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

gabrielschulhof · 2020-08-19T18:29:40Z

Hmmm ... CI is failing, but the logs are truncated, so I can't see what the failure was.

gabrielschulhof · 2020-08-20T00:03:14Z

Resumed as: https://ci.nodejs.org/job/node-test-pull-request/32860/ ✔️

When deleting a weak reference that has no finalizer we must not defer deletion until the non-existent finalizer gets called. Fixes: #34731 Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #34839 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Trott · 2020-08-21T13:16:44Z

Landed in acd423b

mhdawson

Belated LGTM

When deleting a weak reference that has no finalizer we must not defer deletion until the non-existent finalizer gets called. Fixes: #34731 Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #34839 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Notable changes: - build: set --v8-enable-object-print by default (Mary Marchini) [#34705](#34705) - deps: - upgrade to libuv 1.39.0 (cjihrig) [#34915](#34915) - upgrade npm to 6.14.8 (Ruy Adorno) [#34834](#34834) - V8: cherry-pick e06ace6b5cdb (Anna Henningsen) [#34673](#34673) - n-api: handle weak no-finalizer refs correctly (Gabriel Schulhof) [#34839](#34839) - tools: add debug entitlements for macOS 10.15+ (Gabriele Greco) [#34378](#34378) PR-URL: #34852

When deleting a weak reference that has no finalizer we must not defer deletion until the non-existent finalizer gets called. Fixes: #34731 Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #34839 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Currently, a reference is being unlinked from the list of references tracked by the environment when `v8impl::Reference::Delete` is called. This causes a leak when deletion must be deferred because the finalizer hasn't yet run, but the finalizer does not run because environment teardown is in progress, and so no more gc runs will happen, and the `FinalizeAll` run that happens during environment teardown does not catch the reference because it's no longer in the list. The test below will fail when running with ASAN: ``` ./node ./test/node-api/test_worker_terminate_finalization/test.js ``` OTOH if, to address the above leak, we make a special case to not unlink a reference during environment teardown, we run into a situation where the reference gets deleted by `v8impl::Reference::Delete` but does not get unlinked because it's environment teardown time. This leaves a stale pointer in the linked list which will result in a use-after-free in `FinalizeAll` during environment teardown. The test below will fail if we make the above change: ``` ./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');" ``` Thus, we unlink a reference precisely when we destroy it – in its destructor. Refs: nodejs#34731 Refs: nodejs#34839 Refs: nodejs#35620 Refs: nodejs#35777 Fixes: nodejs#35778 Signed-off-by: Gabriel Schulhof <[email protected]>

Currently, a reference is being unlinked from the list of references tracked by the environment when `v8impl::Reference::Delete` is called. This causes a leak when deletion must be deferred because the finalizer hasn't yet run, but the finalizer does not run because environment teardown is in progress, and so no more gc runs will happen, and the `FinalizeAll` run that happens during environment teardown does not catch the reference because it's no longer in the list. The test below will fail when running with ASAN: ``` ./node ./test/node-api/test_worker_terminate_finalization/test.js ``` OTOH if, to address the above leak, we make a special case to not unlink a reference during environment teardown, we run into a situation where the reference gets deleted by `v8impl::Reference::Delete` but does not get unlinked because it's environment teardown time. This leaves a stale pointer in the linked list which will result in a use-after-free in `FinalizeAll` during environment teardown. The test below will fail if we make the above change: ``` ./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');" ``` Thus, we unlink a reference precisely when we destroy it – in its destructor. Refs: #34731 Refs: #34839 Refs: #35620 Refs: #35777 Fixes: #35778 Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #35933 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

gabrielschulhof requested review from addaleax and mhdawson August 19, 2020 06:39

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 19, 2020

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Aug 19, 2020

addaleax approved these changes Aug 19, 2020

View reviewed changes

addaleax reviewed Aug 19, 2020

View reviewed changes

gabrielschulhof force-pushed the n-api-fix-delete-after-weaken branch from e9c66de to 7a20fc8 Compare August 19, 2020 14:38

devsnek approved these changes Aug 19, 2020

View reviewed changes

n-api: handle weak no-finalizer refs correctly

2a25930

When deleting a weak reference that has no finalizer we must not defer deletion until the non-existent finalizer gets called. Fixes: nodejs#34731 Signed-off-by: Gabriel Schulhof <[email protected]>

gabrielschulhof force-pushed the n-api-fix-delete-after-weaken branch from 7a20fc8 to 2a25930 Compare August 19, 2020 14:47

jasnell approved these changes Aug 19, 2020

View reviewed changes

Trott approved these changes Aug 21, 2020

View reviewed changes

Trott closed this Aug 21, 2020

mhdawson reviewed Aug 21, 2020

View reviewed changes

This was referenced Aug 22, 2020

CI Reliability 2020-08-22 nodejs/reliability#51

Open

CI Reliability 2020-08-23 nodejs/reliability#52

Open

CI Reliability 2020-08-24 nodejs/reliability#53

Open

github-actions bot mentioned this pull request Aug 25, 2020

CI Reliability 2020-08-25 nodejs/reliability#54

Open

11 tasks

danielleadams mentioned this pull request Aug 25, 2020

V14.9.0 proposal #34852

Merged

github-actions bot mentioned this pull request Aug 26, 2020

CI Reliability 2020-08-26 nodejs/reliability#55

Open

9 tasks

github-actions bot mentioned this pull request Aug 27, 2020

CI Reliability 2020-08-27 nodejs/reliability#56

Open

10 tasks

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

gabrielschulhof mentioned this pull request Nov 2, 2020

n-api: unlink reference during its destructor #35933

Closed

4 tasks

gabrielschulhof deleted the n-api-fix-delete-after-weaken branch January 28, 2021 00:14

gabrielschulhof restored the n-api-fix-delete-after-weaken branch January 28, 2021 05:40

gabrielschulhof deleted the n-api-fix-delete-after-weaken branch February 3, 2021 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: handle weak no-finalizer refs correctly #34839

n-api: handle weak no-finalizer refs correctly #34839

gabrielschulhof commented Aug 19, 2020

nodejs-github-bot commented Aug 19, 2020

addaleax Aug 19, 2020

gabrielschulhof Aug 19, 2020

gabrielschulhof commented Aug 19, 2020 •

edited

Loading

gabrielschulhof commented Aug 19, 2020

gabrielschulhof commented Aug 19, 2020

gabrielschulhof commented Aug 19, 2020

gabrielschulhof commented Aug 20, 2020 •

edited

Loading

Trott commented Aug 21, 2020

mhdawson left a comment

n-api: handle weak no-finalizer refs correctly #34839

n-api: handle weak no-finalizer refs correctly #34839

Conversation

gabrielschulhof commented Aug 19, 2020

Checklist

nodejs-github-bot commented Aug 19, 2020

addaleax Aug 19, 2020

Choose a reason for hiding this comment

gabrielschulhof Aug 19, 2020

Choose a reason for hiding this comment

gabrielschulhof commented Aug 19, 2020 • edited Loading

gabrielschulhof commented Aug 19, 2020

gabrielschulhof commented Aug 19, 2020

gabrielschulhof commented Aug 19, 2020

gabrielschulhof commented Aug 20, 2020 • edited Loading

Trott commented Aug 21, 2020

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Aug 19, 2020 •

edited

Loading

gabrielschulhof commented Aug 20, 2020 •

edited

Loading