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

src: handle thrown errors in CopyProperties() #8649

Merged
merged 1 commit into from
Sep 20, 2016
Merged

src: handle thrown errors in CopyProperties() #8649

merged 1 commit into from
Sep 20, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Sep 18, 2016

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

src

Description of change

This commit prevents thrown JavaScript exceptions from crashing the process in node_contextify's CopyProperties() function.

Closes #8537

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 18, 2016
@cjihrig cjihrig mentioned this pull request Sep 18, 2016
Closed
@imyller
Copy link
Member

imyller commented Sep 18, 2016

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

bnoordhuis
bnoordhuis reviewed Sep 19, 2016
View reviewed changes
src/node_contextify.cc
@@ -113,7 +113,7 @@ class ContextifyContext {
// removed once there is a better way.
void CopyProperties() {
HandleScope scope(env()->isolate());

TryCatch try_catch(env()->isolate());
Copy link
Member

Choose a reason for hiding this comment

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

You don't need a TryCatch because...

src/node_contextify.cc
@@ -125,7 +125,8 @@ class ContextifyContext {
int length = names->Length();
for (int i = 0; i < length; i++) {
Local<String> key = names->Get(i)->ToString(env()->isolate());
bool has = sandbox_obj->HasOwnProperty(context, key).FromJust();
bool has = sandbox_obj->HasOwnProperty(context, key).FromMaybe(true);
Copy link
Member

@bnoordhuis bnoordhuis Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

...the proper way is to check:

auto maybe_has = sandbox_obj->HasOwnProperty(context, key);
if (!maybe_has.IsJust()) break;  // Exception pending.
auto has = maybe_has.FromJust();
// ...

src/node_contextify.cc
@@ -161,6 +162,9 @@ class ContextifyContext {
clone_property_method->Call(global, arraysize(args), args);
}
}

if (try_catch.HasCaught())
try_catch.ReThrow();
Copy link
Member

@bnoordhuis bnoordhuis Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

You don't need to ReThrow() if you drop the TryCatch, the exception will simply bubble up.

(I believe the caller of CopyProperties() is doing that wrong, too.)

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 19, 2016

Thanks @bnoordhuis. Updated with your suggestions.

addaleax
addaleax reviewed Sep 19, 2016
View reviewed changes
src/node_contextify.cc
if (!maybe_has.IsJust())
break;

auto has = maybe_has.FromJust();
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: I would be okay with just keeping the bool type explicit, using auto here doesn’t save any space and erases information that a reader of the code may find helpful.

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 19, 2016

@addaleax done.

addaleax
addaleax approved these changes Sep 19, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bnoordhuis
Copy link
Member

Tangential: CopyProperties() doesn't look very rigorous to me. GetOwnPropertyNames() returns only enumerable string properties; no symbols, no non-enumerable properties.

Perhaps only enumerable makes some sense because you don't want to copy over globals like Array and Date.

@fhinkel fhinkel added the vm Issues and PRs related to the vm subsystem. label Sep 20, 2016
@fhinkel
Copy link
Member

fhinkel commented Sep 20, 2016

FYI: We made API changes upstream in V8, so CopyProperties() will go away soon.

@fhinkel
Copy link
Member

fhinkel commented Sep 20, 2016

Can you add a regression test?

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 20, 2016

@fhinkel what do you mean? I thought I did (see the other changed file) :-)

@fhinkel
Copy link
Member

fhinkel commented Sep 20, 2016

So sorry, I must have completely missed that! LGTM.

jasnell
jasnell approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

imyller
imyller approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

targos
targos approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

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

@cjihrig
src: handle thrown errors in CopyProperties()
d7994db 
This commit prevents thrown JavaScript exceptions from crashing
the process in node_contextify's CopyProperties() function.

Fixes: nodejs#8537
PR-URL: nodejs#8649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@cjihrig cjihrig merged commit d7994db into nodejs:master Sep 20, 2016
@cjihrig cjihrig deleted the 8537 branch September 20, 2016 23:51
@MylesBorins
Copy link
Contributor

@cjihrig I've set this as do not land as proxies are not on v4.x

Let me know if I'm mistaken here

@bnoordhuis
Copy link
Member

I think it would be good to back-port, it's not just proxies that can cause exceptions.

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@cjihrig @Fishrock123
src: handle thrown errors in CopyProperties()
2dbef79 
This commit prevents thrown JavaScript exceptions from crashing
the process in node_contextify's CopyProperties() function.

Fixes: #8537
PR-URL: #8649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@snyk-bot snyk-bot mentioned this pull request Jul 17, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@imyller imyller imyller approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@fhinkel fhinkel fhinkel approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @imyller @bnoordhuis @fhinkel @jasnell @MylesBorins @addaleax @targos @nodejs-github-bot