Skip to content

Fix GH-12380: JIT+private array property access inside closure accesses private property in child class #12381

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Oct 7, 2023

For private fields, the scope has to be taken into account, otherwise the property info may come from the wrong ce.
trace_ce is the class entry for b in the test, so we look up the property info from the wrong ce (should be a).

Maybe this can be improved further by checking is_derived_class(ce, scope) and then reading the property info from the scope, e.g. by using return zend_get_parent_private_property(op_array->scope, ce, member); instead of return NULL? But that API isn't public though.

…esses private property in child class

For private fields, the scope has to be taken into account, otherwise
the property info may come from the wrong ce.
@nielsdos
Copy link
Member Author

nielsdos commented Oct 7, 2023

Maybe this can be improved further by checking is_derived_class(ce, scope) and then reading the property info from the scope, e.g. by using return zend_get_parent_private_property(op_array->scope, ce, member); instead of return NULL? But that API isn't public though.

Here's how that idea would look like (apply patch on top of this PR):

https://gist.github.com/nielsdos/3090a86982cba83124ffc8715ff3e031

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

Great work!

@nielsdos
Copy link
Member Author

nielsdos commented Oct 9, 2023

Thank you for reviewing.
@dstogov my PR returns NULL in the else branch. I proposed https://gist.github.com/nielsdos/3090a86982cba83124ffc8715ff3e031 additionally. But that changes the API (makes it public) so that is probably not ok for stable branches. Do you think the additional patch may be applied to master?

@dstogov
Copy link
Member

dstogov commented Oct 9, 2023

Thank you for reviewing. @dstogov my PR returns NULL in the else branch. I proposed https://gist.github.com/nielsdos/3090a86982cba83124ffc8715ff3e031 additionally. But that changes the API (makes it public) so that is probably not ok for stable branches. Do you think the additional patch may be applied to master?

Yes. It's not OK to break API in old versions.
I didn't understand why do you use zend_get_parent_private_property(). Can you explain?
May be it's better to just commit this and then create new PE for master with explanation test.

@nielsdos nielsdos closed this in fb68387 Oct 9, 2023
@nielsdos
Copy link
Member Author

nielsdos commented Oct 9, 2023

Merged as-is.

I didn't understand why do you use zend_get_parent_private_property(). Can you explain?

Please disregard, I see now this is wrong, I guess I was confused.
Thanks for checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT+private array property access inside closure accesses private property in child class
2 participants