src: handle thrown errors in CopyProperties()#8649
Conversation
nodejs-github-bot
added
the
c++
src/node_contextify.cc
Outdated
There was a problem hiding this comment.
You don't need a TryCatch because...
src/node_contextify.cc
Outdated
There was a problem hiding this comment.
...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
Outdated
There was a problem hiding this comment.
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.)
|
Thanks @bnoordhuis. Updated with your suggestions. |
src/node_contextify.cc
Outdated
There was a problem hiding this comment.
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.
|
@addaleax done. |
|
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. |
|
FYI: We made API changes upstream in V8, so CopyProperties() will go away soon. |
|
Can you add a regression test? |
|
@fhinkel what do you mean? I thought I did (see the other changed file) :-) |
|
So sorry, I must have completely missed that! LGTM. |
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 I've set this as do not land as proxies are not on v4.x Let me know if I'm mistaken here |
|
I think it would be good to back-port, it's not just proxies that can cause exceptions. |
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>
9 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
src
Description of change
This commit prevents thrown JavaScript exceptions from crashing the process in
node_contextify'sCopyProperties()function.Closes #8537