From 175e22394a4cac9ee1d2e86809ccc0bef7e9eb65 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Fri, 16 Apr 2021 16:59:10 +0200 Subject: [PATCH 1/3] Fix #80931: HTTP stream hangs if server doesn't close connection As is, we read HTTP streams until the server closes the connection, what is standard behavior prior to HTTP/1.1 anyway. However, HTTP/1.1 defaults to not closing the connection, and although we are sending a `Connection: close` header, there is no guarantee that the server heeds it. Even worse, if a user passes a `Connection: keep-alive` header via the stream context, the server is not supposed to close the connection, so the request hangs (and will only terminated by a timeout). The proper way to read HTTP responses is to the heed the sent Content- Length, or in case of `Transfer-encoding: chunked`, to read until after the final chunk (zero bytes). To avoid baking this into the general streams behavior, we use stream filters for this purpose (as suggested by @IMSoP), and if the end of the stream is detected by those, we set the `eof` flag. Since there are already the quite suitable filters for our purpose (`dechunk` and `consumed`) we attach the desired behavior to these. We avoid the filter overhead in cases where the server is going to close the connection anyway, i.e. prior to HTTP/1.1, or if the server responds with `Connection: close`. Currently, the end of content check is enabled for the existing filters by passing filter parameters. I kept that simple for now, by just passing a bool or a long, but that might be to big a BC break; neither the `dechunk` nor the `consumed` filter are documented, but easily recognizable via `stream_get_filters()`, so existing code may already pass parameters, although these are so far unused. That could be mitigated by passing an array with respective members instead. Another option to avoid the BC altogether would be to add the required info to the stream name (similar to the `convert.iconv.*` filter). For best BC, it might also be desireable to only set the `eof` flag from the `dechunk` filter, if `dechunk` is explicitly requested. --- ext/standard/filters.c | 30 +++++++++++++++++++++----- ext/standard/http_fopen_wrapper.c | 35 ++++++++++++++++++++++++------- 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/ext/standard/filters.c b/ext/standard/filters.c index 018270c730d37..63ed8b46e87cf 100644 --- a/ext/standard/filters.c +++ b/ext/standard/filters.c @@ -1760,6 +1760,7 @@ typedef struct _php_consumed_filter_data { size_t consumed; zend_off_t offset; uint8_t persistent; + size_t length; } php_consumed_filter_data; static php_stream_filter_status_t consumed_filter_filter( @@ -1790,6 +1791,9 @@ static php_stream_filter_status_t consumed_filter_filter( php_stream_seek(stream, data->offset + data->consumed, SEEK_SET); } data->consumed += consumed; + if (data->consumed >= data->length) { + stream->eof = 1; + } return PSFS_PASS_ON; } @@ -1822,6 +1826,7 @@ static php_stream_filter *consumed_filter_create(const char *filtername, zval *f data->persistent = persistent; data->consumed = 0; data->offset = ~0; + data->length = filterparams == NULL ? (size_t) -1 : Z_LVAL_P(filterparams); fops = &consumed_filter_ops; return php_stream_filter_alloc(fops, data, persistent); @@ -1851,9 +1856,10 @@ typedef struct _php_chunked_filter_data { size_t chunk_size; php_chunked_filter_state state; int persistent; + zend_bool dechunk; } php_chunked_filter_data; -static size_t php_dechunk(char *buf, size_t len, php_chunked_filter_data *data) +static size_t php_dechunk_ex(char *buf, size_t len, php_chunked_filter_data *data, zend_bool dechunk) { char *p = buf; char *end = p + len; @@ -1920,7 +1926,7 @@ static size_t php_dechunk(char *buf, size_t len, php_chunked_filter_data *data) } case CHUNK_BODY: if ((size_t) (end - p) >= data->chunk_size) { - if (p != out) { + if (dechunk && p != out) { memmove(out, p, data->chunk_size); } out += data->chunk_size; @@ -1931,7 +1937,7 @@ static size_t php_dechunk(char *buf, size_t len, php_chunked_filter_data *data) return out_len; } } else { - if (p != out) { + if (dechunk && p != out) { memmove(out, p, end - p); } data->chunk_size -= end - p; @@ -1961,7 +1967,7 @@ static size_t php_dechunk(char *buf, size_t len, php_chunked_filter_data *data) p = end; continue; case CHUNK_ERROR: - if (p != out) { + if (dechunk && p != out) { memmove(out, p, end - p); } out_len += end - p; @@ -1971,6 +1977,11 @@ static size_t php_dechunk(char *buf, size_t len, php_chunked_filter_data *data) return out_len; } +static size_t php_dechunk(char *buf, size_t len, php_chunked_filter_data *data) +{ + return php_dechunk_ex(buf, len, data, 1); +} + static php_stream_filter_status_t php_chunked_filter( php_stream *stream, php_stream_filter *thisfilter, @@ -1987,7 +1998,11 @@ static php_stream_filter_status_t php_chunked_filter( while (buckets_in->head) { bucket = php_stream_bucket_make_writeable(buckets_in->head); consumed += bucket->buflen; - bucket->buflen = php_dechunk(bucket->buf, bucket->buflen, data); + if (data->dechunk) { + bucket->buflen = php_dechunk(bucket->buf, bucket->buflen, data); + } else { + (void) php_dechunk_ex(bucket->buf, bucket->buflen, data, 0); + } php_stream_bucket_append(buckets_out, bucket); } @@ -1995,6 +2010,10 @@ static php_stream_filter_status_t php_chunked_filter( *bytes_consumed = consumed; } + if (data->state == CHUNK_TRAILER) { + stream->eof = 1; + } + return PSFS_PASS_ON; } @@ -2026,6 +2045,7 @@ static php_stream_filter *chunked_filter_create(const char *filtername, zval *fi data->state = CHUNK_SIZE_START; data->chunk_size = 0; data->persistent = persistent; + data->dechunk = filterparams == NULL || zend_is_true(filterparams); fops = &chunked_filter_ops; return php_stream_filter_alloc(fops, data, persistent); diff --git a/ext/standard/http_fopen_wrapper.c b/ext/standard/http_fopen_wrapper.c index 4d918b21e659c..3ad61053fe6ad 100644 --- a/ext/standard/http_fopen_wrapper.c +++ b/ext/standard/http_fopen_wrapper.c @@ -144,10 +144,12 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, int header_init = ((flags & HTTP_WRAPPER_HEADER_INIT) != 0); int redirected = ((flags & HTTP_WRAPPER_REDIRECTED) != 0); zend_bool follow_location = 1; - php_stream_filter *transfer_encoding = NULL; + php_stream_filter *body_filter = NULL; int response_code; smart_str req_buf = {0}; zend_bool custom_request_method; + zend_bool server_closes_connection = 0; + zend_bool chunked_encoding = 0; tmp_line[0] = '\0'; @@ -664,6 +666,9 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, php_stream_get_line(stream, tmp_line, sizeof(tmp_line) - 1, &tmp_line_len) != NULL) { zval http_response; + if (strncasecmp(tmp_line, "HTTP/1.1", sizeof("HTTP/1.1")-1) < 0) { + server_closes_connection = 1; + } if (tmp_line_len > 9) { response_code = atoi(tmp_line + 9); } else { @@ -793,6 +798,10 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, } else if (!strncasecmp(http_header_line, "Content-Length:", sizeof("Content-Length:")-1)) { file_size = atoi(http_header_value); php_stream_notify_file_size(context, file_size, http_header_line, 0); + } else if (!strncasecmp(http_header_line, "Connection:", sizeof("Connection:")-1) + && !strncasecmp(http_header_value, "Close", sizeof("Close")-1) + ) { + server_closes_connection = 1; } else if ( !strncasecmp(http_header_line, "Transfer-Encoding:", sizeof("Transfer-Encoding:")-1) && !strncasecmp(http_header_value, "Chunked", sizeof("Chunked")-1) @@ -806,11 +815,13 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, decode = zend_is_true(tmpzval); } if (decode) { - transfer_encoding = php_stream_filter_create("dechunk", NULL, php_stream_is_persistent(stream)); - if (transfer_encoding) { + body_filter = php_stream_filter_create("dechunk", NULL, php_stream_is_persistent(stream)); + if (body_filter) { /* don't store transfer-encodeing header */ continue; } + } else { + chunked_encoding = 1; } } } @@ -949,12 +960,22 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, /* restore mode */ strlcpy(stream->mode, mode, sizeof(stream->mode)); - if (transfer_encoding) { - php_stream_filter_append(&stream->readfilters, transfer_encoding); + if (body_filter == NULL && !server_closes_connection) { + zval param; + if (chunked_encoding) { + ZVAL_FALSE(¶m); + body_filter = php_stream_filter_create("dechunk", ¶m, php_stream_is_persistent(stream)); + } else { + ZVAL_LONG(¶m, file_size); + body_filter = php_stream_filter_create("consumed", ¶m, php_stream_is_persistent(stream)); + } + } + if (body_filter) { + php_stream_filter_append(&stream->readfilters, body_filter); } } else { - if (transfer_encoding) { - php_stream_filter_free(transfer_encoding); + if (body_filter) { + php_stream_filter_free(body_filter); } } From 72869f92b0b7a532cf932048ac5609b509672db6 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Fri, 16 Apr 2021 19:01:53 +0200 Subject: [PATCH 2/3] Add regression tests This is tricky, since the test server closes the socket right after sending the response. For now, we simulate `Connection: keep-alive` by sleeping for two seconds, and check on the client side whether the request took less than two seconds. --- ext/standard/tests/http/bug80931.phpt | 56 +++++++++++++++++++++++++++ ext/standard/tests/http/server.inc | 7 ++++ 2 files changed, 63 insertions(+) create mode 100644 ext/standard/tests/http/bug80931.phpt diff --git a/ext/standard/tests/http/bug80931.phpt b/ext/standard/tests/http/bug80931.phpt new file mode 100644 index 0000000000000..a9016f5b19d7e --- /dev/null +++ b/ext/standard/tests/http/bug80931.phpt @@ -0,0 +1,56 @@ +--TEST-- +Bug #80931 (HTTP stream hangs if server doesn't close connection) +--INI-- +allow_url_fopen=1 +--SKIPIF-- + +--FILE-- + [ + 'protocol_version' => "1.1", + 'header' => "Connection: keep-alive", + ] +]; + +$stream = fopen("http://127.0.0.1:12342/", "r", false, stream_context_create($options)); +$t0 = microtime(true); +var_dump(stream_get_contents($stream)); +$t1 = microtime(true); +var_dump($t1 - $t0 < 2); + +$stream = fopen("http://127.0.0.1:12342/", "r", false, stream_context_create($options)); +$t0 = microtime(true); +var_dump(stream_get_contents($stream)); +$t1 = microtime(true); +var_dump($t1 - $t0 < 2); + +$options['http']['auto_decode'] = false; +$stream = fopen("http://127.0.0.1:12342/", "r", false, stream_context_create($options)); +$t0 = microtime(true); +var_dump(stream_get_contents($stream)); +$t1 = microtime(true); +var_dump($t1 - $t0 < 2); + +http_server_kill($pid); +?> +--EXPECT-- +string(10) "0123456789" +bool(true) +string(4) "****" +bool(true) +string(14) "4 +**** +0 + +" +bool(true) diff --git a/ext/standard/tests/http/server.inc b/ext/standard/tests/http/server.inc index e58067492891e..786ca712408b6 100644 --- a/ext/standard/tests/http/server.inc +++ b/ext/standard/tests/http/server.inc @@ -54,6 +54,7 @@ function http_server($socket_string, array $files, &$output = null) { // read headers $content_length = 0; + $keep_alive = false; stream_set_blocking($sock, 0); while (!feof($sock)) { @@ -70,6 +71,8 @@ function http_server($socket_string, array $files, &$output = null) { if (preg_match('#^Content-Length\s*:\s*([[:digit:]]+)\s*$#i', $line, $matches)) { $content_length = (int) $matches[1]; + } elseif (preg_match('#^Connection\s*:\s*keep-alive\s*$#i', $line, $matches)) { + $keep_alive = true; } } } @@ -86,6 +89,10 @@ function http_server($socket_string, array $files, &$output = null) { $fd = fopen($file, 'rb'); stream_copy_to_stream($fd, $sock); + if ($keep_alive) { + sleep(2); + } + fclose($sock); } From cd3e968e4be3f537e2f0146041c7e7349e39524c Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 19 Apr 2021 14:21:35 +0200 Subject: [PATCH 3/3] Cater to missing Content-Length header In several cases besides `Transfer-Encoding` headers, a server is not supposed to send a `Content-Length` header; in other cases, the server is not required to send a `Content-Length` header. We need to cater to that and must not apply the `consumed` filter. --- ext/standard/http_fopen_wrapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/standard/http_fopen_wrapper.c b/ext/standard/http_fopen_wrapper.c index 3ad61053fe6ad..f0aea5a425b96 100644 --- a/ext/standard/http_fopen_wrapper.c +++ b/ext/standard/http_fopen_wrapper.c @@ -965,7 +965,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, if (chunked_encoding) { ZVAL_FALSE(¶m); body_filter = php_stream_filter_create("dechunk", ¶m, php_stream_is_persistent(stream)); - } else { + } else if (file_size > 0) { ZVAL_LONG(¶m, file_size); body_filter = php_stream_filter_create("consumed", ¶m, php_stream_is_persistent(stream)); }