Skip to content
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

Fixed GH-17398: Decrease BCG's refcount before error #17594

Open
wants to merge 1 commit into
base: PHP-8.3
Choose a base branch
from

Conversation

SakiTakamachi
Copy link
Member

No description provided.

@SakiTakamachi SakiTakamachi changed the title Fixed GH-17398: Decrease refcount before error Fixed GH-17398: Decrease BCG's refcount before error Jan 27, 2025
…ible

When using `BCG`, when memory is exhausted, the refcount may not decrease
as expected and a memory leak may occur.
@SakiTakamachi SakiTakamachi marked this pull request as ready for review January 27, 2025 10:49
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

I wonder if for master it makes sense to add a custom ZPP specifier like I did for GMP to do the error handling up front.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Doesn't make sense to me.

The fix is incomplete: you only guard the allocation bailout for bc_num2str_ex, but there are many other ways to get an allocation bailout that leaves the refcount of the global numbers incremented, resulting in a leak.

It also increases the complexity quite a bit for an exceptional case, which I don't like.

I think you should fix this by creating a dedicated bc_destroy_num function that destroys a bcmath number regardless of the refcount. This can then be used in GSHUTDOWN. This is safe because we're so far into shutdown that no PHP code could run anyway.

@SakiTakamachi
Copy link
Member Author

SakiTakamachi commented Jan 27, 2025

@nielsdos

Indeed.
First of all, do you think a refcount is even necessary here?

edit:
I guess I was sleepy. ↑

Understood. I will try to discard the BCG regardless of the refcount value.

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