★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21129Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
wants to merge 1 commit into from

Conversation

Reviewers

@apapirovski apapirovski

@refack refack

@mhdawson mhdawson

@addaleax addaleax

Assignees
No one assigned
Labels
C++ dont-land-on-v6.x n-api
Projects
None yet
Milestone
No milestone
7 participants
@gabrielschulhof @MayhemYDG @refack @MylesBorins @apapirovski @mhdawson @nodejs-github-bot
@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jun 4, 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

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

@nodejs-github-bot nodejs-github-bot added C++ dont-land-on-v4.x dont-land-on-v6.x n-api labels Jun 4, 2018

@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jun 4, 2018

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

@gabrielschulhof gabrielschulhof requested a review from addaleax Jun 4, 2018

@MayhemYDG

This comment has been minimized.

Sign in to view
Copy link
Contributor

MayhemYDG commented Jun 4, 2018 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jun 4, 2018

@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 gabrielschulhof force-pushed the gabrielschulhof:napi-delete-async-work branch from 18a98d9 to 2853b40 Jun 5, 2018

@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jun 5, 2018

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

mhdawson approved these changes Jun 5, 2018
View changes

Copy link
Member

mhdawson left a comment

LGTM
@mhdawson

mhdawson approved these changes Jun 5, 2018
View changes

Copy link
Member

mhdawson left a comment

LGTM
@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jun 5, 2018

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

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:napi-delete-async-work branch from 2853b40 to a3b4c9e Jun 5, 2018

@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jun 5, 2018

Another CI: https://ci.nodejs.org/job/node-test-pull-request/15268/
@gabrielschulhof
n-api: back up env before async work finalize 
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
Loading status checks…
c7b1950

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:napi-delete-async-work branch from a3b4c9e to c7b1950 Jun 6, 2018

@gabrielschulhof

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jun 6, 2018 •
edited

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/
@apapirovski

apapirovski approved these changes Jun 6, 2018
View changes

@MayhemYDG

This comment has been minimized.

Sign in to view
Copy link
Contributor

MayhemYDG commented Jun 6, 2018

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

refack approved these changes Jun 6, 2018
View changes

@gabrielschulhof gabrielschulhof referenced this pull request Jun 6, 2018

Merged

v10.4.0 proposal #21167

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jun 6, 2018

@refack OK, landing it then.

gabrielschulhof added a commit that referenced this pull request Jun 6, 2018

@gabrielschulhof
n-api: back up env before async work finalize 
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>
991f406
@MylesBorins

This comment has been minimized.

Sign in to view
Copy link
Member

MylesBorins commented Jun 6, 2018

landed in 991f406

@MylesBorins MylesBorins closed this Jun 6, 2018

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:napi-delete-async-work branch Jun 6, 2018

targos added a commit that referenced this pull request Jun 7, 2018

@gabrielschulhof @targos
n-api: back up env before async work finalize 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
917615e

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

@gabrielschulhof @shisama
n-api: back up env before async work finalize 
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
PR-URL: nodejs#21129
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
d4ef0b9

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

@gabrielschulhof @shisama
n-api: back up env before async work finalize 
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
PR-URL: nodejs#21129
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
7be0911

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

@gabrielschulhof @shisama
n-api: back up env before async work finalize 
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
PR-URL: nodejs#21129
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
e634cb1

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

@gabrielschulhof @shisama
n-api: back up env before async work finalize 
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
PR-URL: nodejs#21129
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
b8d223b

@mhdawson mhdawson referenced this pull request Jul 4, 2018

Merged

v8.12.0 proposal #21593

@MylesBorins MylesBorins added the backport-requested-v8.x label Jul 9, 2018

@MylesBorins

This comment has been minimized.

Sign in to view
Copy link
Member

MylesBorins commented Jul 9, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gabrielschulhof commented Jul 10, 2018

@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.

@gabrielschulhof gabrielschulhof removed the backport-requested-v8.x label Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.