Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: unlink reference during its destructor #35933

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Nov 2, 2020

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: @gabrielschulhof

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

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 <gabriel.schulhof@intel.com>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 2, 2020

Review requested:

  • @nodejs/n-api

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 2, 2020
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Nov 2, 2020
@nodejs-github-bot
Copy link
Collaborator

@Trott Trott requested a review from addaleax November 3, 2020 13:37
@gabrielschulhof
Copy link
Contributor Author

@himself65 @Trott I made the destructor virtual.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great that we discussed this in the last N-API team meeting.

@gabrielschulhof gabrielschulhof added commit-queue Add this label to land a pull request using GitHub Actions. c++ Issues and PRs that require attention from people who are familiar with C++. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels Nov 5, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 5, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35933
✔  Done loading data for nodejs/node/pull/35933
----------------------------------- PR info ------------------------------------
Title      n-api: unlink reference during its destructor (#35933)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     gabrielschulhof:reference-segfault-35620-final -> nodejs:master
Labels     C++, n-api
Commits    2
 - n-api: unlink reference during its destructor
 - fixup! make the destructor virtual
Committers 1
 - Gabriel Schulhof 
PR-URL: https://github.com/nodejs/node/pull/35933
Fixes: https://github.com/nodejs/node/issues/35778
Refs: https://github.com/nodejs/node/issues/34731
Refs: https://github.com/nodejs/node/pull/34839
Refs: https://github.com/nodejs/node/issues/35620
Refs: https://github.com/nodejs/node/pull/35777
Reviewed-By: Zeyu Yang 
Reviewed-By: Rich Trott 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35933
Fixes: https://github.com/nodejs/node/issues/35778
Refs: https://github.com/nodejs/node/issues/34731
Refs: https://github.com/nodejs/node/pull/34839
Refs: https://github.com/nodejs/node/issues/35620
Refs: https://github.com/nodejs/node/pull/35777
Reviewed-By: Zeyu Yang 
Reviewed-By: Rich Trott 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-11-03T19:46:51Z: https://ci.nodejs.org/job/node-test-pull-request/34041/
- Querying data for job/node-test-pull-request/34041/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Mon, 02 Nov 2020 23:33:51 GMT
   ✔  Approvals: 3
   ✔  - Zeyu Yang (@himself65): https://github.com/nodejs/node/pull/35933#pullrequestreview-522152386
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35933#pullrequestreview-522500099
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/35933#pullrequestreview-523676946
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 35933
From https://github.com/nodejs/node
 * branch                  refs/pull/35933/merge -> FETCH_HEAD
✔  Fetched commits as d4d6dfdb9412..b2cf693bbfd6
--------------------------------------------------------------------------------
[master 5ae2240fe7] n-api: unlink reference during its destructor
 Author: Gabriel Schulhof 
 Date: Mon Nov 2 15:06:23 2020 -0800
 2 files changed, 2 insertions(+), 3 deletions(-)
[master 275e347723] fixup! make the destructor virtual
 Author: Gabriel Schulhof 
 Date: Tue Nov 3 08:16:59 2020 -0800
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
⚠ Found Fixes: #35778, skipping..
⚠ Found Refs: #34731, skipping..
⚠ Found Refs: #34839, skipping..
⚠ Found Refs: #35620, skipping..
--------------------------------- New Message ----------------------------------
n-api: unlink reference during its destructor

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 gabriel.schulhof@intel.com

PR-URL: #35933
Refs: #35777
Reviewed-By: Zeyu Yang himself65@outlook.com
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Michael Dawson midawson@redhat.com

[detached HEAD 9c0df1f02f] n-api: unlink reference during its destructor
Author: Gabriel Schulhof gabriel.schulhof@intel.com
Date: Mon Nov 2 15:06:23 2020 -0800
2 files changed, 2 insertions(+), 3 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup! make the destructor virtual

PR-URL: #35933
Fixes: #35778
Refs: #34731
Refs: #34839
Refs: #35620
Refs: #35777
Reviewed-By: Zeyu Yang himself65@outlook.com
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Michael Dawson midawson@redhat.com

[detached HEAD cba44aac0a] fixup! make the destructor virtual
Author: Gabriel Schulhof gabriel.schulhof@intel.com
Date: Tue Nov 3 08:16:59 2020 -0800
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/master.
✖ 9c0df1f02fc2d62519172b10e54d2aec4f17fa91
✔ 32:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✖ 22:72 Line should be <= 72 columns. line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 35:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ cba44aac0a5a9cc1a7429de5cc753e6fdd77693f
✔ 2:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
ℹ Please fix the commit message and try again.

Commit Queue action: https://github.com/nodejs/node/actions/runs/346580978

@gabrielschulhof
Copy link
Contributor Author

Landed in c822ba7.

gabrielschulhof pushed a commit that referenced this pull request Nov 5, 2020
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 <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
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 <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
@gabrielschulhof gabrielschulhof deleted the reference-segfault-35620-final branch November 13, 2020 21:42
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
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 <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
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 <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
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 <gabriel.schulhof@intel.com>
PR-URL: #35933
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate test/node-api/test_worker_terminate_finalization/test.js
6 participants