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: back up env before async work finalize #21129

Closed

Conversation

gabrielschulhof
Copy link
Contributor

We must back up the value of _env before calling the async work
complete callback, because the complete callback may delete the
instance in which _env is stored by calling napi_delete_async_work,
and because we need to use it after the complete callback has
completed.

Fixes: #20966

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Jun 4, 2018
@gabrielschulhof
Copy link
Contributor Author

I'm not sure if it makes any sense to add a test like this one, but finite.

@nstepien
Copy link
Contributor

nstepien commented Jun 4, 2018

Considering this fixes a regression, I'd say a test should be added.

@gabrielschulhof
Copy link
Contributor Author

@MayhemYDG the problem is it's a memory corruption regression, so a 100% reliable test is hard to create. After all, when c072057 introduced the regression, the async work test which we already have should have segfaulted but didn't.

My only thought with the above comment was to add a second test which essentially runs the first test over and over and over, because that's what I used for reproducing. But you're right - I think I'll add a test like the one in the gist, with, say, 500 iterations. Hopefully in that many iterations it will crash if we ever again have such a bug.

@gabrielschulhof
Copy link
Contributor Author

Well, the test I added crashes before the fix and passes after the fix, so FWIW, it captures the fix.

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

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

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

We must back up the value of `_env` before calling the async work
complete callback, because the complete callback may delete the
instance in which `_env` is stored by calling `napi_delete_async_work`,
and because we need to use it after the complete callback has
completed.

Fixes: nodejs#20966
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Jun 6, 2018

sigh ... forgot to set the name of the async context - weird that it did not throw in debug mode O_o

Here's another CI: https://ci.nodejs.org/job/node-test-pull-request/15273/

@nstepien
Copy link
Contributor

nstepien commented Jun 6, 2018

@gabrielschulhof
Nice. Do you think this could get merged in time for the v10.4.0 release?

@refack
Copy link
Contributor

refack commented Jun 6, 2018

I'm +1 to land this after only 40h so it make it into 10.4.0, since the most significant stakeholders are aware of this.

@gabrielschulhof
Copy link
Contributor Author

@refack OK, landing it then.

gabrielschulhof pushed a commit that referenced this pull request Jun 6, 2018
We must back up the value of `_env` before calling the async work
complete callback, because the complete callback may delete the
instance in which `_env` is stored by calling `napi_delete_async_work`,
and because we need to use it after the complete callback has
completed.

Fixes: #20966
PR-URL: #21129
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins
Copy link
Contributor

landed in 991f406

@MylesBorins MylesBorins closed this Jun 6, 2018
@gabrielschulhof gabrielschulhof deleted the napi-delete-async-work branch June 6, 2018 17:32
targos pushed a commit that referenced this pull request Jun 7, 2018
We must back up the value of `_env` before calling the async work
complete callback, because the complete callback may delete the
instance in which `_env` is stored by calling `napi_delete_async_work`,
and because we need to use it after the complete callback has
completed.

Fixes: #20966
PR-URL: #21129
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@gabrielschulhof
Copy link
Contributor Author

@MylesBorins this actually fixes a regression introduced by c072057, so, unless we backport that commit too, we need not backport this commit. As it stands, it doesn't look like that commit was backported to v8.x.

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.

n-api: regression with napi_delete_async_work since v10.2.0
7 participants