GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [v14.x] Backport environment teardown Node-API reference double free fixes by gabrielschulhof · Pull Request #37802 · nodejs/node

Conversation

@gabrielschulhof

@gabrielschulhof gabrielschulhof commented Mar 19, 2021

Copy link
Copy Markdown
Contributor

These two fixes are necessary in order to prevent an issue on v14.x whereby a gc happens during environment shutdown causing a double free of a Node-API reference.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gabrielschulhof gabrielschulhof added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to Node-API. v14.x labels Mar 19, 2021
@gabrielschulhof
gabrielschulhof requested review from legendecas and mhdawson March 19, 2021 03:56
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 19, 2021
@gabrielschulhof gabrielschulhof changed the title Backport environment teardown Node-API reference double free fixes [v14.x] Backport environment teardown Node-API reference double free fixes Mar 19, 2021
@gabrielschulhof
gabrielschulhof force-pushed the backport-teardown-gc-ref-fix-to-v14.x branch from 9083454 to e31b2e3 Compare March 19, 2021 04:26
legendecas
legendecas approved these changes Mar 19, 2021
View reviewed changes
mhdawson
mhdawson approved these changes Mar 19, 2021
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mhdawson
mhdawson suggested changes Mar 22, 2021
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think we should figure out nodejs/node-addon-api#906 before this lands.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gabrielschulhof reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@mhdawson mhdawson mentioned this pull request Mar 22, 2021
Intermittent failure in ThreadSafe function nodejs/node-addon-api#906
Closed
@gabrielschulhof

gabrielschulhof commented Mar 23, 2021

Copy link
Copy Markdown
Contributor Author

@legendecas can you verify that this fixes #37236? @mhdawson if so, we may want to land it for that alone.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhdawson

mhdawson commented Mar 25, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@gabrielschulhof nodejs/node-addon-api#906 has landed so as long as the 2 land together we should be good to go.

EDIT: The PR that landed was: #37876 and we should make sure that it lands along with this one in 14.x (ie either both or neither)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhdawson
mhdawson approved these changes Mar 25, 2021
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@legendecas

legendecas commented Mar 26, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@gabrielschulhof I tried to cherry-picked @mhdawson's fix and ran with debug build and get segv rarely with stack:

#0  0x00000000070c5630 in ?? ()
#1  0x0000000000f43cb8 in v8impl::(anonymous namespace)::Reference::SecondPassCallback (data=...) at ../../../src/js_native_api_v8.cc:405
#2  0x00000000014f0ea2 in v8::internal::GlobalHandles::PendingPhantomCallback::Invoke (this=this@entry=0x7ffd2bd85aa0, isolate=<optimized out>, type=type@entry=v8::internal::GlobalHandles::PendingPhantomCallback::kSecondPass)
    at ../../../deps/v8/src/handles/global-handles.cc:1500
#3  0x00000000014f0f27 in v8::internal::GlobalHandles::InvokeSecondPassPhantomCallbacks (this=0x68071c0) at ../../../deps/v8/src/handles/global-handles.cc:1367
#4  0x00000000014f0fd8 in v8::internal::GlobalHandles::InvokeSecondPassPhantomCallbacksFromTask (this=0x68071c0) at ../../../deps/v8/src/handles/global-handles.cc:1350
#5  0x00000000013ff9b9 in non-virtual thunk to v8::internal::CancelableTask::Run() ()
Python Exception <class 'gdb.error'> There is no member or method named _M_head_impl.:
#6  0x000000000107abb4 in node::PerIsolatePlatformData::RunForegroundTask (this=0x6806640, task=) at ../../../src/node_platform.cc:410
#7  0x000000000107b157 in node::PerIsolatePlatformData::FlushForegroundTasksInternal (this=0x6806640) at ../../../src/node_platform.cc:479
#8  0x0000000001079e61 in node::PerIsolatePlatformData::FlushTasks (handle=0x6806d50) at ../../../src/node_platform.cc:238
#9  0x0000000001e20d3c in uv__async_io (loop=0x59f7d60 <default_loop_struct>, w=0x59f7f28 <default_loop_struct+456>, events=1) at ../../../deps/uv/src/unix/async.c:163
#10 0x0000000001e38ea1 in uv__io_poll (loop=0x59f7d60 <default_loop_struct>, timeout=0) at ../../../deps/uv/src/unix/linux-core.c:462
#11 0x0000000001e21725 in uv_run (loop=0x59f7d60 <default_loop_struct>, mode=UV_RUN_ONCE) at ../../../deps/uv/src/unix/core.c:385
#12 0x0000000000f1d6d1 in node::Environment::CleanupHandles (this=0x689abf0) at ../../../src/env.cc:576
#13 0x0000000000f1da25 in node::Environment::RunCleanup (this=0x689abf0) at ../../../src/env.cc:627
#14 0x0000000000eb2447 in node::FreeEnvironment (env=0x689abf0) at ../../../src/api/environment.cc:385
#15 0x0000000001029556 in node::FunctionDeleter<node::Environment, &node::FreeEnvironment>::operator() (this=0x7ffd2bd89850, pointer=0x689abf0) at ../../../src/util.h:624
#16 0x000000000102925d in std::unique_ptr<node::Environment, node::FunctionDeleter<node::Environment, &node::FreeEnvironment> >::~unique_ptr (this=0x7ffd2bd89850, __in_chrg=<optimized out>) at /opt/rh/devtoolset-7/root/usr/include/c++/7/bits/unique_ptr.h:268
#17 0x0000000001028a4d in node::NodeMainInstance::Run (this=0x7ffd2bd898e0) at ../../../src/node_main_instance.cc:114
#18 0x0000000000f65738 in node::Start (argc=4, argv=0x7ffd2bd89b78) at ../../../src/node.cc:1101
#19 0x0000000002665934 in main (argc=4, argv=0x7ffd2bd89b78) at ../../../src/node_main.cc:141

and the data of frame 2:

(gdb) print *data.parameter_
$4 = {<v8impl::(anonymous namespace)::RefBase> = {<v8impl::Finalizer> = {_env = 0x6855a90, _finalize_callback = 0x0, _finalize_data = 0x70cb6d0, _finalize_hint = 0x0, _finalize_ran = false, _has_env_reference = false}, <v8impl::RefTracker> = {_vptr.RefTracker = 0x6d8acf0,
      next_ = 0x0, prev_ = 0x0}, _refcount = 0, _delete_self = true}, _persistent = {<v8::PersistentBase<v8::Value>> = {val_ = 0x0}, <No data fields>}}

Not sure why it is crashing.. Will try to dig into it to see if the problem is related to the one we are resolving.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gabrielschulhof

gabrielschulhof commented Mar 26, 2021

Copy link
Copy Markdown
Contributor Author

@legendecas @mhdawson I have now cherry-picked #37876 on top. Please re-review!

@legendecas does @mhdawson's fix prevent the crash you are seeing? Please let us know if this additional patch fixes your issue, and whether this issue occurs in production for you. We need to know because there are currently no v14.x releases planned so, if this is an important fix, we may need to arrange for an additional v14.x release.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@legendecas

legendecas commented Mar 29, 2021

Copy link
Copy Markdown
Member

@gabrielschulhof So I find out that v8 doesn't clear the second pass callback if the weakness was cleared (see https://github.com/nodejs/node/blob/master/deps/v8/src/handles/global-handles.cc#L532). The fix in the PR itself is self-contained, however, v8 does call into invalidated callbacks (https://github.com/nodejs/node/blob/master/deps/v8/src/handles/global-handles.cc#L1387) as the callback was not been cleared. Maybe we need to file a fix to v8 first.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

legendecas
legendecas reviewed Mar 31, 2021
View reviewed changes
Comment thread src/js_native_api_v8.cc Outdated

@legendecas legendecas Mar 31, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

After digging into v8::PersistentBase and the second pass callbacks of GlobalHandles, I find out that _persistent.IsWeak() is always false after the first pass weak callback (in which we reset the persistent).

So this fix ultimately doesn't seem to be applied to the issue at all 😵 , so sorry for not picking this up earlier.

Anyway, the Persistent handle has to be reset on the first pass weak callback, thus the persistent no longer holds an address value of the global handle -- and unable to cancel the second pass callback by any means after the first pass callback. So here we have to ensure that the parameters of the weak info of second pass callbacks been kept alive until the second pass callbacks have been invoked.

I've submitted an issue to https://bugs.chromium.org/p/v8/issues/detail?id=11608. However, while I'm thinking that in the nature of the Persistent, it is possible that we can split the weak parameter lifetime from the v8impl::Reference. Trying to sum up a fix so that we can determine if the approach is acceptable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@legendecas legendecas mentioned this pull request Mar 31, 2021
node-api: make reference weak parameter an indirect link to references #38000
Closed
@MylesBorins
MylesBorins force-pushed the v14.x-staging branch from 11505ad to 6b115d7 Compare April 6, 2021 19:54
Gabriel Schulhof and others added 3 commits April 24, 2021 14:55
Gabriel Schulhof @legendecas @targos
node-api: force env shutdown deferring behavior
82b6116
The finalizer normally never gets called while a reference is strong.
However, during environment shutdown all finalizers must get called. In
order to unify the deferring behavior with that of a regular
finalization, we must force the reference to be weak when we call its
finalizer during environment shutdown.

Fixes: nodejs#37236
Co-authored-by: Chengzhong Wu <[email protected]>
PR-URL: nodejs#37303
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof @targos
node-api: stop ref gc during environment teardown
466d768
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: nodejs#37236
PR-URL: nodejs#37616
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson @targos
node-api: fix crash in finalization
180c33d
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
targos force-pushed the backport-teardown-gc-ref-fix-to-v14.x branch from fecc3f4 to 180c33d Compare April 24, 2021 12:55
@nodejs-github-bot

nodejs-github-bot commented Apr 24, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Apr 25, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Apr 26, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Apr 26, 2021

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos

targos commented Apr 26, 2021

Copy link
Copy Markdown
Member

Landed in 5a3e12b...8413759

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos targos closed this Apr 26, 2021
targos pushed a commit that referenced this pull request Apr 26, 2021
Gabriel Schulhof @legendecas @targos
node-api: force env shutdown deferring behavior
b5b1ad3
The finalizer normally never gets called while a reference is strong.
However, during environment shutdown all finalizers must get called. In
order to unify the deferring behavior with that of a regular
finalization, we must force the reference to be weak when we call its
finalizer during environment shutdown.

Fixes: #37236
Co-authored-by: Chengzhong Wu <[email protected]>
PR-URL: #37303
Backport-PR-URL: #37802
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Apr 26, 2021
@gabrielschulhof @targos
node-api: stop ref gc during environment teardown
6e01855
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: #37236
PR-URL: #37616
Backport-PR-URL: #37802
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Apr 26, 2021
@mhdawson @targos
node-api: fix crash in finalization
8413759
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Backport-PR-URL: #37802
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@deepak1556 deepak1556 mentioned this pull request Dec 9, 2021
Crash on shutdown in v8impl::(anonymous namespace)::RefBase::Finalize microsoft/vscode#138721
Closed
@legendecas legendecas mentioned this pull request Mar 27, 2022
Node.js 12.x need to cherry-pick https://github.com/nodejs/node/pull/37616 #42376
Closed
This was referenced Apr 18, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 Rednickle/node-1#71
Open
[Snyk] Security upgrade gatsby-plugin-sharp from 2.6.24 to 4.25.1 baby636/node#89
Open
@137717unity 137717unity mentioned this pull request Apr 18, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 137717unity/node#94
Open
@wedataintelligence wedataintelligence mentioned this pull request Apr 18, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 wedataintelligence/node#76
Open
This was referenced Apr 19, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.2.13 to 4.25.1 kissofire/node#97
Open
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 molllyn1/node#77
Open
@ron-a20 ron-a20 mentioned this pull request Apr 19, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 ron-a20/node#94
Open
@snyk-bot snyk-bot mentioned this pull request Apr 19, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 aliceUnhinged613/node#91
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecas legendecas legendecas approved these changes
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@gabrielschulhof @mhdawson @legendecas @nodejs-github-bot @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.