Skip to content

standard: various nit and refactorings for standard stream wrappers #22875

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

Open
Girgias wants to merge 3 commits into
base: master
Choose a base branch
from
+74 −74
Open

standard: various nit and refactorings for standard stream wrappers #22875

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
85bcf64
standard/http_fopen_wrapper: use zend_string for Location header value
Girgias Jul 23, 2026
3aaadc6
Fix new test
Girgias Sep 6, 2026
7db3ad8
fix other test which required posix and pcntl
Girgias Sep 6, 2026
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
102 changes: 51 additions & 51 deletions ext/standard/http_fopen_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,9 @@ static zend_result php_stream_handle_proxy_authorization_header(const char *s, s
typedef struct _php_stream_http_response_header_info {
php_stream_filter *transfer_encoding;
size_t file_size;
zend_string *location;
bool error;
bool follow_location;
char *location;
size_t location_len;
} php_stream_http_response_header_info;

static void php_stream_http_response_header_info_init(
Expand Down Expand Up @@ -281,13 +280,11 @@ static zend_string *php_stream_http_response_headers_parse(php_stream_wrapper *w
zend_string_efree(last_header_line);
return NULL;
}
if (header_info->location_len == 0) {
header_info->location = emalloc(last_header_value_len + 1);
} else if (header_info->location_len <= last_header_value_len) {
header_info->location = erealloc(header_info->location, last_header_value_len + 1);
/* Previous location header encountered */
if (UNEXPECTED(header_info->location )) {
zend_string_release_ex(header_info->location, false);
}
header_info->location_len = last_header_value_len;
memcpy(header_info->location, last_header_value, last_header_value_len + 1);
header_info->location = zend_string_init(last_header_value, last_header_value_len, false);
} else if (zend_string_starts_with_literal_ci(last_header_line, "Content-Type:")) {
php_stream_notify_info(context, PHP_STREAM_NOTIFY_MIME_TYPE_IS, last_header_value, 0);
} else if (zend_string_starts_with_literal_ci(last_header_line, "Content-Length:")) {
Expand Down Expand Up @@ -1019,7 +1016,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
break;
}
/* Save current line as the last line so it gets parsed in the next round. */
last_header_line_str = zend_string_init(http_header_line, http_header_line_length, 0);
last_header_line_str = zend_string_init_fast(http_header_line, http_header_line_length);
} else {
break;
}
Expand All @@ -1037,7 +1034,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
}

if (header_info.location != NULL)
php_stream_notify_info(context, PHP_STREAM_NOTIFY_REDIRECTED, header_info.location, 0);
php_stream_notify_info(context, PHP_STREAM_NOTIFY_REDIRECTED, ZSTR_VAL(header_info.location), 0);

php_stream_close(stream);
stream = NULL;
Expand All @@ -1048,63 +1045,66 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
}

if (header_info.location != NULL) {
zend_string *new_path = NULL;

/* Redirection location must be a URI with schema,
* if we don't have a schema, the location is within the existing schema and host */
if (
!zend_string_starts_with_literal_ci(header_info.location, "http://") &&
!zend_string_starts_with_literal_ci(header_info.location, "https://") &&
!zend_string_starts_with_literal_ci(header_info.location, "ftp://") &&
!zend_string_starts_with_literal_ci(header_info.location, "ftps://")
) {
zend_string *loc_path = NULL;

char *new_path = NULL;

if (header_info.location_len < 8 ||
(strncasecmp(header_info.location, "http://", sizeof("http://")-1) &&
strncasecmp(header_info.location, "https://", sizeof("https://")-1) &&
strncasecmp(header_info.location, "ftp://", sizeof("ftp://")-1) &&
strncasecmp(header_info.location, "ftps://", sizeof("ftps://")-1)))
{
char *loc_path = NULL;
if (*header_info.location != '/') {
if (header_info.location_len > 1 && resource->path) {
/* if we don't have an absolute location we need to determine it */
if (ZSTR_VAL(header_info.location)[0] != '/') {
if (ZSTR_VAL(header_info.location)[1] != '\0' && resource->path) {
/* find last '/' to determine relative path */
char *s = strrchr(ZSTR_VAL(resource->path), '/');
if (!s) {
s = ZSTR_VAL(resource->path);
if (!ZSTR_LEN(resource->path)) {
zend_string_release_ex(resource->path, 0);
resource->path = ZSTR_INIT_LITERAL("/", 0);
s = ZSTR_VAL(resource->path);
} else {
*s = '/';
}
}
s[1] = '\0';
if (resource->path &&
ZSTR_VAL(resource->path)[0] == '/' &&
ZSTR_VAL(resource->path)[1] == '\0') {
spprintf(&loc_path, 0, "%s%s", ZSTR_VAL(resource->path), header_info.location);
/* No path segment, just prefix relative location with '/' */
loc_path = zend_string_concat2(
ZEND_STRL("/"),
ZSTR_VAL(header_info.location), ZSTR_LEN(header_info.location)
);
} else {
spprintf(&loc_path, 0, "%s/%s", ZSTR_VAL(resource->path), header_info.location);
size_t offset_to_last_slash = s - ZSTR_VAL(resource->path);
ZEND_ASSERT(ZSTR_VAL(resource->path)[offset_to_last_slash] == '/');
loc_path = zend_string_concat2(
ZSTR_VAL(resource->path), offset_to_last_slash + 1,
ZSTR_VAL(header_info.location), ZSTR_LEN(header_info.location)
);
}
} else {
spprintf(&loc_path, 0, "/%s", header_info.location);
loc_path = zend_string_concat2(
ZEND_STRL("/"),
ZSTR_VAL(header_info.location), ZSTR_LEN(header_info.location)
);
}
} else {
loc_path = header_info.location;
header_info.location = NULL;
}
if ((use_ssl && resource->port != 443) || (!use_ssl && resource->port != 80)) {
spprintf(&new_path, 0, "%s://%s:" ZEND_LONG_FMT "%s", ZSTR_VAL(resource->scheme),
ZSTR_VAL(resource->host), resource->port, loc_path);
new_path = zend_strpprintf(0, "%s://%s:" ZEND_LONG_FMT "%s", ZSTR_VAL(resource->scheme),
ZSTR_VAL(resource->host), resource->port, ZSTR_VAL(loc_path));
} else {
spprintf(&new_path, 0, "%s://%s%s", ZSTR_VAL(resource->scheme),
ZSTR_VAL(resource->host), loc_path);
new_path = zend_strpprintf(0, "%s://%s%s", ZSTR_VAL(resource->scheme),
ZSTR_VAL(resource->host), ZSTR_VAL(loc_path));
}
efree(loc_path);
zend_string_release_ex(loc_path, false);
} else {
new_path = header_info.location;
header_info.location = NULL;
}

php_uri_struct_free(resource);
/* check for invalid redirection URLs */
if ((resource = php_uri_parse_to_struct(uri_parser, new_path, strlen(new_path), PHP_URI_COMPONENT_READ_MODE_RAW, true)) == NULL) {
if ((resource = php_uri_parse_to_struct(uri_parser, ZSTR_VAL(new_path), ZSTR_LEN(new_path), PHP_URI_COMPONENT_READ_MODE_RAW, true)) == NULL) {
php_stream_wrapper_log_warn(wrapper, context, options, InvalidUrl,
"Invalid redirect URL! %s", new_path);
efree(new_path);
"Invalid redirect URL! %s", ZSTR_VAL(new_path));
zend_string_release_ex(new_path, false);
goto out;
}

Expand All @@ -1116,16 +1116,16 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
while (s < e) { \
if (iscntrl(*s)) { \
php_stream_wrapper_log_warn(wrapper, context, options, InvalidUrl, \
"Invalid redirect URL! %s", new_path); \
efree(new_path); \
"Invalid redirect URL! %s", ZSTR_VAL(new_path)); \
zend_string_release_ex(new_path, false); \
goto out; \
} \
s++; \
} \
} \
}
/* check for control characters in login, password & path */
if (strncasecmp(new_path, "http://", sizeof("http://") - 1) || strncasecmp(new_path, "https://", sizeof("https://") - 1)) {
if (zend_string_starts_with_literal_ci(new_path, "http://") || zend_string_starts_with_literal_ci(new_path, "https://")) {

@devnexen devnexen Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it seems the old condition was always true regardless ? I would either drop the "migrated" version of it or

if (!zend_string_starts_with_literal_ci(new_path, "http://") && !zend_string_starts_with_literal_ci(new_path, "https://"))
```. wdyt ?

@Girgias Girgias Jul 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm... not even sure what the semantics are. Reading the rest of the code it does seem to me that it expects the path to start with an HTTP protocol schema....

As we are passing new_path to php_stream_url_wrap_http_ex

@devnexen devnexen Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No you re right.

CHECK_FOR_CNTRL_CHARS(resource->user);

@Sjord Sjord Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This checks for control characters in the output from php_uri_parse_to_struct. When using the URL default parser, the control characters have already been replaced by underscores when we get here (php_replace_controlchars in php_url_parse_ex2). When setting the parser to Uri\Rfc3986\Uri, the URL fails to parse. With Uri\WhatWg\Url, it is possible to get control characters in the URL.

The check used to be performed for all URLs, but now is only performed for http(s) URLs. So this makes it possible to redirect to e.g. FTP URLs that contain control characters.

But perhaps it should be up to the URL parser to decide how to handle control characters? If WhatWg thinks URLs can have control characters, should we really discard them here?

Edit: I tested this with this on the server:

<?php
header("Location: ftp://u\vser:pass@localhost:41303/");

And this as the client:

<?php
$context = stream_context_create([
    "http" => [
        "uri_parser_class" => \Uri\WhatWg\Url::class,
    ]
]);
file_get_contents("http://localhost:41303/", false, $context);

@Girgias Girgias Sep 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess this makes sense, but might leave this for a follow-up PR and do this after 8.6 is branched as this might be contentious?

@Sjord Sjord Sep 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I agree. There's no need to change that in this PR.

CHECK_FOR_CNTRL_CHARS(resource->password);
CHECK_FOR_CNTRL_CHARS(resource->path);
Expand All @@ -1138,9 +1138,9 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
new_flags |= HTTP_WRAPPER_KEEP_METHOD;
}
stream = php_stream_url_wrap_http_ex(
wrapper, new_path, mode, options, opened_path, context,
wrapper, ZSTR_VAL(new_path), mode, options, opened_path, context,
--redirect_max, new_flags, response_header STREAMS_CC);
efree(new_path);
zend_string_release_ex(new_path, false);
} else {
php_stream_wrapper_log_warn(wrapper, context, options, ProtocolError,
"HTTP request failed! %s", tmp_line);
Expand All @@ -1155,7 +1155,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
}

if (header_info.location != NULL) {
efree(header_info.location);
zend_string_release_ex(header_info.location, false);
}

if (resource) {
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/tests/http/http_empty_location_redirect.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ include sprintf("%s/../../../openssl/tests/ServerClientTestCase.inc", __DIR__);
ServerClientTestCase::getInstance()->run($clientCode, $serverCode);
?>
--EXPECT--
uri=/
uri=/a/
44 changes: 22 additions & 22 deletions ext/standard/tests/http/http_relative_redirect.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@ http_server_kill($pid);
--EXPECT--
# URI parser:
Redirect from '/dir/page' to '/dir/page': /dir/page
Redirect from '/dir/page' to 'dir/page': /dir//dir/page
Redirect from '/dir/page' to 'dir/page': /dir/dir/page
Redirect from '/dir/page' to 'a': /a
Redirect from '/dir/page' to 'other': /dir//other
Redirect from '/dir/page' to '': /
Redirect from '/dir/page' to '../../../foo': /dir//../../../foo
Redirect from '/dir/page' to 'space bar': /dir//space bar
Redirect from '/dir/page' to 'other': /dir/other
Redirect from '/dir/page' to '': /dir/
Redirect from '/dir/page' to '../../../foo': /dir/../../../foo
Redirect from '/dir/page' to 'space bar': /dir/space bar
Redirect from '/a' to '/dir/page': /dir/page
Redirect from '/a' to 'dir/page': /dir/page
Redirect from '/a' to 'a': /a
Expand All @@ -85,12 +85,12 @@ Redirect from '' to '': /
Redirect from '' to '../../../foo': /../../../foo
Redirect from '' to 'space bar': /space bar
Redirect from '/../../../foo' to '/dir/page': /dir/page
Redirect from '/../../../foo' to 'dir/page': /../../..//dir/page
Redirect from '/../../../foo' to 'dir/page': /../../../dir/page
Redirect from '/../../../foo' to 'a': /a
Redirect from '/../../../foo' to 'other': /../../..//other
Redirect from '/../../../foo' to '': /
Redirect from '/../../../foo' to '../../../foo': /../../..//../../../foo
Redirect from '/../../../foo' to 'space bar': /../../..//space bar
Redirect from '/../../../foo' to 'other': /../../../other
Redirect from '/../../../foo' to '': /../../../
Redirect from '/../../../foo' to '../../../foo': /../../../../../../foo
Redirect from '/../../../foo' to 'space bar': /../../../space bar
Redirect from '/space bar' to '/dir/page': /dir/page
Redirect from '/space bar' to 'dir/page': /dir/page
Redirect from '/space bar' to 'a': /a
Expand All @@ -100,11 +100,11 @@ Redirect from '/space bar' to '../../../foo': /../../../foo
Redirect from '/space bar' to 'space bar': /space bar
# URI parser: Uri\Rfc3986\Uri
Redirect from '/dir/page' to '/dir/page': /dir/page
Redirect from '/dir/page' to 'dir/page': /dir//dir/page
Redirect from '/dir/page' to 'dir/page': /dir/dir/page
Redirect from '/dir/page' to 'a': /a
Redirect from '/dir/page' to 'other': /dir//other
Redirect from '/dir/page' to '': /
Redirect from '/dir/page' to '../../../foo': /dir//../../../foo
Redirect from '/dir/page' to 'other': /dir/other
Redirect from '/dir/page' to '': /dir/
Redirect from '/dir/page' to '../../../foo': /dir/../../../foo
Redirect from '/dir/page' to 'space bar': failed
Redirect from '/a' to '/dir/page': /dir/page
Redirect from '/a' to 'dir/page': /dir/page
Expand All @@ -128,11 +128,11 @@ Redirect from '' to '': /
Redirect from '' to '../../../foo': /../../../foo
Redirect from '' to 'space bar': failed
Redirect from '/../../../foo' to '/dir/page': /dir/page
Redirect from '/../../../foo' to 'dir/page': /../../..//dir/page
Redirect from '/../../../foo' to 'dir/page': /../../../dir/page
Redirect from '/../../../foo' to 'a': /a
Redirect from '/../../../foo' to 'other': /../../..//other
Redirect from '/../../../foo' to '': /
Redirect from '/../../../foo' to '../../../foo': /../../..//../../../foo
Redirect from '/../../../foo' to 'other': /../../../other
Redirect from '/../../../foo' to '': /../../../
Redirect from '/../../../foo' to '../../../foo': /../../../../../../foo
Redirect from '/../../../foo' to 'space bar': failed
Redirect from '/space bar' to '/dir/page': failed
Redirect from '/space bar' to 'dir/page': failed
Expand All @@ -143,12 +143,12 @@ Redirect from '/space bar' to '../../../foo': failed
Redirect from '/space bar' to 'space bar': failed
# URI parser: Uri\WhatWg\Url
Redirect from '/dir/page' to '/dir/page': /dir/page
Redirect from '/dir/page' to 'dir/page': /dir//dir/page
Redirect from '/dir/page' to 'dir/page': /dir/dir/page
Redirect from '/dir/page' to 'a': /a
Redirect from '/dir/page' to 'other': /dir//other
Redirect from '/dir/page' to '': /
Redirect from '/dir/page' to 'other': /dir/other
Redirect from '/dir/page' to '': /dir/
Redirect from '/dir/page' to '../../../foo': /foo
Redirect from '/dir/page' to 'space bar': /dir//space%20bar
Redirect from '/dir/page' to 'space bar': /dir/space%20bar
Redirect from '/a' to '/dir/page': /dir/page
Redirect from '/a' to 'dir/page': /dir/page
Redirect from '/a' to 'a': /a
Expand Down
Loading