Skip to content

Fix memory leak in by-ref foreach. #12538

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
Closed

Conversation

danog
Copy link
Contributor

@danog danog commented Oct 27, 2023

The issue occurs if a circular reference on the array var of a by-ref foreach is created, all visible references to the array var are removed and the garbage collector is invoked before exiting the foreach.

Invoking the garbage collector removes the array var from the roots, because the foreach statement itself is still holding a reference to it and thus it's still alive, but when the foreach ends it does not re-add the array var to the GC roots, thus the GC cannot collect it and it leaks.

This PR fixes the issue by invoking the GC-aware zval destructor, which correctly adds the zval to the GC roots.

Fixes oss-fuzz #54515.

@danog danog marked this pull request as ready for review October 27, 2023 15:02
@danog
Copy link
Contributor Author

danog commented Oct 27, 2023

PIng @dstogov, could you take a look at this and #12501 please? :)

The issue occurs if a circular reference on the array var of a by-ref foreach is created,
all visible references to the array var are removed and the garbage collector is invoked before exiting the foreach.

Invoking the garbage collector removes the array var from the roots,
because the foreach statement itself is still holding a reference to it and thus it's still alive,
but when the foreach ends it does not re-add the array var to the GC roots, thus the GC cannot collect it and it leaks.

This PR fixes the issue by invoking the GC-aware zval destructor, which correctly adds the zval to the GC roots.

Fixes oss-fuzz #54515.
@danog danog force-pushed the fix_oss_fuzz_54515 branch from cc6498d to bb283ee Compare October 28, 2023 15:00
@dstogov
Copy link
Member

dstogov commented Oct 29, 2023

Please, don't commit this yet.
I thought about the same fix a while ago, but for some reason I decided this fix is not good.
I need to refresh my mind...

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.

After all, this fix is not the best one.
The proper fix is even simple #12572

@danog
Copy link
Contributor Author

danog commented Oct 30, 2023

The proper fix is even simple #12572

Awesome!
Closing this then :)

@danog danog closed this Oct 30, 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