Mark filename parameter as const for zend_resolve_path() #11003
Conversation
There was a problem hiding this comment.
The argument was not made const on purpose. To keep an ability to return the copy of argument.
Actually, this should be the usual case for absolute paths without sym-links.
I remember I had some troubles converting php_resolve_path() to usezend_string instead of const char* in PHP-7 times and this opportunity wasn't implemented.
| @@ -85,7 +85,7 @@ ZEND_API void (*zend_error_cb)(int type, zend_string *error_filename, const uint | |||
| void (*zend_printf_to_smart_string)(smart_string *buf, const char *format, va_list ap); | |||
| void (*zend_printf_to_smart_str)(smart_str *buf, const char *format, va_list ap); | |||
| ZEND_API char *(*zend_getenv)(const char *name, size_t name_len); | |||
| ZEND_API zend_string *(*zend_resolve_path)(zend_string *filename); | |||
| ZEND_API zend_string *(*zend_resolve_path)(const zend_string *filename); | |||
There was a problem hiding this comment.
In general, zend_resolve_path() may return the same string and in case of non-interned string this requires incrementation if reference counter.
There was a problem hiding this comment.
Right, this makes sense, but are we not doing this in practice because tsrm_realpath() still returns a char * ?
Also, shouldn't the ZendAccelerator implementation also increment the refcount? Instead of doing what it does by returning the pointer directly instead of zend_string_copy(str);?
There was a problem hiding this comment.
zend_string_copy() just increment the refcount and return the same string. See https://github.com/php/php-src/blob/master/Zend/zend_string.h#L236
There was a problem hiding this comment.
Right, this makes sense, but are we not doing this in practice because
tsrm_realpath()still returns achar *?
Yes. It would be great to switch to zend_string in all related functions, but this is going to be a big and not simple patch.
There was a problem hiding this comment.
zend_string_copy()just increment the refcount and return the same string. See https://github.com/php/php-src/blob/master/Zend/zend_string.h#L236
Yes I know this, my question is why in ZendAccelerator.c the string is returned directly without incrementing the refcount, as after your explanation it seems this should be done.
| @@ -1359,7 +1359,7 @@ zend_string *accel_make_persistent_key(zend_string *str) | |||
| } | |||
|
|
|||
| /* not use_cwd */ | |||
| return str; | |||
| return (zend_string*)str; | |||
There was a problem hiding this comment.
Here you remove const, so something in your patch is not consistent.
|
Closing as it doesn't make sense in practice as the string might need to increment the refcount. I will look into switch the relevant functions from |
Because it shouldn't be modified, and I hit this while working on something related to #8294 as I was passing a const filename.