Skip to content

Commit

Permalink
Reduce number of string duplications in phar (php#17814)
Browse files Browse the repository at this point in the history 
We don't need to duplicate these strings from the resource, we can just
use them with an offset.
To prove this was safe, I had to make the arguments const and then
propagate that everywhere, so this patch also introduces some more
constness.
  • Loading branch information
@nielsdos
nielsdos authored Feb 15, 2025
1 parent c7d62cf commit 619d293
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 32 deletions.
14 changes: 7 additions & 7 deletions ext/phar/phar_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,8 +420,8 @@ char *phar_decompress_filter(phar_entry_info * entry, int return_unknown);
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);
zend_result phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_len, char *path, size_t path_len);
void phar_add_virtual_dirs(phar_archive_data *phar, const char *filename, size_t filename_len);
zend_result phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_len, const char *path, size_t path_len);
zend_string *phar_find_in_include_path(zend_string *file, 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);
Expand Down Expand Up @@ -463,10 +463,10 @@ extern HashTable cached_alias;
bool phar_archive_delref(phar_archive_data *phar);
void phar_entry_delref(phar_entry_data *idata);

phar_entry_info *phar_get_entry_info(phar_archive_data *phar, char *path, size_t path_len, char **error, int security);
phar_entry_info *phar_get_entry_info_dir(phar_archive_data *phar, char *path, size_t path_len, char dir, char **error, int security);
phar_entry_data *phar_get_or_create_entry_data(char *fname, size_t fname_len, char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security);
zend_result phar_get_entry_data(phar_entry_data **ret, char *fname, size_t fname_len, char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security);
phar_entry_info *phar_get_entry_info(phar_archive_data *phar, const char *path, size_t path_len, char **error, int security);
phar_entry_info *phar_get_entry_info_dir(phar_archive_data *phar, const char *path, size_t path_len, char dir, char **error, int security);
phar_entry_data *phar_get_or_create_entry_data(char *fname, size_t fname_len, const char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security);
zend_result phar_get_entry_data(phar_entry_data **ret, char *fname, size_t fname_len, const char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security);
void phar_flush_ex(phar_archive_data *archive, zend_string *user_stub, bool is_default_stub, char **error);
void phar_flush(phar_archive_data *archive, char **error);
zend_result phar_detect_phar_fname_ext(const char *filename, size_t filename_len, const char **ext_str, size_t *ext_len, int executable, int for_create, int is_complete);
Expand All @@ -484,6 +484,6 @@ typedef enum {
pcr_err_empty_entry
} phar_path_check_result;

phar_path_check_result phar_path_check(char **p, size_t *len, const char **error);
phar_path_check_result phar_path_check(const char **p, size_t *len, const char **error);

END_EXTERN_C()
4 changes: 2 additions & 2 deletions ext/phar/phar_object.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1976,7 +1976,7 @@ static zend_result phar_copy_file_contents(phar_entry_info *entry, php_stream *f
}
/* }}} */

static zend_object *phar_rename_archive(phar_archive_data **sphar, char *ext) /* {{{ */
static zend_object *phar_rename_archive(phar_archive_data **sphar, const char *ext) /* {{{ */
{
const char *oldname = NULL;
phar_archive_data *phar = *sphar;
Expand Down Expand Up @@ -3484,7 +3484,7 @@ PHP_METHOD(Phar, copy)
}

size_t tmp_len = ZSTR_LEN(new_file);
char *tmp_new_file = ZSTR_VAL(new_file);
const char *tmp_new_file = ZSTR_VAL(new_file);
if (phar_path_check(&tmp_new_file, &tmp_len, &pcr_error) > pcr_is_ok) {
zend_throw_exception_ex(spl_ce_UnexpectedValueException, 0,
"file \"%s\" contains invalid characters %s, cannot be copied from \"%s\" in phar %s", ZSTR_VAL(new_file), pcr_error, ZSTR_VAL(old_file), phar_obj->archive->fname);
Expand Down
2 changes: 1 addition & 1 deletion ext/phar/phar_path_check.re
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#include "phar_internal.h"

phar_path_check_result phar_path_check(char **s, size_t *len, const char **error)
phar_path_check_result phar_path_check(const char **s, size_t *len, const char **error)
{
const unsigned char *p = (const unsigned char*)*s;
const unsigned char *m;
Expand Down
21 changes: 5 additions & 16 deletions ext/phar/stream.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
{
phar_archive_data *phar;
phar_entry_data *idata;
char *internal_file;
const char *internal_file;
char *error;
HashTable *pharcontext;
php_url *resource = NULL;
Expand All @@ -189,7 +189,7 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
phar_request_initialize();

/* strip leading "/" */
internal_file = estrndup(ZSTR_VAL(resource->path) + 1, ZSTR_LEN(resource->path) - 1);
internal_file = ZSTR_VAL(resource->path) + 1;
if (mode[0] == 'w' || (mode[0] == 'r' && mode[1] == '+')) {
if (NULL == (idata = phar_get_or_create_entry_data(ZSTR_VAL(resource->host), ZSTR_LEN(resource->host), internal_file, strlen(internal_file), mode, 0, &error, 1))) {
if (error) {
Expand All @@ -198,7 +198,6 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
} else {
php_stream_wrapper_log_error(wrapper, options, "phar error: file \"%s\" could not be created in phar \"%s\"", internal_file, ZSTR_VAL(resource->host));
}
efree(internal_file);
php_url_free(resource);
return NULL;
}
Expand All @@ -207,7 +206,6 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
}
fpf = php_stream_alloc(&phar_ops, idata, NULL, mode);
php_url_free(resource);
efree(internal_file);

if (context && Z_TYPE(context->options) != IS_UNDEF && (pzoption = zend_hash_str_find(HASH_OF(&context->options), "phar", sizeof("phar")-1)) != NULL) {
pharcontext = HASH_OF(pzoption);
Expand Down Expand Up @@ -237,15 +235,13 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
/* retrieve the stub */
if (FAILURE == phar_get_archive(&phar, ZSTR_VAL(resource->host), ZSTR_LEN(resource->host), NULL, 0, NULL)) {
php_stream_wrapper_log_error(wrapper, options, "file %s is not a valid phar archive", ZSTR_VAL(resource->host));
efree(internal_file);
php_url_free(resource);
return NULL;
}
if (phar->is_tar || phar->is_zip) {
if ((FAILURE == phar_get_entry_data(&idata, ZSTR_VAL(resource->host), ZSTR_LEN(resource->host), ".phar/stub.php", sizeof(".phar/stub.php")-1, "r", 0, &error, 0)) || !idata) {
goto idata_error;
}
efree(internal_file);
if (opened_path) {
*opened_path = strpprintf(MAXPATHLEN, "%s", phar->fname);
}
Expand All @@ -256,7 +252,6 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
if (stream == NULL) {
if (UNEXPECTED(FAILURE == phar_open_archive_fp(phar))) {
php_stream_wrapper_log_error(wrapper, options, "phar error: could not reopen phar \"%s\"", ZSTR_VAL(resource->host));
efree(internal_file);
php_url_free(resource);
return NULL;
}
Expand Down Expand Up @@ -285,7 +280,6 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
if (opened_path) {
*opened_path = strpprintf(MAXPATHLEN, "%s", phar->fname);
}
efree(internal_file);
goto phar_stub;
}
}
Expand All @@ -298,7 +292,6 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
} else {
php_stream_wrapper_log_error(wrapper, options, "phar error: \"%s\" is not a file in phar \"%s\"", internal_file, ZSTR_VAL(resource->host));
}
efree(internal_file);
php_url_free(resource);
return NULL;
}
Expand All @@ -317,7 +310,6 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
php_stream_wrapper_log_error(wrapper, options, "%s", error);
efree(error);
phar_entry_delref(idata);
efree(internal_file);
return NULL;
}

Expand All @@ -340,7 +332,6 @@ static php_stream * phar_wrapper_open_url(php_stream_wrapper *wrapper, const cha
if (opened_path) {
*opened_path = zend_strpprintf_unchecked(MAXPATHLEN, "phar://%s/%S", idata->phar->fname, idata->internal_file->filename);
}
efree(internal_file);
phar_stub:
fpf = php_stream_alloc(&phar_ops, idata, NULL, mode);
return fpf;
Expand Down Expand Up @@ -662,7 +653,8 @@ static int phar_wrapper_stat(php_stream_wrapper *wrapper, const char *url, int f
static int phar_wrapper_unlink(php_stream_wrapper *wrapper, const char *url, int options, php_stream_context *context) /* {{{ */
{
php_url *resource;
char *internal_file, *error;
const char *internal_file;
char *error;
int internal_file_len;
phar_entry_data *idata;
phar_archive_data *pphar;
Expand Down Expand Up @@ -695,7 +687,7 @@ static int phar_wrapper_unlink(php_stream_wrapper *wrapper, const char *url, int
}

/* need to copy to strip leading "/", will get touched again */
internal_file = estrndup(ZSTR_VAL(resource->path) + 1, ZSTR_LEN(resource->path) - 1);
internal_file = ZSTR_VAL(resource->path) + 1;
internal_file_len = ZSTR_LEN(resource->path) - 1;
if (FAILURE == phar_get_entry_data(&idata, ZSTR_VAL(resource->host), ZSTR_LEN(resource->host), internal_file, internal_file_len, "r", 0, &error, 1)) {
/* constraints of fp refcount were not met */
Expand All @@ -705,7 +697,6 @@ static int phar_wrapper_unlink(php_stream_wrapper *wrapper, const char *url, int
} else {
php_stream_wrapper_log_error(wrapper, options, "unlink of \"%s\" failed, file does not exist", url);
}
efree(internal_file);
php_url_free(resource);
return 0;
}
Expand All @@ -715,13 +706,11 @@ static int phar_wrapper_unlink(php_stream_wrapper *wrapper, const char *url, int
if (idata->internal_file->fp_refcount > 1) {
/* more than just our fp resource is open for this file */
php_stream_wrapper_log_error(wrapper, options, "phar error: \"%s\" in phar \"%s\", has open file pointers, cannot unlink", internal_file, ZSTR_VAL(resource->host));
efree(internal_file);
php_url_free(resource);
phar_entry_delref(idata);
return 0;
}
php_url_free(resource);
efree(internal_file);
phar_entry_remove(idata, &error);
if (error) {
php_stream_wrapper_log_error(wrapper, options, "%s", error);
Expand Down
12 changes: 6 additions & 6 deletions ext/phar/util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ int phar_seek_efp(phar_entry_info *entry, zend_off_t offset, int whence, zend_of
/* }}} */

/* mount an absolute path or uri to a path internal to the phar archive */
zend_result phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_len, char *path, size_t path_len) /* {{{ */
zend_result phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_len, const char *path, size_t path_len) /* {{{ */
{
phar_entry_info entry = {0};
php_stream_statbuf ssb;
Expand Down Expand Up @@ -472,7 +472,7 @@ static zend_result phar_separate_entry_fp(phar_entry_info *entry, char **error)
* appended, truncated, or read. For read, if the entry is marked unmodified, it is
* assumed that the file pointer, if present, is opened for reading
*/
zend_result phar_get_entry_data(phar_entry_data **ret, char *fname, size_t fname_len, char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security) /* {{{ */
zend_result phar_get_entry_data(phar_entry_data **ret, char *fname, size_t fname_len, const char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security) /* {{{ */
{
phar_archive_data *phar;
phar_entry_info *entry;
Expand Down Expand Up @@ -632,7 +632,7 @@ zend_result phar_get_entry_data(phar_entry_data **ret, char *fname, size_t fname
/**
* Create a new dummy file slot within a writeable phar for a newly created file
*/
phar_entry_data *phar_get_or_create_entry_data(char *fname, size_t fname_len, char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security) /* {{{ */
phar_entry_data *phar_get_or_create_entry_data(char *fname, size_t fname_len, const char *path, size_t path_len, const char *mode, char allow_dir, char **error, int security) /* {{{ */
{
phar_archive_data *phar;
phar_entry_info *entry, etemp;
Expand Down Expand Up @@ -1234,7 +1234,7 @@ char * phar_decompress_filter(phar_entry_info * entry, int return_unknown) /* {{
/**
* retrieve information on a file contained within a phar, or null if it ain't there
*/
phar_entry_info *phar_get_entry_info(phar_archive_data *phar, char *path, size_t path_len, char **error, int security) /* {{{ */
phar_entry_info *phar_get_entry_info(phar_archive_data *phar, const char *path, size_t path_len, char **error, int security) /* {{{ */
{
return phar_get_entry_info_dir(phar, path, path_len, 0, error, security);
}
Expand All @@ -1245,7 +1245,7 @@ phar_entry_info *phar_get_entry_info(phar_archive_data *phar, char *path, size_t
* valid pre-existing empty directory entries
*/
// TODO: convert this to use zend_string too
phar_entry_info *phar_get_entry_info_dir(phar_archive_data *phar, char *path, size_t path_len, char dir, char **error, int security) /* {{{ */
phar_entry_info *phar_get_entry_info_dir(phar_archive_data *phar, const char *path, size_t path_len, char dir, char **error, int security) /* {{{ */
{
const char *pcr_error;
phar_entry_info *entry;
Expand Down Expand Up @@ -2006,7 +2006,7 @@ zend_result phar_create_signature(phar_archive_data *phar, php_stream *fp, char
/* }}} */

// TODO: convert this to zend_string too
void phar_add_virtual_dirs(phar_archive_data *phar, char *filename, size_t filename_len) /* {{{ */
void phar_add_virtual_dirs(phar_archive_data *phar, const char *filename, size_t filename_len) /* {{{ */
{
const char *s;
zend_string *str;
Expand Down

0 comments on commit 619d293

Please sign in to comment.