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.

Sign up for GitHub

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

Jump to bottom

Fix dispose without checking #31260

Closed
wants to merge 1 commit into from
Closed

Fix dispose without checking #31260

wants to merge 1 commit into from

Conversation

jc-lab
Copy link

@jc-lab jc-lab commented Jan 8, 2020

If created platform with CreatePlatform, the crash occurs because it does not check if it was initialized to v8_platform when DisposePlatform was called.

node/src/env.cc

Line 958 in bd6d651

DisposePlatform();

node/src/node_v8_platform-inl.h

Lines 176 to 178 in bd6d651

inline void DisposePlatform() {
per_process::v8_platform.Dispose();
}

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Jan 8, 2020
src: fix what a dispose without checking
4058d53 
If created platform with CreatePlatform, the crash occurs because it does not check if it was initialized to v8_platform when DisposePlatform was called.
@jc-lab jc-lab force-pushed the fix-dispose-without-checking branch from 0641ee3 to 4058d53 Compare January 8, 2020 12:39
joyeecheung
joyeecheung reviewed Jan 8, 2020
View reviewed changes
src/node_v8_platform-inl.h
@@ -99,6 +107,10 @@ struct V8Platform {
}

inline void Dispose() {
if(!initialize_)
Copy link
Member

Choose a reason for hiding this comment

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

Disposing without initializing first sounds like a buggy use case to me. Why do we return instead of aborting here?

Copy link
Author

Choose a reason for hiding this comment

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

node/src/env.cc

Line 958 in bd6d651

DisposePlatform();

Here is the part that calls DisposePlatform globally.

If created through CreatePlatform then v8_platform is not initialized.
So should be ignored instead of Aborting.

Copy link
Member

Choose a reason for hiding this comment

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

In that case I think the state should be maintained in the caller instead of in the callee..

Copy link
Author

Choose a reason for hiding this comment

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

DrainTasks now run after RunAtExit has run.
Is this order of execution important?
If order is not important, I think can Dispose with AtExit.

@Trott
Copy link
Member

Trott commented Jan 10, 2020

@nodejs/v8

@addaleax
Copy link
Member

@Trott Fwiw, the embedders team is probably a better target for pings, if you want to notify somebody – I don’t quite know why the bot tagged this with V8, it’s not directly related.

@jc-lab Thanks for bringing this up – I feel like the issue is a bit larger; in particular, currently we always initialize/tear down the platform in InitializeOncePerProcess()/TearDownOncePerProcess() although I don’t think we should do that, either. However, this PR seems like a step in the right direction either way.

addaleax
addaleax approved these changes Jan 14, 2020
View reviewed changes
src/node_v8_platform-inl.h
@@ -79,8 +80,15 @@ class NodeTraceStateObserver
};

struct V8Platform {
bool initialize_;
Copy link
Member

Choose a reason for hiding this comment

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

@jc-lab Can you call this initialized_ and use a default initializer instead of adding a constructor? i.e.

Suggested change
bool initialize_;
bool initialized_ = false;

src/node_v8_platform-inl.h
@@ -99,6 +107,10 @@ struct V8Platform {
}

inline void Dispose() {
if(!initialize_)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(!initialize_)
if (!initialized_)

@addaleax addaleax added embedding Issues and PRs related to embedding Node.js in another project. and removed v8 engine Issues and PRs related to the V8 dependency. labels Jan 14, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@richardlau
Copy link
Member

@Trott Fwiw, the embedders team is probably a better target for pings, if you want to notify somebody – I don’t quite know why the bot tagged this with V8, it’s not directly related.

The bot tags on the names of the files changed. In this case it matched the following regular expression:
https://github.com/nodejs/github-bot/blob/7026435015b0a9f4bceea21a78b18067dd595e0f/lib/node-labels.js#L26

@Trott
Copy link
Member

Trott commented Jan 18, 2020

@nodejs/embedders

@addaleax addaleax mentioned this pull request Feb 28, 2020
Closed
4 tasks
@addaleax
Copy link
Member

I’ve included this in #30467, along with a fixup commit for the comments above.

addaleax pushed a commit to addaleax/node that referenced this pull request Mar 12, 2020
@addaleax
src: fix what a dispose without checking
36ec54b 
If created platform with CreatePlatform, the crash occurs because
it does not check if it was initialized to v8_platform
when DisposePlatform was called.

Refs: nodejs#31260
addaleax added a commit that referenced this pull request Mar 21, 2020
@addaleax
src: fix what a dispose without checking
0e57674 
If created platform with CreatePlatform, the crash occurs because
it does not check if it was initialized to v8_platform
when DisposePlatform was called.

Refs: #31260
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #30467
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@addaleax
Copy link
Member

This ended up being landed in 0e57674 as part of #30467. Thanks for the PR, @jc-lab! :)

@addaleax addaleax closed this Mar 21, 2020
addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
@addaleax
src: fix what a dispose without checking
691a575 
If created platform with CreatePlatform, the crash occurs because
it does not check if it was initialized to v8_platform
when DisposePlatform was called.

Refs: nodejs#31260
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: nodejs#30467
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
addaleax added a commit that referenced this pull request Sep 23, 2020
@addaleax
src: fix what a dispose without checking
a28c990 
If created platform with CreatePlatform, the crash occurs because
it does not check if it was initialized to v8_platform
when DisposePlatform was called.

Backport-PR-URL: #35241
Refs: #31260
Co-authored-by: Anna Henningsen <[email protected]>
PR-URL: #30467
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jc-lab @Trott @addaleax @nodejs-github-bot @richardlau @joyeecheung