Skip to content

Fix RC assertion in fpm when php_admin_value setting fails - #19671

Closed
iluuu1994 wants to merge 1 commit into
php:PHP-8.3from
iluuu1994:fix-rc-error-in-fpm-php_admin_value-failure
Closed

Fix RC assertion in fpm when php_admin_value setting fails#19671
iluuu1994 wants to merge 1 commit into
php:PHP-8.3from
iluuu1994:fix-rc-error-in-fpm-php_admin_value-failure

Conversation

@iluuu1994

Copy link
Copy Markdown
Member

The value is temporarily duplicated. While the value is allocated persistently, it will be freed if the ini value can't be set. This is safe, given the value has not actually been stored.

Exposed by GH-19619

See https://github.com/php/php-src/actions/runs/17390733646/job/49364059680.

The value is temporarily duplicated. While the value is allocated persistently,
it will be freed if the ini value can't be set. This is safe, given the value
has not actually been stored.
Exposed by phpGH-19619
Comment on lines +44 to 46
/* The string wasn't installed and won't be shared, it's safe to drop. */
GC_MAKE_PERSISTENT_LOCAL(duplicate);
zend_string_release_ex(duplicate, 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alternatively, zend_string_free() might be a little more explicit in that “this is expected to be RC 1 at this point”?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'd honestly prefer not to change behavior for the patch. Should be ok for master.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No strong opinion either way.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@iluuu1994@bukka@TimWolla