Skip to content

Audit zend_ini_string() and related functions#21146

Open
Girgias wants to merge 13 commits intophp:masterfrom
Girgias:ini-audit-zend_ini_string
Open

Audit zend_ini_string() and related functions#21146
Girgias wants to merge 13 commits intophp:masterfrom
Girgias:ini-audit-zend_ini_string

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Feb 6, 2026

Conflicts with #21143

I audited the usage of zend_ini_string() and co as I believe these functions should return a const char* because they point to a pointer owner by a zend_string *.

The two other things I am wondering is

  • if we should remove the INI_STR() macro as it doesn't follow the usual convention of "string" being a char* and "str" meaning a zend_string*.
  • if we should remove the INI_ORIG_* macros as they seem mostly pointless?

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One caveat with ZEND_STRL() is that it would break if the outer function call was turned into a macro, as it is seen as a single argument during expansion.

An alternative to removing INI_STR(), that would also eliminate the need for ZEND_STRL(), would be to introduce _literal variants of zend_ini_str(ing)(_ex), like we have in the string API, and then remove or deprecate INI_STR(). But then we might want to also add _literal variants of the other zend_ini_ functions to replace all INI_ macros. No strong opinion on this.

@Girgias
Copy link
Member Author

Girgias commented Feb 17, 2026

Looks good to me.

One caveat with ZEND_STRL() is that it would break if the outer function call was turned into a macro, as it is seen as a single argument during expansion.

An alternative to removing INI_STR(), that would also eliminate the need for ZEND_STRL(), would be to introduce _literal variants of zend_ini_str(ing)(_ex), like we have in the string API, and then remove or deprecate INI_STR(). But then we might want to also add _literal variants of the other zend_ini_ functions to replace all INI_ macros. No strong opinion on this.

I quite like the idea of using a new literal API variant, will do this and check what the usage of thoses macros are with sourcegraph to see if it is reasonable to remove them outright or not.

@Girgias Girgias force-pushed the ini-audit-zend_ini_string branch from cb2e357 to 76ebba6 Compare February 23, 2026 13:33
@Girgias Girgias requested a review from derickr as a code owner February 23, 2026 13:33
It is used as a target for snprintf, so must be mutable
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.

4 participants