Skip to content

Conversation

galpeter
Copy link
Contributor

@galpeter galpeter commented Jun 3, 2020

When the getOwnPropertyDescriptor method was invoked the input property
descriptor was not cleared in every case. This could lead to problems when
the property descriptor is not set/modified by the getOwnPropertyDescriptor
call, resulting in a failure at a later state.

Related to this the Proxy getOwnPropertyDescriptor method incorrectly returned
"undefined" value in a single case.

Fixes: #3837

@galpeter galpeter added the bug Undesired behaviour label Jun 3, 2020
When the getOwnPropertyDescriptor method was invoked the input property
descriptor was not cleared in every case. This could lead to problems when
the property descriptor is not set/modified by the getOwnPropertyDescriptor
call, resulting in a failure at a later state.

Related to this the Proxy getOwnPropertyDescriptor method incorrectly returned
"undefined" value in a single case.

JerryScript-DCO-1.0-Signed-off-by: Peter Gal [email protected]
@zherczeg
Copy link
Member

zherczeg commented Jun 3, 2020

Initializing the descriptior was not a requirement before, so is the ecma_op_object_define_own_property function is wrong or the function who calls it?

@galpeter
Copy link
Contributor Author

galpeter commented Jun 4, 2020

The original problem came from the incorrect proxy implementation which resulted in check after the ecma_op_object_define_own_property call to be false, thus a next instruction incorrectly assumed that the descriptor has some useful data (the input descriptor is a stack allocated variable). The PR fixes the proxy issue and also resets the descriptor in every case to avoid such errors in the future.

In the original ecma_op_object_define_own_property the descriptor will only be inited if the property was found. In some places where the ecma_op_object_define_own_property is used the descriptor is inited to empy in other places it does not. I can revert the initialization if you want it.

Copy link
Member

@rerobika rerobika left a comment

Choose a reason for hiding this comment

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

LGTM with the initialization as well.

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@zherczeg zherczeg merged commit 0d41169 into jerryscript-project:master Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Undesired behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEGV ecma_deref_object (erry-core/ecma/base/ecma-gc.c:149)
3 participants