Skip to content

ext/phar: More minor refactorings #20186

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

Merged
merged 8 commits into from
Oct 16, 2025
Merged

ext/phar: More minor refactorings #20186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6babab5
ext/phar: remove unused parameters for phar_do_403()
Girgias Oct 15, 2025
abe0361
ext/phar: remove unused parameters for phar_do_404()
Girgias Oct 15, 2025
5380c68
ext/phar: use zend_string for f404 param of phar_do_404()
Girgias Oct 15, 2025
02250ef
ext/phar: Use RETURN_BOOL() when possible
Girgias Oct 15, 2025
7bc7810
ext/phar: Do not aquire lifetime of basename parameter in phar_file_a…
Girgias Oct 15, 2025
24dba79
ext/phar/phar_object.c: add some const qualifiers
Girgias Oct 15, 2025
4e41fe2
ext/phar: Refactor phar_create_default_stub()
Girgias Oct 15, 2025
37f456f
ext/phar: start refactoring phar_postprocess_ru_web()
Girgias Oct 16, 2025
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
50 changes: 28 additions & 22 deletions ext/phar/phar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2488,42 +2488,48 @@ static int phar_flush_clean_deleted_apply(zval *zv) /* {{{ */

#include "stub.h" /* Generated phar_get_stub() function from makestub.php script */

zend_string *phar_create_default_stub(const char *index_php, const char *web_index, char **error) /* {{{ */
zend_string *phar_create_default_stub(const zend_string *php_index_str, const zend_string *web_index_str, char **error) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

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

I think the old code in this function is clearer.
High chance that the compiler pre-computes the string length anyway in the !index and !web_index branches, so no need to optimize that.
The changes to "P" instead of "S" in ZPP should be kept though

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I also keep the change where it returns NULL in case the path is over 400 even if error == NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also the other motivation was to get rid of a strlen call that is not needed

Copy link
Member

Choose a reason for hiding this comment

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

Should I also keep the change where it returns NULL in case the path is over 400 even if error == NULL?

Yes

Also the other motivation was to get rid of a strlen call that is not needed

Right, but I think the compiler is smart enough to avoid it.

Anyway, reading this again it's clearer to me now, so you may keep it anyway.

{
size_t index_len, web_len;
const char *php_index;
const char *web_index;
size_t php_len, web_len;

if (error) {
*error = NULL;
}

if (!index_php) {
index_php = "index.php";
}

if (!web_index) {
web_index = "index.php";
}

index_len = strlen(index_php);
web_len = strlen(web_index);

if (index_len > 400) {
/* ridiculous size not allowed for index.php startup filename */
if (error) {
spprintf(error, 0, "Illegal filename passed in for stub creation, was %zd characters long, and only 400 or less is allowed", index_len);
if (!php_index_str) {
php_index = "index.php";
php_len = strlen("index.php");
} else {
php_index = ZSTR_VAL(php_index_str);
php_len = ZSTR_LEN(php_index_str);
if (php_len > 400) {
/* ridiculous size not allowed for index.php startup filename */
if (error) {
spprintf(error, 0, "Illegal filename passed in for stub creation, was %zd characters long, and only 400 or less is allowed", php_len);
}
return NULL;
}
}

if (web_len > 400) {
/* ridiculous size not allowed for index.php startup filename */
if (error) {
spprintf(error, 0, "Illegal web filename passed in for stub creation, was %zd characters long, and only 400 or less is allowed", web_len);
if (!web_index_str) {
web_index = "index.php";
web_len = strlen("index.php");
} else {
web_index = ZSTR_VAL(web_index_str);
web_len = ZSTR_LEN(web_index_str);

if (web_len > 400) {
/* ridiculous size not allowed for index.php startup filename */
if (error) {
spprintf(error, 0, "Illegal web filename passed in for stub creation, was %zd characters long, and only 400 or less is allowed", web_len);
}
return NULL;
}
}

return phar_get_stub(index_php, web_index, index_len+1, web_len+1);
return phar_get_stub(php_index, web_index, php_len+1, web_len+1);
}
/* }}} */

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 @@ -415,7 +415,7 @@ zend_result phar_verify_signature(php_stream *fp, size_t end_of_phar, uint32_t s
zend_result phar_create_signature(phar_archive_data *phar, php_stream *fp, char **signature, size_t *signature_length, char **error);

/* utility functions */
zend_string *phar_create_default_stub(const char *index_php, const char *web_index, char **error);
zend_string *phar_create_default_stub(const zend_string *php_index_str, const zend_string *web_index_str, char **error);
const char *phar_decompress_filter(const phar_entry_info *entry, bool return_unknown);
const char *phar_compress_filter(const phar_entry_info *entry, bool return_unknown);

Expand Down
100 changes: 40 additions & 60 deletions ext/phar/phar_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ static zend_class_entry *phar_ce_entry;
RETURN_THROWS(); \
}

static int phar_file_type(HashTable *mimes, char *file, char **mime_type) /* {{{ */
static int phar_file_type(const HashTable *mimes, const char *file, char **mime_type) /* {{{ */
{
char *ext;
phar_mime_type *mime;
Expand All @@ -65,7 +65,7 @@ static int phar_file_type(HashTable *mimes, char *file, char **mime_type) /* {{{
}
/* }}} */

static void phar_mung_server_vars(char *fname, char *entry, size_t entry_len, char *basename, size_t request_uri_len) /* {{{ */
static void phar_mung_server_vars(char *fname, char *entry, size_t entry_len, const char *basename, size_t request_uri_len) /* {{{ */
{
HashTable *_SERVER;
zval *stuff;
Expand Down Expand Up @@ -151,7 +151,7 @@ static void phar_mung_server_vars(char *fname, char *entry, size_t entry_len, ch
}
/* }}} */

static int phar_file_action(phar_archive_data *phar, phar_entry_info *info, char *mime_type, int code, char *entry, size_t entry_len, char *arch, char *basename, char *ru, size_t ru_len) /* {{{ */
static int phar_file_action(phar_archive_data *phar, phar_entry_info *info, char *mime_type, int code, char *entry, size_t entry_len, char *arch, const char *basename, char *ru, size_t ru_len) /* {{{ */
{
char *name = NULL, buf[8192];
const char *cwd;
Expand All @@ -168,7 +168,6 @@ static int phar_file_action(phar_archive_data *phar, phar_entry_info *info, char

switch (code) {
case PHAR_MIME_PHPS:
efree(basename);
/* highlight source */
if (entry[0] == '/') {
spprintf(&name, 4096, "phar://%s%s", arch, entry);
Expand All @@ -186,7 +185,6 @@ static int phar_file_action(phar_archive_data *phar, phar_entry_info *info, char
zend_bailout();
case PHAR_MIME_OTHER:
/* send headers, output file contents */
efree(basename);
ctr.line_len = spprintf((char **) &(ctr.line), 0, "Content-type: %s", mime_type);
sapi_header_op(SAPI_HEADER_REPLACE, &ctr);
efree((void *) ctr.line);
Expand Down Expand Up @@ -230,7 +228,6 @@ static int phar_file_action(phar_archive_data *phar, phar_entry_info *info, char
case PHAR_MIME_PHP:
if (basename) {
phar_mung_server_vars(arch, entry, entry_len, basename, ru_len);
efree(basename);
}

if (entry[0] == '/') {
Expand Down Expand Up @@ -311,7 +308,7 @@ static int phar_file_action(phar_archive_data *phar, phar_entry_info *info, char
}
/* }}} */

static void phar_do_403(char *entry, size_t entry_len) /* {{{ */
static void phar_do_403(void) /* {{{ */
{
sapi_header_line ctr = {0};

Expand All @@ -325,16 +322,16 @@ static void phar_do_403(char *entry, size_t entry_len) /* {{{ */
}
/* }}} */

static void phar_do_404(phar_archive_data *phar, char *fname, size_t fname_len, char *f404, size_t f404_len, char *entry, size_t entry_len) /* {{{ */
static void phar_do_404(phar_archive_data *phar, char *fname, size_t fname_len, zend_string *f404) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this particular commit is an improvement as it just adds extra NULL checks

Copy link
Member Author

Choose a reason for hiding this comment

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

My main motivation was to make phar_file_action() use zend_strings in the long run, but to do so I need all the inputs to ideally also be zstrs

Copy link
Member

Choose a reason for hiding this comment

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

Does phar_file_action benefit that much from zend_strings?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could propagate it down to phar_mung_server_vars() to use some nicer zstr APIs and prevent a call to ZVAL_STRINGL()

Copy link
Member

Choose a reason for hiding this comment

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

Okay I see, fine then

{
sapi_header_line ctr = {0};
phar_entry_info *info;

if (phar && f404_len) {
info = phar_get_entry_info(phar, f404, f404_len, NULL, true);
if (phar && f404 && ZSTR_LEN(f404)) {
info = phar_get_entry_info(phar, ZSTR_VAL(f404), ZSTR_LEN(f404), NULL, true);

if (info) {
phar_file_action(phar, info, "text/html", PHAR_MIME_PHP, f404, f404_len, fname, NULL, NULL, 0);
phar_file_action(phar, info, "text/html", PHAR_MIME_PHP, ZSTR_VAL(f404), ZSTR_LEN(f404), fname, NULL, NULL, 0);
return;
}
}
Expand All @@ -352,9 +349,10 @@ static void phar_do_404(phar_archive_data *phar, char *fname, size_t fname_len,
/* post-process REQUEST_URI and retrieve the actual request URI. This is for
cases like http://localhost/blah.phar/path/to/file.php/extra/stuff
which calls "blah.phar" file "path/to/file.php" with PATH_INFO "/extra/stuff" */
static void phar_postprocess_ru_web(char *fname, size_t fname_len, char **entry, size_t *entry_len, char **ru, size_t *ru_len) /* {{{ */
static void phar_postprocess_ru_web(const char *fname, size_t fname_len, const char *entry, size_t *entry_len, char **ru, size_t *ru_len) /* {{{ */
{
char *e = *entry + 1, *u = NULL, *u1 = NULL, *saveu = NULL;
const char *e = entry + 1;
char *u = NULL, *u1 = NULL, *saveu = NULL;
size_t e_len = *entry_len - 1, u_len = 0;
phar_archive_data *pphar;

Expand Down Expand Up @@ -559,8 +557,9 @@ PHP_METHOD(Phar, webPhar)
zval *mimeoverride = NULL;
zend_fcall_info rewrite_fci = {0};
zend_fcall_info_cache rewrite_fcc;
char *alias = NULL, *error, *index_php = NULL, *f404 = NULL, *ru = NULL;
size_t alias_len = 0, f404_len = 0, free_pathinfo = 0;
char *alias = NULL, *error, *index_php = NULL, *ru = NULL;
size_t alias_len = 0, free_pathinfo = 0;
zend_string *f404 = NULL;
size_t ru_len = 0;
char *fname, *path_info, *mime_type = NULL, *entry, *pt;
const char *basename;
Expand All @@ -571,7 +570,7 @@ PHP_METHOD(Phar, webPhar)
phar_entry_info *info = NULL;
size_t sapi_mod_name_len = strlen(sapi_module.name);

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|s!s!s!af!", &alias, &alias_len, &index_php, &index_php_len, &f404, &f404_len, &mimeoverride, &rewrite_fci, &rewrite_fcc) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|s!s!S!af!", &alias, &alias_len, &index_php, &index_php_len, &f404, &mimeoverride, &rewrite_fci, &rewrite_fcc) == FAILURE) {
RETURN_THROWS();
}

Expand Down Expand Up @@ -629,7 +628,7 @@ PHP_METHOD(Phar, webPhar)
|| (sapi_mod_name_len == sizeof("litespeed") - 1 && !strncmp(sapi_module.name, "litespeed", sizeof("litespeed") - 1))) {

if (Z_TYPE(PG(http_globals)[TRACK_VARS_SERVER]) != IS_UNDEF) {
HashTable *_server = Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]);
const HashTable *_server = Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]);
zval *z_script_name, *z_path_info;

if (NULL == (z_script_name = zend_hash_str_find(_server, "SCRIPT_NAME", sizeof("SCRIPT_NAME")-1)) ||
Expand Down Expand Up @@ -723,7 +722,7 @@ PHP_METHOD(Phar, webPhar)
break;
case IS_TRUE:
case IS_FALSE:
phar_do_403(entry, entry_len);
phar_do_403();

if (free_pathinfo) {
efree(path_info);
Expand All @@ -748,7 +747,7 @@ PHP_METHOD(Phar, webPhar)
}

if (entry_len) {
phar_postprocess_ru_web(fname, fname_len, &entry, &entry_len, &ru, &ru_len);
phar_postprocess_ru_web(fname, fname_len, entry, &entry_len, &ru, &ru_len);
}

if (!entry_len || (entry_len == 1 && entry[0] == '/')) {
Expand All @@ -769,7 +768,7 @@ PHP_METHOD(Phar, webPhar)

if (FAILURE == phar_get_archive(&phar, fname, fname_len, NULL, 0, NULL) ||
(info = phar_get_entry_info(phar, entry, entry_len, NULL, false)) == NULL) {
phar_do_404(phar, fname, fname_len, f404, f404_len, entry, entry_len);
phar_do_404(phar, fname, fname_len, f404);

if (free_pathinfo) {
efree(path_info);
Expand Down Expand Up @@ -815,7 +814,7 @@ PHP_METHOD(Phar, webPhar)

if (FAILURE == phar_get_archive(&phar, fname, fname_len, NULL, 0, NULL) ||
(info = phar_get_entry_info(phar, entry, entry_len, NULL, false)) == NULL) {
phar_do_404(phar, fname, fname_len, f404, f404_len, entry, entry_len);
phar_do_404(phar, fname, fname_len, f404);
zend_bailout();
}

Expand Down Expand Up @@ -869,6 +868,7 @@ PHP_METHOD(Phar, webPhar)
code = phar_file_type(&PHAR_G(mime_types), entry, &mime_type);
}
phar_file_action(phar, info, mime_type, code, entry, entry_len, fname, pt, ru, ru_len);
efree(pt);

finish: ;
#ifdef PHP_WIN32
Expand Down Expand Up @@ -947,11 +947,11 @@ PHP_METHOD(Phar, interceptFileFuncs)
*/
PHP_METHOD(Phar, createDefaultStub)
{
char *index = NULL, *webindex = NULL, *error;
zend_string *index = NULL, *webindex = NULL;
char *error;
zend_string *stub;
size_t index_len = 0, webindex_len = 0;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|p!p!", &index, &index_len, &webindex, &webindex_len) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|P!P!", &index, &webindex) == FAILURE) {
RETURN_THROWS();
}

Expand Down Expand Up @@ -1029,23 +1029,11 @@ PHP_METHOD(Phar, canCompress)
phar_request_initialize();
switch (method) {
case PHAR_ENT_COMPRESSED_GZ:
if (PHAR_G(has_zlib)) {
RETURN_TRUE;
} else {
RETURN_FALSE;
}
RETURN_BOOL(PHAR_G(has_zlib));
case PHAR_ENT_COMPRESSED_BZ2:
if (PHAR_G(has_bz2)) {
RETURN_TRUE;
} else {
RETURN_FALSE;
}
RETURN_BOOL(PHAR_G(has_bz2));
default:
if (PHAR_G(has_zlib) || PHAR_G(has_bz2)) {
RETURN_TRUE;
} else {
RETURN_FALSE;
}
RETURN_BOOL(PHAR_G(has_zlib) || PHAR_G(has_bz2));
}
}
/* }}} */
Expand Down Expand Up @@ -2576,11 +2564,8 @@ PHP_METHOD(Phar, isWritable)
}

if (SUCCESS != php_stream_stat_path(phar_obj->archive->fname, &ssb)) {
if (phar_obj->archive->is_brandnew) {
/* assume it works if the file doesn't exist yet */
RETURN_TRUE;
}
RETURN_FALSE;
/* assume it works if the file doesn't exist yet */
RETURN_BOOL(phar_obj->archive->is_brandnew);
}

RETURN_BOOL((ssb.sb.st_mode & (S_IWOTH | S_IWGRP | S_IWUSR)) != 0);
Expand Down Expand Up @@ -2930,12 +2915,11 @@ PHP_METHOD(Phar, setStub)
*/
PHP_METHOD(Phar, setDefaultStub)
{
char *index = NULL, *webindex = NULL, *error = NULL;
zend_string *index = NULL, *webindex = NULL;
zend_string *stub = NULL;
size_t index_len = 0, webindex_len = 0;
bool created_stub = false;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|s!s!", &index, &index_len, &webindex, &webindex_len) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|P!P!", &index, &webindex) == FAILURE) {
RETURN_THROWS();
}

Expand Down Expand Up @@ -2963,6 +2947,7 @@ PHP_METHOD(Phar, setDefaultStub)
RETURN_THROWS();
}

char *error = NULL;
if (!phar_obj->archive->is_tar && !phar_obj->archive->is_zip) {
stub = phar_create_default_stub(index, webindex, &error);

Expand Down Expand Up @@ -3130,7 +3115,7 @@ static int phar_set_compression(zval *zv, void *argument) /* {{{ */

static int phar_test_compression(zval *zv, void *argument) /* {{{ */
{
phar_entry_info *entry = (phar_entry_info *)Z_PTR_P(zv);
const phar_entry_info *entry = Z_PTR_P(zv);

if (entry->is_deleted) {
return ZEND_HASH_APPLY_KEEP;
Expand Down Expand Up @@ -3502,27 +3487,22 @@ PHP_METHOD(Phar, copy)
PHP_METHOD(Phar, offsetExists)
{
zend_string *file_name;
phar_entry_info *entry;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "P", &file_name) == FAILURE) {
RETURN_THROWS();
}

PHAR_ARCHIVE_OBJECT();

if (zend_hash_exists(&phar_obj->archive->manifest, file_name)) {
if (NULL != (entry = zend_hash_find_ptr(&phar_obj->archive->manifest, file_name))) {
if (entry->is_deleted) {
/* entry is deleted, but has not been flushed to disk yet */
RETURN_FALSE;
}
}

if (zend_string_starts_with_literal(file_name, ".phar")) {
/* none of these are real files, so they don't exist */
const phar_entry_info *entry = zend_hash_find_ptr(&phar_obj->archive->manifest, file_name);
if (entry != NULL) {
if (entry->is_deleted) {
/* entry is deleted, but has not been flushed to disk yet */
RETURN_FALSE;
}
RETURN_TRUE;

/* none of these are real files, so they don't exist */
RETURN_BOOL(!zend_string_starts_with_literal(file_name, ".phar"));
} else {
/* If the info class is not based on PharFileInfo, directories are not directly instantiable */
if (UNEXPECTED(!instanceof_function(phar_obj->spl.info_class, phar_ce_entry))) {
Expand Down