-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Speculative fix for napi_set_property crash #10842
Conversation
Identifier identifier = keyProp.toPropertyKey(globalObject); | ||
RETURN_IF_EXCEPTION(scope, napi_generic_failure); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need this exception check in napi_get_property
before calling getIfPropertyExists
JSValue jsValue = toJS(value); | ||
|
||
object->put(object, globalObject, identifier, jsValue, slot); | ||
RETURN_IF_EXCEPTION(scope, napi_generic_failure); | ||
if (!object->put(object, globalObject, identifier, jsValue, slot)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this check added? How is put
returning false different from an exception thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments. Looks good, just confused why we have a check for false
from obj->put
What does this PR do?
We have received many crash reports for
napi_set_property
.The crash implies that
target
is set to undefined (08
) and we are calling.getObject()
on it, without properly validating that it is an objectNode.js' implementation of
napi_set_property
checks target is an object viaCHECK_TO_OBJECT(env, context, obj, object);
.This PR makes
napi_set_property
check that the property is an object first.We also disable strict mode when putting the property. Since Node doesn't default to strict mode, it's a good bet napi_set_property shouldn't either.
How did you verify your code works?
No tests