Audit zend_ini_string() and related functions#21146
Audit zend_ini_string() and related functions#21146Girgias wants to merge 13 commits intophp:masterfrom
Conversation
Girgias
force-pushed
the
ini-audit-zend_ini_string
branch
2 times, most recently
from
58f9395 to
5095e2f
Compare
Girgias
force-pushed
the
ini-audit-zend_ini_string
branch
2 times, most recently
from
13411e8 to
cb2e357
Compare
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
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
force-pushed
the
ini-audit-zend_ini_string
branch
from
cb2e357 to
76ebba6
Compare
Girgias
force-pushed
the
ini-audit-zend_ini_string
branch
from
76ebba6 to
fb66db3
Compare
Girgias
requested review from
alexdowad,
devnexen and
youkidearitai
as code owners
| if (cafile == NULL) { | ||
| cafile = zend_ini_string("openssl.cafile", sizeof("openssl.cafile")-1, 0); | ||
| cafile = strlen(cafile) ? cafile : NULL; | ||
| const zend_string *cafile_str = zend_ini_str_literal("openssl.cafile"); |
There was a problem hiding this comment.
I feel this should be zend_ini_string_literal, since you don't actually use the stored length for further processing (i.e. OpenSSL takes a NUL-terminated C string).
There was a problem hiding this comment.
The issue is that this leads to calling to zend_ini_str which returns an empty string if the INI setting exists, but the current code does the strlen() check to determine if it is an empty string or not and sets it back to NULL if so. And I don't want to change the behaviour of now sometimes passing an empty string to openssl.
| if (capath == NULL) { | ||
| capath = zend_ini_string("openssl.capath", sizeof("openssl.capath")-1, 0); | ||
| capath = strlen(capath) ? capath : NULL; | ||
| const zend_string *capath_str = zend_ini_str_literal("openssl.capath"); |
| char *extension_dir; | ||
|
|
||
| extension_dir = INI_STR("extension_dir"); | ||
| zend_string *extension_dir = zend_ini_str_literal("extension_dir"); |
There was a problem hiding this comment.
Similarly this one is used as a C string.
There was a problem hiding this comment.
Sure, but the length of it is recomputed using strlen() so I don't see why we wouldn't just use the known information directly?
Moreover, we could potentially remove the calls to spprintf() and just use the string concat API.
A sourcegraph search shows 0 results other than for the buggy BOOL one in ext/tidy
It is used as a target for snprintf, so must be mutable
…y string rather than NULL in certain cases
Girgias
force-pushed
the
ini-audit-zend_ini_string
branch
from
dfa590f to
a59fa1c
Compare
4 participants
Conflicts with #21143I audited the usage of
zend_ini_string()and co as I believe these functions should return aconst char*because they point to a pointer owner by azend_string *.The two other things I am wondering is
INI_STR()macro as it doesn't follow the usual convention of "string" being achar*and "str" meaning azend_string*.INI_ORIG_*macros as they seem mostly pointless?