Skip to content

Zend: Fix memory leak in ++/-- when overloading fetch access #11859

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 2 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 2, 2023

Addresses #11759 (comment)

Also fixed the comments as per: #11850 (comment)

Comment on lines +35 to +36
Cannot increment array
Cannot decrement array
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why the op type becomes an array, but that seems minor.

Comment on lines 1563 to 1568
if (UNEXPECTED(EG(exception))) {
/* Smart branch expects result to be set with exceptions */
FREE_OP1();
/* opcodes are expected to set the result value */
if (UNEXPECTED(RETURN_VALUE_USED(opline))) {
ZVAL_NULL(EX_VAR(opline->result.var));
}
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you need all this code?
Removing the whole if (UNEXPECTED(EG(exception))) {...} shouldn't change anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think so no, I'm still figuring out the VM at times.

I think my rationale was that I wanted to deal with stuff localy to be sure I understand what is going on. But as nothing else happens here this code can be dropped.

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.

Now this should be right. Check the tests run before merging.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM otherwise, thanks @Girgias!

} while (0);

/* opcodes are expected to set the result value */
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This comment seems rather useless. I'd prefer just removing it. result is primarily assigned for when the result of ++$foo is used. Nobody will understand the relation to the throw helper issue from this comment.

@Girgias Girgias closed this in fc3df28 Aug 2, 2023
@Girgias Girgias deleted the inc-dec-follow-up branch August 2, 2023 17:47
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants