Skip to content

Fix FETCH_STATIC_PROP_W to prevent returning INDIRECT/UNDEF zval #11046

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

dstogov
Copy link
Member

@dstogov dstogov commented Apr 10, 2023

This behaviour repeats FETCH_OBJ_W behaviour introduced by the typed properties patch.

This behaviour repeats FETCH_OBJ_W behaviour introduced by the typed
properties patch.
@dstogov
Copy link
Member Author

dstogov commented Apr 10, 2023

Note this sets type of the property to NULL that is not valid and the next instruction must change it to valid ARRAY.

I'm not completely sure why @nikic decided non to return UNDEF for FETCH_OBJ_W.
It's not supported by optimizer and JIT, but this may be fixed.
May be there are some more fundamental problems.

For now I decided to fix this in a consistent with FETCH_OBJ_W way.

@dstogov dstogov requested a review from iluuu1994 April 10, 2023 10:07
@nikic
Copy link
Member

nikic commented Apr 10, 2023

I'm not completely sure why @nikic decided non to return UNDEF for FETCH_OBJ_W.

Not sure I understand. Isn't this case handled by zend_handle_fetch_obj_flags with ZEND_FETCH_DIM_WRITE flag, in which case we keep the UNDEF until promotion? Setting it to null doesn't seem right.

@dstogov
Copy link
Member Author

dstogov commented Apr 10, 2023

@nikic
Copy link
Member

nikic commented Apr 10, 2023

I see, you're right. Unfortunately I don't remember why it is implemented in this way.

@dstogov
Copy link
Member Author

dstogov commented Apr 10, 2023

I see, you're right. Unfortunately I don't remember why it is implemented in this way.

OK. I'll try another way, but I afraid it may be dangerous to commit this serious change into stable branch(es).

@dstogov
Copy link
Member Author

dstogov commented Apr 10, 2023

@nikic please take a look at #11048 when you have time

@dstogov dstogov closed this Apr 10, 2023
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.

2 participants