node-api: avoid SecondPassCallback crash #38899

mhdawson · 2021-06-02T01:46:43Z

PR #38000 added
indirection so that we could stop finalization in
cases where it had been scheduled in a second
pass callback but we were doing it in advance in
environment teardown.

Unforunately we missed that the code which tries
to clear the second pass parameter checked if
the pointer to the parameter (_secondPassParameter)
was nullptr and that when the second pass callback
was scheduled we set _secondPassParameter to nullptr
in order to avoid it being deleted outside of the second
pass callback. The net result was that we
would not clear the _secondPassParameter contents
and failed to avoid the Finalization in the second pass
callback.

This PR adds an additional boolean for deciding if
the secondPassParameter should be deleted outside
of the second pass callback instead of setting
secondPassParameter to nullptr thus avoiding the
conflict between the 2 ways it was being used.

See the discussion starting at:
#38273 (comment)
for how this was discovered on OSX while trying to
upgrade to a new V8 version.

Signed-off-by: Michael Dawson mdawson@devrus.com

PR nodejs#38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: nodejs#38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com>

mhdawson · 2021-06-02T01:48:30Z

@legendecas would be great to get you to look at this as experience shows it is tricky to get right.

nodejs-github-bot · 2021-06-02T07:41:50Z

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

targos · 2021-06-02T14:41:16Z

@nodejs/node-api

nodejs-github-bot · 2021-06-03T05:49:34Z

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

legendecas · 2021-06-03T06:39:39Z

src/js_native_api_v8.cc

@@ -468,12 +468,14 @@ class Reference : public RefBase {
      // the reference itself has already been deleted so nothing to do
      return;
    }
+    reference->_secondPassParameter = nullptr;
    reference->Finalize();


IIUC, the problem happens here as the SecondPassCallParameterRef ownership is been transferred to the SecondPass weak parameter but its content (v8impl::Reference) is already been destroyed by env teardown. I'm afraid I did not understand how the patch could fix the problem?

@legendecas see comment above which explains how the fix addresses the issue.

mhdawson · 2021-06-03T19:30:42Z

src/js_native_api_v8.cc

@@ -445,8 +446,7 @@ class Reference : public RefBase {
    reference->_persistent.Reset();
    // Mark the parameter not delete-able until the second pass callback is
    // invoked.
-    reference->_secondPassParameter = nullptr;


@legendecas removing this is the line and using the boolean flag instead is what resolves the issue. The with this line is that it sets the reference->_secondPassParameter to nullptr which means that when we later try to clear what it points to its null and we con't actually clear it.

Putting that in another way the old flow was

-> set reference->_secondPassParameter to nullptr
-> schedule second pass callback
-> start env teardown and decide we want to try to cancel the Finalization in the second pass callback. To do that we need reference->_secondPassParameter but it has already been set to null so we do nothing....
-> the second pass callback still runs Finalization even though it should have been aborted.

This patch avoids setting reference->_secondPassParameter to null too early.
->

Thanks for your explanation. IIUC, This line transfers the ownership of secondPassParameter to the second pass callback, but didn't transfer the ownership of the content of secondPassParameter (i.e. the v8impl::Reference) to the callback. In that sense, I'm afraid I didn't get the point how the change clears the content of secondPassParameter on the env teardown to prevent second pass callback to finalize.

mhdawson · 2021-06-03T19:32:04Z

@gabrielschulhof if you could take a look too that would be good.

mhdawson · 2021-06-03T19:36:18Z

Another note is that when I ran the many runs of the node-add-api tests earlier under valgrind before, there was an even more infrequent issue than the one addressed in the earlier PR.

There was no crash/failure on linux, but in something like 1/1000 runs valgrind would report an invalid read of 1 byte. I was planning to take a look to see if I could track down that issue as a later step.

This patch seem to fix that as in 4778 runs over 16ish hours I did not see a recreate and valgrind reported no invalid reads/writes/

mhdawson · 2021-06-03T19:42:45Z

Resume build as I don't think the failure in CI was related (does not seem to use/run any addons)

nodejs-github-bot · 2021-06-04T07:48:39Z

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

targos · 2021-06-04T11:10:22Z

Resumed build is green.

legendecas · 2021-06-04T14:39:39Z

src/js_native_api_v8.cc

@@ -468,12 +468,14 @@ class Reference : public RefBase {
      // the reference itself has already been deleted so nothing to do
      return;


It is this branch that early breaks from the SecondPassCallback. However, it seems that we did fail to reach to this branch.

legendecas

Thanks for the update. I find that in the v8impl::Finalizer::Finalize on env teardown, ClearWeak will clear the content of second pass parameter when it is not nullptr. So that's going to fix the problem.

I do really believe we need a test suite around this part. I'll continue that part as I just get back to my home.

mhdawson · 2021-06-04T20:34:18Z

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

nodejs-github-bot · 2021-06-04T20:34:21Z

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

nodejs-github-bot · 2021-06-05T08:22:17Z

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

nodejs-github-bot · 2021-06-07T12:55:39Z

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

nodejs-github-bot · 2021-06-07T14:42:53Z

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

nodejs-github-bot · 2021-06-08T06:24:59Z

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

PR #38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: #38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #38899 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos · 2021-06-08T12:03:53Z

Landed in 65a7fd3

PR #38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: #38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #38899 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR nodejs#38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: nodejs#38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#38899 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR #38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: #38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #38899 Backport-PR-URL: #42512 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

github-actions 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 Jun 2, 2021

mhdawson mentioned this pull request Jun 2, 2021

deps: update V8 to 9.1 #38273

Merged

bl-ue approved these changes Jun 2, 2021

View reviewed changes

legendecas reviewed Jun 3, 2021

View reviewed changes

mhdawson commented Jun 3, 2021

View reviewed changes

targos added the node-api Issues and PRs related to the Node-API. label Jun 4, 2021

legendecas reviewed Jun 4, 2021

View reviewed changes

legendecas approved these changes Jun 4, 2021

View reviewed changes

jasnell approved these changes Jun 7, 2021

View reviewed changes

targos mentioned this pull request Jun 8, 2021

Node.js Technical Steering Committee (TSC) Meeting 2021-06-10 nodejs/TSC#1036

Closed

targos closed this Jun 8, 2021

legendecas mentioned this pull request Jun 8, 2021

node-api: refactor to cctest v8impl::Reference #38970

Closed

danielleadams mentioned this pull request Jun 14, 2021

v16.4.0 proposal #39031

Merged

legendecas mentioned this pull request Jul 8, 2021

src: remove unused guards around node-api reference #38334

Closed

richardlau mentioned this pull request Jul 20, 2021

v14.17.4 proposal #39466

Merged

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

richardlau mentioned this pull request Mar 30, 2022

v12.22.12 proposal #42531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node-api: avoid SecondPassCallback crash #38899

node-api: avoid SecondPassCallback crash #38899

mhdawson commented Jun 2, 2021

mhdawson commented Jun 2, 2021

nodejs-github-bot commented Jun 2, 2021

targos commented Jun 2, 2021

nodejs-github-bot commented Jun 3, 2021

legendecas Jun 3, 2021

mhdawson Jun 3, 2021

mhdawson Jun 3, 2021 •

edited

Loading

legendecas Jun 4, 2021 •

edited

Loading

mhdawson commented Jun 3, 2021

mhdawson commented Jun 3, 2021

mhdawson commented Jun 3, 2021

nodejs-github-bot commented Jun 4, 2021

targos commented Jun 4, 2021

legendecas Jun 4, 2021

legendecas left a comment

mhdawson commented Jun 4, 2021

nodejs-github-bot commented Jun 4, 2021

nodejs-github-bot commented Jun 5, 2021

nodejs-github-bot commented Jun 7, 2021

nodejs-github-bot commented Jun 7, 2021

nodejs-github-bot commented Jun 8, 2021

targos commented Jun 8, 2021

		@@ -468,12 +468,14 @@ class Reference : public RefBase {
		// the reference itself has already been deleted so nothing to do
		return;

node-api: avoid SecondPassCallback crash #38899

node-api: avoid SecondPassCallback crash #38899

Conversation

mhdawson commented Jun 2, 2021

mhdawson commented Jun 2, 2021

nodejs-github-bot commented Jun 2, 2021

targos commented Jun 2, 2021

nodejs-github-bot commented Jun 3, 2021

legendecas Jun 3, 2021

Choose a reason for hiding this comment

mhdawson Jun 3, 2021

Choose a reason for hiding this comment

mhdawson Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

legendecas Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

mhdawson commented Jun 3, 2021

mhdawson commented Jun 3, 2021

mhdawson commented Jun 3, 2021

nodejs-github-bot commented Jun 4, 2021

targos commented Jun 4, 2021

legendecas Jun 4, 2021

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

mhdawson commented Jun 4, 2021

nodejs-github-bot commented Jun 4, 2021

nodejs-github-bot commented Jun 5, 2021

nodejs-github-bot commented Jun 7, 2021

nodejs-github-bot commented Jun 7, 2021

nodejs-github-bot commented Jun 8, 2021

targos commented Jun 8, 2021

mhdawson Jun 3, 2021 •

edited

Loading

legendecas Jun 4, 2021 •

edited

Loading