Skip to content
Permalink
Browse files

Refactor php_url struct to save memory dup in common cases

  • Loading branch information...
laruence committed Aug 6, 2017
1 parent 6546c51 commit 513b0093c2b480bb752fb354012f42c446769486
@@ -210,7 +210,7 @@ static int php_curl_option_url(php_curl *ch, const char *url, const size_t len)
return FAILURE;
}

if (uri->scheme && !strncasecmp("file", uri->scheme, sizeof("file"))) {
if (uri->scheme && zend_string_equals_literal_ci(uri->scheme, "file")) {

This comment has been minimized.

Copy link
@kohler

kohler Aug 4, 2018

Contributor

Won't this behave differently if the scheme begins with "file" but does not equal "file"? (because strncasecmp)

php_error_docref(NULL, E_WARNING, "Protocol 'file' disabled in cURL");
php_url_free(uri);
return FAILURE;
@@ -532,17 +532,18 @@ void php_filter_validate_url(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */
RETURN_VALIDATION_FAILED
}

if (url->scheme != NULL && (!strcasecmp(url->scheme, "http") || !strcasecmp(url->scheme, "https"))) {
if (url->scheme != NULL &&
(zend_string_equals_literal_ci(url->scheme, "http") || zend_string_equals_literal_ci(url->scheme, "https"))) {
char *e, *s, *t;
size_t l;

if (url->host == NULL) {
goto bad_url;
}

s = url->host;
l = strlen(s);
e = url->host + l;
s = ZSTR_VAL(url->host);
l = ZSTR_LEN(url->host);
e = s + l;
t = e - 1;

/* An IPv6 enclosed by square brackets is a valid hostname */
@@ -552,7 +553,7 @@ void php_filter_validate_url(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */
}

// Validate domain
if (!_php_filter_validate_domain(url->host, l, FILTER_FLAG_HOSTNAME)) {
if (!_php_filter_validate_domain(ZSTR_VAL(url->host), l, FILTER_FLAG_HOSTNAME)) {
php_url_free(url);
RETURN_VALIDATION_FAILED
}
@@ -561,7 +562,7 @@ void php_filter_validate_url(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */
if (
url->scheme == NULL ||
/* some schemas allow the host to be empty */
(url->host == NULL && (strcmp(url->scheme, "mailto") && strcmp(url->scheme, "news") && strcmp(url->scheme, "file"))) ||
(url->host == NULL && (strcmp(ZSTR_VAL(url->scheme), "mailto") && strcmp(ZSTR_VAL(url->scheme), "news") && strcmp(ZSTR_VAL(url->scheme), "file"))) ||
((flags & FILTER_FLAG_PATH_REQUIRED) && url->path == NULL) || ((flags & FILTER_FLAG_QUERY_REQUIRED) && url->query == NULL)
) {
bad_url:
@@ -2581,9 +2581,9 @@ static char *php_openssl_get_url_name(const char *resourcename,
}

if (url->host) {
const char * host = url->host;
const char * host = ZSTR_VAL(url->host);
char * url_name = NULL;
size_t len = strlen(host);
size_t len = ZSTR_LEN(url->host);

/* skip trailing dots */
while (len && host[len-1] == '.') {
@@ -319,7 +319,7 @@ php_stream *phar_wrapper_open_dir(php_stream_wrapper *wrapper, const char *path,
/* we must have at the very least phar://alias.phar/ */
if (!resource->scheme || !resource->host || !resource->path) {
if (resource->host && !resource->path) {
php_stream_wrapper_log_error(wrapper, options, "phar error: no directory in \"%s\", must have at least phar://%s/ for root directory (always use full path to a new phar)", path, resource->host);
php_stream_wrapper_log_error(wrapper, options, "phar error: no directory in \"%s\", must have at least phar://%s/ for root directory (always use full path to a new phar)", path, ZSTR_VAL(resource->host));
php_url_free(resource);
return NULL;
}
@@ -328,22 +328,22 @@ php_stream *phar_wrapper_open_dir(php_stream_wrapper *wrapper, const char *path,
return NULL;
}

if (strcasecmp("phar", resource->scheme)) {
if (!zend_string_equals_literal_ci(resource->scheme, "phar")) {
php_url_free(resource);
php_stream_wrapper_log_error(wrapper, options, "phar error: not a phar url \"%s\"", path);
return NULL;
}

host_len = strlen(resource->host);
host_len = ZSTR_LEN(resource->host);
phar_request_initialize();
internal_file = resource->path + 1; /* strip leading "/" */
internal_file = ZSTR_VAL(resource->path) + 1; /* strip leading "/" */

if (FAILURE == phar_get_archive(&phar, resource->host, host_len, NULL, 0, &error)) {
if (FAILURE == phar_get_archive(&phar, ZSTR_VAL(resource->host), host_len, NULL, 0, &error)) {
if (error) {
php_stream_wrapper_log_error(wrapper, options, "%s", error);
efree(error);
} else {
php_stream_wrapper_log_error(wrapper, options, "phar file \"%s\" is unknown", resource->host);
php_stream_wrapper_log_error(wrapper, options, "phar file \"%s\" is unknown", ZSTR_VAL(resource->host));
}
php_url_free(resource);
return NULL;
@@ -446,48 +446,48 @@ int phar_wrapper_mkdir(php_stream_wrapper *wrapper, const char *url_from, int mo
return 0;
}

if (strcasecmp("phar", resource->scheme)) {
if (!zend_string_equals_literal_ci(resource->scheme, "phar")) {
php_url_free(resource);
php_stream_wrapper_log_error(wrapper, options, "phar error: not a phar stream url \"%s\"", url_from);
return 0;
}

host_len = strlen(resource->host);
host_len = ZSTR_LEN(resource->host);

if (FAILURE == phar_get_archive(&phar, resource->host, host_len, NULL, 0, &error)) {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", error retrieving phar information: %s", resource->path+1, resource->host, error);
if (FAILURE == phar_get_archive(&phar, ZSTR_VAL(resource->host), host_len, NULL, 0, &error)) {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", error retrieving phar information: %s", resource->path+1, ZSTR_VAL(resource->host), error);
efree(error);
php_url_free(resource);
return 0;
}

if ((e = phar_get_entry_info_dir(phar, resource->path + 1, strlen(resource->path + 1), 2, &error, 1))) {
if ((e = phar_get_entry_info_dir(phar, ZSTR_VAL(resource->path) + 1, ZSTR_LEN(resource->path) - 1, 2, &error, 1))) {
/* directory exists, or is a subdirectory of an existing file */
if (e->is_temp_dir) {
efree(e->filename);
efree(e);
}
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", directory already exists", resource->path+1, resource->host);
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", directory already exists", ZSTR_VAL(resource->path)+1, ZSTR_VAL(resource->host));
php_url_free(resource);
return 0;
}

if (error) {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", %s", resource->path+1, resource->host, error);
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", %s", ZSTR_VAL(resource->path)+1, ZSTR_VAL(resource->host), error);
efree(error);
php_url_free(resource);
return 0;
}

if (phar_get_entry_info_dir(phar, resource->path + 1, strlen(resource->path + 1), 0, &error, 1)) {
if (phar_get_entry_info_dir(phar, ZSTR_VAL(resource->path) + 1, ZSTR_LEN(resource->path) - 1, 0, &error, 1)) {
/* entry exists as a file */
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", file already exists", resource->path+1, resource->host);
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", file already exists", ZSTR_VAL(resource->path)+1, ZSTR_VAL(resource->host));
php_url_free(resource);
return 0;
}

if (error) {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", %s", resource->path+1, resource->host, error);
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot create directory \"%s\" in phar \"%s\", %s", ZSTR_VAL(resource->path)+1, ZSTR_VAL(resource->host), error);
efree(error);
php_url_free(resource);
return 0;
@@ -500,14 +500,14 @@ int phar_wrapper_mkdir(php_stream_wrapper *wrapper, const char *url_from, int mo
entry.is_zip = 1;
}

entry.filename = estrdup(resource->path + 1);
entry.filename = estrdup(ZSTR_VAL(resource->path) + 1);

if (phar->is_tar) {
entry.is_tar = 1;
entry.tar_type = TAR_DIR;
}

entry.filename_len = strlen(resource->path + 1);
entry.filename_len = ZSTR_LEN(resource->path) - 1;
php_url_free(resource);
entry.is_dir = 1;
entry.phar = phar;
@@ -581,29 +581,29 @@ int phar_wrapper_rmdir(php_stream_wrapper *wrapper, const char *url, int options
return 0;
}

if (strcasecmp("phar", resource->scheme)) {
if (!zend_string_equals_literal_ci(resource->scheme, "phar")) {
php_url_free(resource);
php_stream_wrapper_log_error(wrapper, options, "phar error: not a phar stream url \"%s\"", url);
return 0;
}

host_len = strlen(resource->host);
host_len = ZSTR_LEN(resource->host);

if (FAILURE == phar_get_archive(&phar, resource->host, host_len, NULL, 0, &error)) {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot remove directory \"%s\" in phar \"%s\", error retrieving phar information: %s", resource->path+1, resource->host, error);
if (FAILURE == phar_get_archive(&phar, ZSTR_VAL(resource->host), host_len, NULL, 0, &error)) {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot remove directory \"%s\" in phar \"%s\", error retrieving phar information: %s", ZSTR_VAL(resource->path)+1, ZSTR_VAL(resource->host), error);
efree(error);
php_url_free(resource);
return 0;
}

path_len = strlen(resource->path+1);
path_len = ZSTR_LEN(resource->path) - 1;

if (!(entry = phar_get_entry_info_dir(phar, resource->path + 1, path_len, 2, &error, 1))) {
if (!(entry = phar_get_entry_info_dir(phar, ZSTR_VAL(resource->path) + 1, path_len, 2, &error, 1))) {
if (error) {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot remove directory \"%s\" in phar \"%s\", %s", resource->path+1, resource->host, error);
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot remove directory \"%s\" in phar \"%s\", %s", ZSTR_VAL(resource->path)+1, ZSTR_VAL(resource->host), error);
efree(error);
} else {
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot remove directory \"%s\" in phar \"%s\", directory does not exist", resource->path+1, resource->host);
php_stream_wrapper_log_error(wrapper, options, "phar error: cannot remove directory \"%s\" in phar \"%s\", directory does not exist", ZSTR_VAL(resource->path)+1, ZSTR_VAL(resource->host));
}
php_url_free(resource);
return 0;
@@ -615,7 +615,7 @@ int phar_wrapper_rmdir(php_stream_wrapper *wrapper, const char *url, int options
zend_hash_move_forward(&phar->manifest)
) {
if (ZSTR_LEN(str_key) > path_len &&
memcmp(ZSTR_VAL(str_key), resource->path+1, path_len) == 0 &&
memcmp(ZSTR_VAL(str_key), ZSTR_VAL(resource->path)+1, path_len) == 0 &&
IS_SLASH(ZSTR_VAL(str_key)[path_len])) {
php_stream_wrapper_log_error(wrapper, options, "phar error: Directory not empty");
if (entry->is_temp_dir) {
@@ -632,7 +632,7 @@ int phar_wrapper_rmdir(php_stream_wrapper *wrapper, const char *url, int options
zend_hash_move_forward(&phar->virtual_dirs)) {

if (ZSTR_LEN(str_key) > path_len &&
memcmp(ZSTR_VAL(str_key), resource->path+1, path_len) == 0 &&
memcmp(ZSTR_VAL(str_key), ZSTR_VAL(resource->path)+1, path_len) == 0 &&
IS_SLASH(ZSTR_VAL(str_key)[path_len])) {
php_stream_wrapper_log_error(wrapper, options, "phar error: Directory not empty");
if (entry->is_temp_dir) {
@@ -646,7 +646,7 @@ int phar_wrapper_rmdir(php_stream_wrapper *wrapper, const char *url, int options
}

if (entry->is_temp_dir) {
zend_hash_str_del(&phar->virtual_dirs, resource->path+1, path_len);
zend_hash_str_del(&phar->virtual_dirs, ZSTR_VAL(resource->path)+1, path_len);
efree(entry->filename);
efree(entry);
} else {

17 comments on commit 513b009

@Majkl578

This comment has been minimized.

Copy link
Contributor

replied Aug 3, 2018

Hi @laurence, this commit introduced a strange PHAR-related bug affecting PHPUnit: https://bugs.php.net/bug.php?id=76510

@Majkl578

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2018

Since mailing list is still unreliable/down, I'll make a bit of a noise here:
@cmb69 @smalyshev This commit completely breaks PHARs (#76510), incl. Composer, PHPUnit, PHPStan, Infection etc. Imho this should be a blocker for RC1 and fixed/reverted ASAP.

@smalyshev

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2018

Looks like revert candidate. @laruence, any comments before we revert it?

@cmb69

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2018

I don't think that we should revert for 7.3.0beta3 which already has been tagged yesterday, and is scheduled for release tomorrow – there are likely too many conflicts, and resolving these might introduce yet other issues. Instead we should preferably fix the existing issues, and revert before 7.3.0RC1 if we fail to do so.

@smalyshev

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2018

No, I meant reverting from master, what is tagged is tagged. I'm ok with waiting for a bit for @laruence to supply the fixes, but if that doesn't happen in a couple of days, I think reverting and working in a branch until it's stable is the way to go.

@cmb69

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2018

@smalyshev You've certainly meant to revert in PHP-7.3 (and maybe also master).

@smalyshev

This comment has been minimized.

Copy link
Contributor

replied Aug 29, 2018

Yes, both.

@nikic

This comment has been minimized.

Copy link
Member

replied Aug 30, 2018

This is a heavy ABI and source level change, I don't think reverting it in the beta phase is a good idea.

@cmb69

This comment has been minimized.

Copy link
Contributor

replied Aug 30, 2018

Thanks, @nikic! I'd rather not revert this, but we need a fix for bug #76510, which has been reported more than two months ago, and hints at memory management errors.

@smalyshev

This comment has been minimized.

Copy link
Contributor

replied Aug 30, 2018

Well, it breaks PHAR, so what alternative do we have beyond reverting it? If the patch is broken, it needs to be reverted. We cannot ship 7.3 with phar completely broken.

@remicollet

This comment has been minimized.

Copy link
Contributor

replied Aug 31, 2018

Reverting this change will imply another API bump (not a real issue), but also another extension mass rebuild, and some extensions are already fixed for current API...

@cmb69

This comment has been minimized.

Copy link
Contributor

replied Aug 31, 2018

Is it possible that the issue is caused by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86914?

@Majkl578

This comment has been minimized.

Copy link
Contributor

replied Aug 31, 2018

It's possible, since this is related to a strlen() optimization. I can try compiling with different GCC versions to see what happens.

@cmb69

This comment has been minimized.

Copy link
Contributor

replied Aug 31, 2018

It's not only that the bug is about a strlen() optimization, but rather that struct _zend_string and the +1 offset is rather similar to what is reported there.

@smalyshev

This comment has been minimized.

Copy link
Contributor

replied Aug 31, 2018

@remicollet given that it breaks a major component, I don't think "but we'd have to recompile extensions" is an important consideration. Before we have .0 GA release, the presumption should be every build is preliminary anyway.

@Majkl578

This comment has been minimized.

Copy link
Contributor

replied Sep 6, 2018

Debian now ships gcc-8=8.2.0-5[unstable] which contains a backported fix for this GCC bug (verified to be working):
https://tracker.debian.org/news/984783/accepted-gcc-8-820-5-source-into-unstable/ (PR tree-optimization/86914)
It's also scheduled for inclusion in regular GCC 8.3 release.

Instead of reverting this altogether (and breaking ABI), would it be possible to disable the specific optimization only for GCC 8.1 and 8.2 (if no bugfix lands in php-src)?

@cmb69

This comment has been minimized.

Copy link
Contributor

replied Sep 6, 2018

Thanks for checking this! Instead of generally disabling the optimization for some GCC versions, I think we could AC_TRY_RUN something like this:

#include <stdlib.h>
#include <string.h>
#include <stdio.h>

struct s
{
    int i;
    char c[1];
};

int main()
{
    struct s *s = malloc(sizeof(struct s) + 3);
    s->i = 3;
    strcpy(s->c, "foo");
    return strlen(s->c+1) == 2;
}
Please sign in to comment.
You can’t perform that action at this time.