Skip to content

ext/phar: Refactor Phar to prevent unnecessary strlen() computations #11033

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 3 commits into from
Closed

ext/phar: Refactor Phar to prevent unnecessary strlen() computations #11033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3fa8cd
Add case insensitive versions of the zend_string_starts_with_* APIs
Girgias Apr 7, 2023
398d64d
ext/phar: Prevent unnecessary known string length computation
Girgias Apr 7, 2023
af93a08
ext/phar: Remove duplicate cleaning-up code
Girgias Apr 7, 2023
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
13 changes: 13 additions & 0 deletions Zend/zend_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,19 @@ static zend_always_inline bool zend_string_starts_with(const zend_string *str, c
#define zend_string_starts_with_literal(str, prefix) \
zend_string_starts_with_cstr(str, prefix, strlen(prefix))

static zend_always_inline bool zend_string_starts_with_cstr_ci(const zend_string *str, const char *prefix, size_t prefix_length)
{
return ZSTR_LEN(str) >= prefix_length && !strncasecmp(ZSTR_VAL(str), prefix, prefix_length);
}

static zend_always_inline bool zend_string_starts_with_ci(const zend_string *str, const zend_string *prefix)
{
return zend_string_starts_with_cstr_ci(str, ZSTR_VAL(prefix), ZSTR_LEN(prefix));
}

#define zend_string_starts_with_literal_ci(str, prefix) \
zend_string_starts_with_cstr(str, prefix, strlen(prefix))

/*
* DJBX33A (Daniel J. Bernstein, Times 33 with Addition)
*
Expand Down
69 changes: 34 additions & 35 deletions ext/phar/func_interceptors.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,17 @@ PHAR_FUNC(phar_opendir) /* {{{ */
}

if (!IS_ABSOLUTE_PATH(filename, filename_len) && !strstr(filename, "://")) {
char *arch, *entry, *fname;
size_t arch_len, entry_len, fname_len;
fname = (char*)zend_get_executed_filename();
char *arch, *entry;
size_t arch_len, entry_len;
zend_string *fname = zend_get_executed_filename_ex();

/* we are checking for existence of a file within the relative path. Chances are good that this is
retrieving something from within the phar archive */

if (strncasecmp(fname, "phar://", 7)) {
if (!zend_string_starts_with_literal_ci(fname, "phar://")) {
goto skip_phar;
}
fname_len = strlen(fname);
if (SUCCESS == phar_split_fname(fname, fname_len, &arch, &arch_len, &entry, &entry_len, 2, 0)) {

if (SUCCESS == phar_split_fname(ZSTR_VAL(fname), ZSTR_LEN(fname), &arch, &arch_len, &entry, &entry_len, 2, 0)) {
php_stream_context *context = NULL;
php_stream *stream;
char *name;
Expand Down Expand Up @@ -91,15 +90,17 @@ PHAR_FUNC(phar_opendir) /* {{{ */

static zend_string* phar_get_name_for_relative_paths(zend_string *filename, bool using_include_path)
{
char *arch, *entry, *fname;
size_t arch_len, entry_len, fname_len;
char *arch, *entry;
size_t arch_len, entry_len;
zend_string *fname = zend_get_executed_filename_ex();

fname = (char*)zend_get_executed_filename();
if (strncasecmp(fname, "phar://", 7)) {
/* we are checking for existence of a file within the relative path. Chances are good that this is
retrieving something from within the phar archive */
if (!zend_string_starts_with_literal_ci(fname, "phar://")) {
return NULL;
}
fname_len = strlen(fname);
if (FAILURE == phar_split_fname(fname, fname_len, &arch, &arch_len, &entry, &entry_len, 2, 0)) {

if (FAILURE == phar_split_fname(ZSTR_VAL(fname), ZSTR_LEN(fname), &arch, &arch_len, &entry, &entry_len, 2, 0)) {
return NULL;
}

Expand Down Expand Up @@ -485,22 +486,22 @@ static void phar_file_stat(const char *filename, size_t filename_length, int typ
}

if (!IS_ABSOLUTE_PATH(filename, filename_length) && !strstr(filename, "://")) {
char *arch, *entry, *fname;
size_t arch_len, entry_len, fname_len;
char *arch, *entry;
size_t arch_len, entry_len;
zend_string *fname;
zend_stat_t sb = {0};
phar_entry_info *data = NULL;
phar_archive_data *phar;

fname = (char*)zend_get_executed_filename();
fname = zend_get_executed_filename_ex();

/* we are checking for existence of a file within the relative path. Chances are good that this is
retrieving something from within the phar archive */

if (strncasecmp(fname, "phar://", 7)) {
if (!zend_string_starts_with_literal_ci(fname, "phar://")) {
goto skip_phar;
}
fname_len = strlen(fname);
if (PHAR_G(last_phar) && fname_len - 7 >= PHAR_G(last_phar_name_len) && !memcmp(fname + 7, PHAR_G(last_phar_name), PHAR_G(last_phar_name_len))) {

if (PHAR_G(last_phar) && ZSTR_LEN(fname) - 7 >= PHAR_G(last_phar_name_len) && !memcmp(ZSTR_VAL(fname) + 7, PHAR_G(last_phar_name), PHAR_G(last_phar_name_len))) {
arch = estrndup(PHAR_G(last_phar_name), PHAR_G(last_phar_name_len));
arch_len = PHAR_G(last_phar_name_len);
entry = estrndup(filename, filename_length);
Expand All @@ -509,7 +510,7 @@ static void phar_file_stat(const char *filename, size_t filename_length, int typ
phar = PHAR_G(last_phar);
goto splitted;
}
if (SUCCESS == phar_split_fname(fname, fname_len, &arch, &arch_len, &entry, &entry_len, 2, 0)) {
if (SUCCESS == phar_split_fname(ZSTR_VAL(fname), ZSTR_LEN(fname), &arch, &arch_len, &entry, &entry_len, 2, 0)) {

efree(entry);
entry = estrndup(filename, filename_length);
Expand Down Expand Up @@ -741,18 +742,17 @@ PHAR_FUNC(phar_is_file) /* {{{ */
goto skip_phar;
}
if (!IS_ABSOLUTE_PATH(filename, filename_len) && !strstr(filename, "://")) {
char *arch, *entry, *fname;
size_t arch_len, entry_len, fname_len;
fname = (char*)zend_get_executed_filename();
char *arch, *entry;
size_t arch_len, entry_len;
zend_string *fname = zend_get_executed_filename_ex();

/* we are checking for existence of a file within the relative path. Chances are good that this is
retrieving something from within the phar archive */

if (strncasecmp(fname, "phar://", 7)) {
if (!zend_string_starts_with_literal_ci(fname, "phar://")) {
goto skip_phar;
}
fname_len = strlen(fname);
if (SUCCESS == phar_split_fname(fname, fname_len, &arch, &arch_len, &entry, &entry_len, 2, 0)) {

if (SUCCESS == phar_split_fname(ZSTR_VAL(fname), ZSTR_LEN(fname), &arch, &arch_len, &entry, &entry_len, 2, 0)) {
phar_archive_data *phar;

efree(entry);
Expand Down Expand Up @@ -808,18 +808,17 @@ PHAR_FUNC(phar_is_link) /* {{{ */
goto skip_phar;
}
if (!IS_ABSOLUTE_PATH(filename, filename_len) && !strstr(filename, "://")) {
char *arch, *entry, *fname;
size_t arch_len, entry_len, fname_len;
fname = (char*)zend_get_executed_filename();
char *arch, *entry;
size_t arch_len, entry_len;
zend_string *fname = zend_get_executed_filename_ex();

/* we are checking for existence of a file within the relative path. Chances are good that this is
retrieving something from within the phar archive */

if (strncasecmp(fname, "phar://", 7)) {
if (!zend_string_starts_with_literal_ci(fname, "phar://")) {
goto skip_phar;
}
fname_len = strlen(fname);
if (SUCCESS == phar_split_fname(fname, fname_len, &arch, &arch_len, &entry, &entry_len, 2, 0)) {

if (SUCCESS == phar_split_fname(ZSTR_VAL(fname), ZSTR_LEN(fname), &arch, &arch_len, &entry, &entry_len, 2, 0)) {
phar_archive_data *phar;

efree(entry);
Expand Down
26 changes: 10 additions & 16 deletions ext/phar/phar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2318,24 +2318,17 @@ int phar_split_fname(const char *filename, size_t filename_len, char **arch, siz
*/
int phar_open_executed_filename(char *alias, size_t alias_len, char **error) /* {{{ */
{
char *fname;
php_stream *fp;
size_t fname_len;
zend_string *actual = NULL;
int ret;

if (error) {
*error = NULL;
}

fname = (char*)zend_get_executed_filename();
fname_len = strlen(fname);
zend_string *fname = zend_get_executed_filename_ex();

if (phar_open_parsed_phar(fname, fname_len, alias, alias_len, 0, REPORT_ERRORS, NULL, 0) == SUCCESS) {
if (phar_open_parsed_phar(ZSTR_VAL(fname), ZSTR_LEN(fname), alias, alias_len, 0, REPORT_ERRORS, NULL, 0) == SUCCESS) {
return SUCCESS;
}

if (!strcmp(fname, "[no active file]")) {
if (zend_string_equals_literal(fname, "[no active file]")) {
Copy link
Member

Choose a reason for hiding this comment

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

This no longer makes sense, this string is only returned in zend_get_executed_filename(). This would now return NULL (and thus already segfault above).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah indeed, surprised we don't have a test for that... AFAIK this only happens with eval()?

if (error) {
spprintf(error, 0, "cannot initialize a phar outside of PHP execution");
}
Expand All @@ -2349,15 +2342,17 @@ int phar_open_executed_filename(char *alias, size_t alias_len, char **error) /*
return FAILURE;
}

if (php_check_open_basedir(fname)) {
if (php_check_open_basedir(ZSTR_VAL(fname))) {
return FAILURE;
}

fp = php_stream_open_wrapper(fname, "rb", IGNORE_URL|STREAM_MUST_SEEK|REPORT_ERRORS, &actual);
zend_string *actual = NULL;
php_stream *fp;
fp = php_stream_open_wrapper(ZSTR_VAL(fname), "rb", IGNORE_URL|STREAM_MUST_SEEK|REPORT_ERRORS, &actual);

if (!fp) {
if (error) {
spprintf(error, 0, "unable to open phar for reading \"%s\"", fname);
spprintf(error, 0, "unable to open phar for reading \"%s\"", ZSTR_VAL(fname));
}
if (actual) {
zend_string_release_ex(actual, 0);
Expand All @@ -2366,11 +2361,10 @@ int phar_open_executed_filename(char *alias, size_t alias_len, char **error) /*
}

if (actual) {
fname = ZSTR_VAL(actual);
fname_len = ZSTR_LEN(actual);
fname = actual;
}

ret = phar_open_from_fp(fp, fname, fname_len, alias, alias_len, REPORT_ERRORS, NULL, 0, error);
int ret = phar_open_from_fp(fp, ZSTR_VAL(fname), ZSTR_LEN(fname), alias, alias_len, REPORT_ERRORS, NULL, 0, error);

if (actual) {
zend_string_release_ex(actual, 0);
Expand Down
49 changes: 24 additions & 25 deletions ext/phar/phar_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,21 +392,24 @@ static void phar_postprocess_ru_web(char *fname, size_t fname_len, char **entry,
*/
PHP_METHOD(Phar, running)
{
char *fname, *arch, *entry;
size_t fname_len, arch_len, entry_len;
zend_string *fname;
char *arch, *entry;
size_t arch_len, entry_len;
bool retphar = 1;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|b", &retphar) == FAILURE) {
RETURN_THROWS();
}

fname = (char*)zend_get_executed_filename();
fname_len = strlen(fname);
fname = zend_get_executed_filename_ex();

if (fname_len > 7 && !memcmp(fname, "phar://", 7) && SUCCESS == phar_split_fname(fname, fname_len, &arch, &arch_len, &entry, &entry_len, 2, 0)) {
if (
zend_string_starts_with_literal_ci(fname, "phar://")
&& SUCCESS == phar_split_fname(ZSTR_VAL(fname), ZSTR_LEN(fname), &arch, &arch_len, &entry, &entry_len, 2, 0)
) {
efree(entry);
if (retphar) {
RETVAL_STRINGL(fname, arch_len + 7);
RETVAL_STRINGL(ZSTR_VAL(fname), arch_len + 7);
efree(arch);
return;
} else {
Expand Down Expand Up @@ -441,8 +444,9 @@ PHP_METHOD(Phar, mount)
RETURN_THROWS();
}

fname = (char*)zend_get_executed_filename();
fname_len = strlen(fname);
zend_string *zend_file_name = zend_get_executed_filename_ex();
fname = ZSTR_VAL(zend_file_name);
fname_len = ZSTR_LEN(zend_file_name);

#ifdef PHP_WIN32
save_fname = fname;
Expand Down Expand Up @@ -482,15 +486,6 @@ PHP_METHOD(Phar, mount)
carry_on:
if (SUCCESS != phar_mount_entry(pphar, actual, actual_len, path, path_len)) {
zend_throw_exception_ex(phar_ce_PharException, 0, "Mounting of %s to %s within phar %s failed", path, actual, arch);
if (path && path == entry) {
efree(entry);
}

if (arch) {
efree(arch);
}

goto finish;
Comment on lines -485 to -493
Copy link
Member

Choose a reason for hiding this comment

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

Why this is removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The code outside the if branch is equivalent to this and performs the necessary clean-up and then jumps to finish

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I see

}

if (entry && path && path == entry) {
Expand Down Expand Up @@ -556,8 +551,6 @@ PHP_METHOD(Phar, webPhar)
}

phar_request_initialize();
fname = (char*)zend_get_executed_filename();
fname_len = strlen(fname);

if (phar_open_executed_filename(alias, alias_len, &error) != SUCCESS) {
if (error) {
Expand All @@ -583,6 +576,10 @@ PHP_METHOD(Phar, webPhar)
return;
}

zend_string *zend_file_name = zend_get_executed_filename_ex();
fname = ZSTR_VAL(zend_file_name);
fname_len = ZSTR_LEN(zend_file_name);

#ifdef PHP_WIN32
if (memchr(fname, '\\', fname_len)) {
fname = estrndup(fname, fname_len);
Expand Down Expand Up @@ -1274,9 +1271,9 @@ PHP_METHOD(Phar, getSupportedCompression)
/* {{{ Completely remove a phar archive from memory and disk */
PHP_METHOD(Phar, unlinkArchive)
{
char *fname, *error, *zname, *arch, *entry;
char *fname, *error, *arch, *entry;
size_t fname_len;
size_t zname_len, arch_len, entry_len;
size_t arch_len, entry_len;
phar_archive_data *phar;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "p", &fname, &fname_len) == FAILURE) {
Expand All @@ -1298,11 +1295,13 @@ PHP_METHOD(Phar, unlinkArchive)
RETURN_THROWS();
}

zname = (char*)zend_get_executed_filename();
zname_len = strlen(zname);
zend_string *zend_file_name = zend_get_executed_filename_ex();

if (zname_len > 7 && !memcmp(zname, "phar://", 7) && SUCCESS == phar_split_fname(zname, zname_len, &arch, &arch_len, &entry, &entry_len, 2, 0)) {
if ((size_t)arch_len == fname_len && !memcmp(arch, fname, arch_len)) {
if (
zend_string_starts_with_literal_ci(zend_file_name, "phar://")
&& SUCCESS == phar_split_fname(ZSTR_VAL(zend_file_name), ZSTR_LEN(zend_file_name), &arch, &arch_len, &entry, &entry_len, 2, 0)
) {
if (arch_len == fname_len && !memcmp(arch, fname, arch_len)) {
zend_throw_exception_ex(phar_ce_PharException, 0, "phar archive \"%s\" cannot be unlinked from within itself", fname);
efree(arch);
efree(entry);
Expand Down
20 changes: 13 additions & 7 deletions ext/phar/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,8 @@ int phar_mount_entry(phar_archive_data *phar, char *filename, size_t filename_le
zend_string *phar_find_in_include_path(zend_string *filename, phar_archive_data **pphar) /* {{{ */
{
zend_string *ret;
char *path, *fname, *arch, *entry, *test;
size_t arch_len, entry_len, fname_len;
char *path, *arch, *entry, *test;
size_t arch_len, entry_len;
phar_archive_data *phar;

if (pphar) {
Expand All @@ -256,17 +256,23 @@ zend_string *phar_find_in_include_path(zend_string *filename, phar_archive_data
return NULL;
}

fname = (char*)zend_get_executed_filename();
fname_len = strlen(fname);
zend_string *fname = zend_get_executed_filename_ex();
Copy link
Member

Choose a reason for hiding this comment

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

Contrary to zend_get_executed_filename() zend_get_executed_filename_ex() may return NULL. I don't know if this code can be triggered outside of user context. The same goes for the calls above.

bool is_file_a_phar_wrapper = zend_string_starts_with_literal_ci(fname, "phar://");
size_t length_phar_protocol = strlen("phar://");

if (PHAR_G(last_phar) && !memcmp(fname, "phar://", 7) && fname_len - 7 >= PHAR_G(last_phar_name_len) && !memcmp(fname + 7, PHAR_G(last_phar_name), PHAR_G(last_phar_name_len))) {
if (
PHAR_G(last_phar)
&& is_file_a_phar_wrapper
&& ZSTR_LEN(fname) - length_phar_protocol >= PHAR_G(last_phar_name_len)
&& !memcmp(ZSTR_VAL(fname) + length_phar_protocol, PHAR_G(last_phar_name), PHAR_G(last_phar_name_len))
) {
arch = estrndup(PHAR_G(last_phar_name), PHAR_G(last_phar_name_len));
arch_len = PHAR_G(last_phar_name_len);
phar = PHAR_G(last_phar);
goto splitted;
}

if (fname_len < 7 || memcmp(fname, "phar://", 7) || SUCCESS != phar_split_fname(fname, strlen(fname), &arch, &arch_len, &entry, &entry_len, 1, 0)) {
if (!is_file_a_phar_wrapper || SUCCESS != phar_split_fname(ZSTR_VAL(fname), ZSTR_LEN(fname), &arch, &arch_len, &entry, &entry_len, 1, 0)) {
return NULL;
}

Expand Down Expand Up @@ -310,7 +316,7 @@ zend_string *phar_find_in_include_path(zend_string *filename, phar_archive_data
ret = php_resolve_path(ZSTR_VAL(filename), ZSTR_LEN(filename), path);
efree(path);

if (ret && ZSTR_LEN(ret) > 8 && !strncmp(ZSTR_VAL(ret), "phar://", 7)) {
if (ret && zend_string_starts_with_literal_ci(ret, "phar://")) {
/* found phar:// */
if (SUCCESS != phar_split_fname(ZSTR_VAL(ret), ZSTR_LEN(ret), &arch, &arch_len, &entry, &entry_len, 1, 0)) {
return ret;
Expand Down