Skip to content

Mark filename parameter as const for zend_resolve_path() #11003

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Mark filename parameter as const for zend_resolve_path() #11003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

In general, zend_resolve_path() may return the same string and in case of non-interned string this requires incrementation if reference counter.

Copy link
Member Author

Choose a reason for hiding this comment

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

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);?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Right, this makes sense, but are we not doing this in practice because tsrm_realpath() still returns a char * ?

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

ZEND_API zend_result (*zend_post_startup_cb)(void) = NULL;
ZEND_API void (*zend_post_shutdown_cb)(void) = NULL;

Expand Down
4 changes: 2 additions & 2 deletions Zend/zend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ typedef struct _zend_utility_functions {
void (*printf_to_smart_string_function)(smart_string *buf, const char *format, va_list ap);
void (*printf_to_smart_str_function)(smart_str *buf, const char *format, va_list ap);
char *(*getenv_function)(const char *name, size_t name_len);
zend_string *(*resolve_path_function)(zend_string *filename);
zend_string *(*resolve_path_function)(const zend_string *filename);
} zend_utility_functions;

typedef struct _zend_utility_values {
Expand Down Expand Up @@ -338,7 +338,7 @@ extern ZEND_API zend_result (*zend_stream_open_function)(zend_file_handle *handl
extern void (*zend_printf_to_smart_string)(smart_string *buf, const char *format, va_list ap);
extern void (*zend_printf_to_smart_str)(smart_str *buf, const char *format, va_list ap);
extern ZEND_API char *(*zend_getenv)(const char *name, size_t name_len);
extern ZEND_API zend_string *(*zend_resolve_path)(zend_string *filename);
extern ZEND_API zend_string *(*zend_resolve_path)(const zend_string *filename);

/* These two callbacks are especially for opcache */
extern ZEND_API zend_result (*zend_post_startup_cb)(void);
Expand Down
8 changes: 4 additions & 4 deletions ext/opcache/ZendAccelerator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ static zend_op_array *(*accelerator_orig_compile_file)(zend_file_handle *file_ha
static zend_class_entry* (*accelerator_orig_inheritance_cache_get)(zend_class_entry *ce, zend_class_entry *parent, zend_class_entry **traits_and_interfaces);
static zend_class_entry* (*accelerator_orig_inheritance_cache_add)(zend_class_entry *ce, zend_class_entry *proto, zend_class_entry *parent, zend_class_entry **traits_and_interfaces, HashTable *dependencies);
static zend_result (*accelerator_orig_zend_stream_open_function)(zend_file_handle *handle );
static zend_string *(*accelerator_orig_zend_resolve_path)(zend_string *filename);
static zend_string *(*accelerator_orig_zend_resolve_path)(const zend_string *filename);
static zif_handler orig_chdir = NULL;
static ZEND_INI_MH((*orig_include_path_on_modify)) = NULL;
static zend_result (*orig_post_startup_cb)(void);
Expand Down Expand Up @@ -1192,7 +1192,7 @@ zend_result validate_timestamp_and_record_ex(zend_persistent_script *persistent_
/* Instead of resolving full real path name each time we need to identify file,
* we create a key that consist from requested file name, current working
* directory, current include_path, etc */
zend_string *accel_make_persistent_key(zend_string *str)
zend_string *accel_make_persistent_key(const zend_string *str)
{
const char *path = ZSTR_VAL(str);
size_t path_length = ZSTR_LEN(str);
Expand Down Expand Up @@ -1359,7 +1359,7 @@ zend_string *accel_make_persistent_key(zend_string *str)
}

/* not use_cwd */
return str;
return (zend_string*)str;
Copy link
Member

Choose a reason for hiding this comment

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

Here you remove const, so something in your patch is not consistent.

}

/**
Expand Down Expand Up @@ -2517,7 +2517,7 @@ static zend_result persistent_stream_open_function(zend_file_handle *handle)
}

/* zend_resolve_path() replacement for PHP 5.3 and above */
static zend_string* persistent_zend_resolve_path(zend_string *filename)
static zend_string* persistent_zend_resolve_path(const zend_string *filename)
{
if (!file_cache_only &&
ZCG(accelerator_enabled)) {
Expand Down
2 changes: 1 addition & 1 deletion ext/opcache/ZendAccelerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ zend_result zend_accel_invalidate(zend_string *filename, bool force);
zend_result accelerator_shm_read_lock(void);
void accelerator_shm_read_unlock(void);

zend_string *accel_make_persistent_key(zend_string *path);
zend_string *accel_make_persistent_key(const zend_string *path);
zend_op_array *persistent_compile_file(zend_file_handle *file_handle, int type);

#define IS_ACCEL_INTERNED(str) \
Expand Down
4 changes: 2 additions & 2 deletions ext/phar/phar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
static void destroy_phar_data(zval *zv);

ZEND_DECLARE_MODULE_GLOBALS(phar)
static zend_string *(*phar_save_resolve_path)(zend_string *filename);
static zend_string *(*phar_save_resolve_path)(const zend_string *filename);

/**
* set's phar->is_writeable based on the current INI value
Expand Down Expand Up @@ -3286,7 +3286,7 @@ static size_t phar_zend_stream_fsizer(void *handle) /* {{{ */

zend_op_array *(*phar_orig_compile_file)(zend_file_handle *file_handle, int type);

static zend_string *phar_resolve_path(zend_string *filename)
static zend_string *phar_resolve_path(const zend_string *filename)
{
zend_string *ret = phar_find_in_include_path(ZSTR_VAL(filename), ZSTR_LEN(filename), NULL);
if (!ret) {
Expand Down
2 changes: 1 addition & 1 deletion ext/phar/phar_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ char *phar_compress_filter(phar_entry_info * entry, int return_unknown);
/* void phar_remove_virtual_dirs(phar_archive_data *phar, char *filename, size_t filename_len); */
void phar_add_virtual_dirs(phar_archive_data *phar, char *filename, size_t filename_len);
int phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_len, char *path, size_t path_len);
zend_string *phar_find_in_include_path(char *file, size_t file_len, phar_archive_data **pphar);
zend_string *phar_find_in_include_path(const char *file, size_t file_len, phar_archive_data **pphar);
char *phar_fix_filepath(char *path, size_t *new_len, int use_cwd);
phar_entry_info * phar_open_jit(phar_archive_data *phar, phar_entry_info *entry, char **error);
void phar_parse_metadata_lazy(const char *buffer, phar_metadata_tracker *tracker, uint32_t zip_metadata_len, int persistent);
Expand Down
2 changes: 1 addition & 1 deletion ext/phar/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ int phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_le
}
/* }}} */

zend_string *phar_find_in_include_path(char *filename, size_t filename_len, phar_archive_data **pphar) /* {{{ */
zend_string *phar_find_in_include_path(const char *filename, size_t filename_len, phar_archive_data **pphar) /* {{{ */
{
zend_string *ret;
char *path, *fname, *arch, *entry, *test;
Expand Down
2 changes: 1 addition & 1 deletion main/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1571,7 +1571,7 @@ PHPAPI zend_result php_stream_open_for_zend_ex(zend_file_handle *handle, int mod
}
/* }}} */

static zend_string *php_resolve_path_for_zend(zend_string *filename) /* {{{ */
static zend_string *php_resolve_path_for_zend(const zend_string *filename) /* {{{ */
{
return php_resolve_path(ZSTR_VAL(filename), ZSTR_LEN(filename), PG(include_path));
}
Expand Down