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: bcmul memory leak #17615

Closed
wants to merge 2 commits into from

Conversation

SakiTakamachi
Copy link
Member

take2 of #17594

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.

See my comments.
Also a small nit: normally our commit subjects are formatted as Fix GH-<nr>: <issue title> instead of Fixed and instead of the description what you changed.

*num = NULL;
}

void bc_force_free_numbers(void)
Copy link
Member

Choose a reason for hiding this comment

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

During GSHUTDOWN you should use the passed bcmath_globals instead of BCG to avoid freeing the wrong data in ZTS. In fact, If I were you I'd call _bc_force_free_number directly in GSHUTDOWN.

@@ -82,6 +82,20 @@ void bc_init_numbers(void)
BCG(_two_)->n_value[0] = 2;
}

static void _bc_force_free_number(bc_num *num)
Copy link
Member

Choose a reason for hiding this comment

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

Function names starting with underscores should be avoided in new code because in C a name starting with underscore is reserved.

@SakiTakamachi SakiTakamachi changed the title Fixed GH-17398: Fixed BCG release processing of PHP_GSHUTDOWN_FUNCTION() Fixed GH-17398: bcmul memory leak Jan 29, 2025
@SakiTakamachi
Copy link
Member Author

@nielsdos
Thanks, fixed!

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.

Seems right to me, and works locally. Please wait for CI before merging.
Also be careful when merging up: there is only one allocation for bc_num in 8.4 and up so you'll have to adjust the freeing code upon merging.

SakiTakamachi added a commit that referenced this pull request Jan 29, 2025
* PHP-8.3:
  Fixed GH-17398: bcmul memory leak (#17615)
SakiTakamachi added a commit that referenced this pull request Jan 29, 2025
* PHP-8.4:
  Fixed GH-17398: bcmul memory leak (#17615)
@SakiTakamachi SakiTakamachi deleted the fix/gh17398_2 branch January 29, 2025 10:16
SakiTakamachi added a commit that referenced this pull request Jan 29, 2025
The deleted line has returned, so delete it again.
SakiTakamachi added a commit that referenced this pull request Jan 29, 2025
* PHP-8.4:
  follow up for #17615
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.

2 participants