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

src: harden JSStream callbacks #18028

Closed
wants to merge 2 commits into from
Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jan 7, 2018

Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Ref: #17938 (comment) (/cc @jasnell)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src/js_stream.cc

Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Ref: nodejs#17938 (comment)
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 7, 2018
TryCatch try_catch(env()->isolate());
Local<Value> value;
if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) {
FatalException(env()->isolate(), try_catch);
Copy link
Member

Choose a reason for hiding this comment

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

FatalException() can return so this should probably be followed by an ABORT() or something. The unguarded value->IsTrue() below is not safe, at any rate.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Yea, thanks. 👍 I’ve added an extra return true; here and UV_EPROTO as the default return value for the other places, that’s what tls_wrap.cc uses as a generic failure code (but feel free to let me know of a better choice) – it shouldn’t make any difference in practice, I guess.

@jasnell
Copy link
Member

jasnell commented Jan 9, 2018

@addaleax
Copy link
Member Author

CI failures are all infrastructure failures … Windows is pretty bad but I’d still go ahead with landing this, this isn’t too platform-specific code and it at least does compile successfully there.

@addaleax
Copy link
Member Author

Landed in 36daf1d

@addaleax addaleax closed this Jan 14, 2018
@addaleax addaleax deleted the jsstream-harden branch January 14, 2018 13:58
addaleax added a commit that referenced this pull request Jan 14, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@MylesBorins
Copy link
Contributor

This seems like something we would want to backport, but perhaps wait a bit of time for it to harden. Thoughts?

@MylesBorins
Copy link
Contributor

@addaleax I've backported to 8.x, please lmk if this should be backed out

MylesBorins pushed a commit that referenced this pull request May 22, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Since these are executing JS code, and in particular parts of that
code may be provided by userland, handle such exceptions in C++.

Refs: #17938 (comment)
PR-URL: #18028
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants