From 5692fa29148817392c645b5e971e98aca19b867b Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 9 May 2022 23:00:33 +0100 Subject: [PATCH 01/25] Convert php_cli_server_client->addr_str to zend_string --- sapi/cli/php_cli_server.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 65a8a65fe2f3b..2e11225d4f91d 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -164,8 +164,7 @@ typedef struct php_cli_server_client { php_socket_t sock; struct sockaddr *addr; socklen_t addr_len; - char *addr_str; - size_t addr_str_len; + zend_string *addr_str; php_http_parser parser; unsigned int request_read:1; char *current_header_name; @@ -640,9 +639,9 @@ static void sapi_cli_server_register_variables(zval *track_vars_array) /* {{{ */ sapi_cli_server_register_variable(track_vars_array, "DOCUMENT_ROOT", client->server->document_root); { char *tmp; - if ((tmp = strrchr(client->addr_str, ':'))) { + if ((tmp = strrchr(ZSTR_VAL(client->addr_str), ':'))) { char addr[64], port[8]; - const char *addr_start = client->addr_str, *addr_end = tmp; + const char *addr_start = ZSTR_VAL(client->addr_str), *addr_end = tmp; if (addr_start[0] == '[') addr_start++; if (addr_end[-1] == ']') addr_end--; @@ -653,7 +652,7 @@ static void sapi_cli_server_register_variables(zval *track_vars_array) /* {{{ */ sapi_cli_server_register_variable(track_vars_array, "REMOTE_ADDR", addr); sapi_cli_server_register_variable(track_vars_array, "REMOTE_PORT", port); } else { - sapi_cli_server_register_variable(track_vars_array, "REMOTE_ADDR", client->addr_str); + sapi_cli_server_register_variable(track_vars_array, "REMOTE_ADDR", ZSTR_VAL(client->addr_str)); } } { @@ -1152,7 +1151,8 @@ static void php_cli_server_log_response(php_cli_server_client *client, int statu #endif /* basic */ - spprintf(&basic_buf, 0, "%s [%d]: %s %s", client->addr_str, status, php_http_method_str(client->request.request_method), client->request.request_uri); + spprintf(&basic_buf, 0, "%s [%d]: %s %s", ZSTR_VAL(client->addr_str), status, + php_http_method_str(client->request.request_method), client->request.request_uri); if (!basic_buf) { return; } @@ -1891,14 +1891,13 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se client->sock = client_sock; client->addr = addr; client->addr_len = addr_len; - { - zend_string *addr_str = 0; - php_network_populate_name_from_sockaddr(addr, addr_len, &addr_str, NULL, 0); - client->addr_str = pestrndup(ZSTR_VAL(addr_str), ZSTR_LEN(addr_str), 1); - client->addr_str_len = ZSTR_LEN(addr_str); - zend_string_release_ex(addr_str, 0); - } + // TODO Prevent realloc? + zend_string *tmp_addr = NULL; + php_network_populate_name_from_sockaddr(addr, addr_len, &tmp_addr, NULL, 0); + client->addr_str = zend_string_dup(tmp_addr, /* persistent */ true); + zend_string_release_ex(tmp_addr, /* persistent */ false); + php_http_parser_init(&client->parser, PHP_HTTP_REQUEST); client->request_read = 0; @@ -1925,7 +1924,7 @@ static void php_cli_server_client_dtor(php_cli_server_client *client) /* {{{ */ client->file_fd = -1; } pefree(client->addr, 1); - pefree(client->addr_str, 1); + zend_string_release_ex(client->addr_str, /* persistent */ true); if (client->content_sender_initialized) { php_cli_server_content_sender_dtor(&client->content_sender); } @@ -1933,7 +1932,7 @@ static void php_cli_server_client_dtor(php_cli_server_client *client) /* {{{ */ static void php_cli_server_close_connection(php_cli_server *server, php_cli_server_client *client) /* {{{ */ { - php_cli_server_logf(PHP_CLI_SERVER_LOG_MESSAGE, "%s Closing", client->addr_str); + php_cli_server_logf(PHP_CLI_SERVER_LOG_MESSAGE, "%s Closing", ZSTR_VAL(client->addr_str)); zend_hash_index_del(&server->clients, client->sock); } /* }}} */ @@ -2515,9 +2514,9 @@ static zend_result php_cli_server_recv_event_read_request(php_cli_server *server if (errstr) { if (strcmp(errstr, php_cli_server_request_error_unexpected_eof) == 0 && client->parser.state == s_start_req) { php_cli_server_logf(PHP_CLI_SERVER_LOG_MESSAGE, - "%s Closed without sending a request; it was probably just an unused speculative preconnection", client->addr_str); + "%s Closed without sending a request; it was probably just an unused speculative preconnection", ZSTR_VAL(client->addr_str)); } else { - php_cli_server_logf(PHP_CLI_SERVER_LOG_ERROR, "%s Invalid request (%s)", client->addr_str, errstr); + php_cli_server_logf(PHP_CLI_SERVER_LOG_ERROR, "%s Invalid request (%s)", ZSTR_VAL(client->addr_str), errstr); } efree(errstr); } @@ -2605,7 +2604,7 @@ static zend_result php_cli_server_do_event_for_each_fd_callback(void *_params, p php_cli_server_client_ctor(client, server, client_sock, sa, socklen); - php_cli_server_logf(PHP_CLI_SERVER_LOG_MESSAGE, "%s Accepted", client->addr_str); + php_cli_server_logf(PHP_CLI_SERVER_LOG_MESSAGE, "%s Accepted", ZSTR_VAL(client->addr_str)); zend_hash_index_update_ptr(&server->clients, client_sock, client); From e770e27b5ec0f54fa2f818f5ffb7214cacfecc25 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 9 May 2022 23:32:05 +0100 Subject: [PATCH 02/25] Convert php_cli_server_client->current_header_name to zend_string --- sapi/cli/php_cli_server.c | 65 +++++++++++---------------------------- 1 file changed, 18 insertions(+), 47 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 2e11225d4f91d..b1cf0ead55834 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -167,9 +167,7 @@ typedef struct php_cli_server_client { zend_string *addr_str; php_http_parser parser; unsigned int request_read:1; - char *current_header_name; - size_t current_header_name_len; - unsigned int current_header_name_allocated:1; + zend_string *current_header_name; char *current_header_value; size_t current_header_value_len; enum { HEADER_NONE=0, HEADER_FIELD, HEADER_VALUE } last_header_element; @@ -1623,22 +1621,13 @@ static int php_cli_server_client_read_request_on_fragment(php_http_parser *parse static void php_cli_server_client_save_header(php_cli_server_client *client) { /* strip off the colon */ - zend_string *orig_header_name = zend_string_init(client->current_header_name, client->current_header_name_len, 1); - zend_string *lc_header_name = zend_string_alloc(client->current_header_name_len, 1); - zend_str_tolower_copy(ZSTR_VAL(lc_header_name), client->current_header_name, client->current_header_name_len); - GC_MAKE_PERSISTENT_LOCAL(orig_header_name); - GC_MAKE_PERSISTENT_LOCAL(lc_header_name); + // TODO Need to duplicate original header and make persistent? + zend_string *lc_header_name = zend_string_tolower_ex(client->current_header_name, /* persistent */ true); zend_hash_add_ptr(&client->request.headers, lc_header_name, client->current_header_value); - zend_hash_add_ptr(&client->request.headers_original_case, orig_header_name, client->current_header_value); - zend_string_release_ex(lc_header_name, 1); - zend_string_release_ex(orig_header_name, 1); + zend_hash_add_ptr(&client->request.headers_original_case, client->current_header_name, client->current_header_value); + zend_string_release_ex(lc_header_name, /* persistent */ true); - if (client->current_header_name_allocated) { - pefree(client->current_header_name, 1); - client->current_header_name_allocated = 0; - } client->current_header_name = NULL; - client->current_header_name_len = 0; client->current_header_value = NULL; client->current_header_value_len = 0; } @@ -1647,31 +1636,19 @@ static int php_cli_server_client_read_request_on_header_field(php_http_parser *p { php_cli_server_client *client = parser->data; switch (client->last_header_element) { - case HEADER_VALUE: - php_cli_server_client_save_header(client); - ZEND_FALLTHROUGH; - case HEADER_NONE: - client->current_header_name = (char *)at; - client->current_header_name_len = length; - break; - case HEADER_FIELD: - if (client->current_header_name_allocated) { - size_t new_length = client->current_header_name_len + length; - client->current_header_name = perealloc(client->current_header_name, new_length + 1, 1); - memcpy(client->current_header_name + client->current_header_name_len, at, length); - client->current_header_name[new_length] = '\0'; - client->current_header_name_len = new_length; - } else { - size_t new_length = client->current_header_name_len + length; - char* field = pemalloc(new_length + 1, 1); - memcpy(field, client->current_header_name, client->current_header_name_len); - memcpy(field + client->current_header_name_len, at, length); - field[new_length] = '\0'; + case HEADER_VALUE: + php_cli_server_client_save_header(client); + ZEND_FALLTHROUGH; + case HEADER_NONE: + client->current_header_name = zend_string_init(at, length, /* persistent */ false); + break; + case HEADER_FIELD: { + zend_string *field = zend_string_concat2( + ZSTR_VAL(client->current_header_name), ZSTR_LEN(client->current_header_name), at, length); + // Free previous + zend_string_release_ex(client->current_header_name, /* persistent */ false); client->current_header_name = field; - client->current_header_name_len = new_length; - client->current_header_name_allocated = 1; } - break; } client->last_header_element = HEADER_FIELD; @@ -1816,12 +1793,7 @@ static int php_cli_server_client_read_request(php_cli_server_client *client, cha return -1; } - if (client->current_header_name) { - char *header_name = safe_pemalloc(client->current_header_name_len, 1, 1, 1); - memmove(header_name, client->current_header_name, client->current_header_name_len); - client->current_header_name = header_name; - client->current_header_name_allocated = 1; - } + return client->request_read ? 1: 0; } /* }}} */ @@ -1903,8 +1875,6 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se client->last_header_element = HEADER_NONE; client->current_header_name = NULL; - client->current_header_name_len = 0; - client->current_header_name_allocated = 0; client->current_header_value = NULL; client->current_header_value_len = 0; @@ -1925,6 +1895,7 @@ static void php_cli_server_client_dtor(php_cli_server_client *client) /* {{{ */ } pefree(client->addr, 1); zend_string_release_ex(client->addr_str, /* persistent */ true); + // TODO release client->current_header_name ? if (client->content_sender_initialized) { php_cli_server_content_sender_dtor(&client->content_sender); } From bb2b62415ac61c5f856c8f6da3be86a02f0fc687 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Tue, 10 May 2022 00:53:23 +0100 Subject: [PATCH 03/25] Convert php_cli_server_client->current_header_value to zend_string --- sapi/cli/php_cli_server.c | 77 +++++++++++++------------- sapi/cli/tests/php_cli_server_022.phpt | 45 +++++++++++++++ 2 files changed, 85 insertions(+), 37 deletions(-) create mode 100644 sapi/cli/tests/php_cli_server_022.phpt diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index b1cf0ead55834..b73b367a31012 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -168,8 +168,7 @@ typedef struct php_cli_server_client { php_http_parser parser; unsigned int request_read:1; zend_string *current_header_name; - char *current_header_value; - size_t current_header_value_len; + zend_string *current_header_value; enum { HEADER_NONE=0, HEADER_FIELD, HEADER_VALUE } last_header_element; size_t post_read_offset; php_cli_server_request request; @@ -347,19 +346,19 @@ static void append_http_status_line(smart_str *buffer, int protocol_version, int static void append_essential_headers(smart_str* buffer, php_cli_server_client *client, bool persistent) /* {{{ */ { - char *val; + zend_string *val; struct timeval tv = {0}; if (NULL != (val = zend_hash_str_find_ptr(&client->request.headers, "host", sizeof("host")-1))) { smart_str_appends_ex(buffer, "Host: ", persistent); - smart_str_appends_ex(buffer, val, persistent); + smart_str_append_ex(buffer, val, persistent); smart_str_appends_ex(buffer, "\r\n", persistent); } if (!gettimeofday(&tv, NULL)) { zend_string *dt = php_format_date("D, d M Y H:i:s", sizeof("D, d M Y H:i:s") - 1, tv.tv_sec, 0); smart_str_appends_ex(buffer, "Date: ", persistent); - smart_str_appends_ex(buffer, dt->val, persistent); + smart_str_append_ex(buffer, dt, persistent); smart_str_appends_ex(buffer, " GMT\r\n", persistent); zend_string_release_ex(dt, 0); } @@ -383,7 +382,7 @@ PHP_FUNCTION(apache_request_headers) /* {{{ */ php_cli_server_client *client; HashTable *headers; zend_string *key; - char *value; + zend_string *value; zval tmp; if (zend_parse_parameters_none() == FAILURE) { @@ -396,7 +395,8 @@ PHP_FUNCTION(apache_request_headers) /* {{{ */ array_init_size(return_value, zend_hash_num_elements(headers)); ZEND_HASH_MAP_FOREACH_STR_KEY_PTR(headers, key, value) { - ZVAL_STRING(&tmp, value); + // TODO There must be a better way + ZVAL_STRING(&tmp, ZSTR_VAL(value)); zend_symtable_update(Z_ARRVAL_P(return_value), key, &tmp); } ZEND_HASH_FOREACH_END(); } @@ -571,11 +571,11 @@ static int sapi_cli_server_send_headers(sapi_headers_struct *sapi_headers) /* {{ static char *sapi_cli_server_read_cookies(void) /* {{{ */ { php_cli_server_client *client = SG(server_context); - char *val; + zend_string *val; if (NULL == (val = zend_hash_str_find_ptr(&client->request.headers, "cookie", sizeof("cookie")-1))) { return NULL; } - return val; + return ZSTR_VAL(val); } /* }}} */ static size_t sapi_cli_server_read_post(char *buf, size_t count_bytes) /* {{{ */ @@ -605,7 +605,7 @@ static void sapi_cli_server_register_variable(zval *track_vars_array, const char } } /* }}} */ -static int sapi_cli_server_register_entry_cb(char **entry, int num_args, va_list args, zend_hash_key *hash_key) /* {{{ */ { +static int sapi_cli_server_register_entry_cb(zend_string **entry, int num_args, va_list args, zend_hash_key *hash_key) /* {{{ */ { zval *track_vars_array = va_arg(args, zval *); if (hash_key->key) { char *real_key, *key; @@ -620,9 +620,10 @@ static int sapi_cli_server_register_entry_cb(char **entry, int num_args, va_list } spprintf(&real_key, 0, "%s_%s", "HTTP", key); if (strcmp(key, "CONTENT_TYPE") == 0 || strcmp(key, "CONTENT_LENGTH") == 0) { - sapi_cli_server_register_variable(track_vars_array, key, *entry); + // TODO make a version specialized for zend_string? + sapi_cli_server_register_variable(track_vars_array, key, ZSTR_VAL(*entry)); } - sapi_cli_server_register_variable(track_vars_array, real_key, *entry); + sapi_cli_server_register_variable(track_vars_array, real_key, ZSTR_VAL(*entry)); efree(key); efree(real_key); } @@ -1629,7 +1630,6 @@ static void php_cli_server_client_save_header(php_cli_server_client *client) client->current_header_name = NULL; client->current_header_value = NULL; - client->current_header_value_len = 0; } static int php_cli_server_client_read_request_on_header_field(php_http_parser *parser, const char *at, size_t length) @@ -1637,12 +1637,15 @@ static int php_cli_server_client_read_request_on_header_field(php_http_parser *p php_cli_server_client *client = parser->data; switch (client->last_header_element) { case HEADER_VALUE: + /* Save previous header before creating new one */ php_cli_server_client_save_header(client); ZEND_FALLTHROUGH; case HEADER_NONE: + /* Create new header field */ client->current_header_name = zend_string_init(at, length, /* persistent */ false); break; case HEADER_FIELD: { + /* Append header field */ zend_string *field = zend_string_concat2( ZSTR_VAL(client->current_header_name), ZSTR_LEN(client->current_header_name), at, length); // Free previous @@ -1659,23 +1662,24 @@ static int php_cli_server_client_read_request_on_header_value(php_http_parser *p { php_cli_server_client *client = parser->data; switch (client->last_header_element) { - case HEADER_FIELD: - client->current_header_value = pestrndup(at, length, 1); - client->current_header_value_len = length; - break; - case HEADER_VALUE: - { - size_t new_length = client->current_header_value_len + length; - client->current_header_value = perealloc(client->current_header_value, new_length + 1, 1); - memcpy(client->current_header_value + client->current_header_value_len, at, length); - client->current_header_value[new_length] = '\0'; - client->current_header_value_len = new_length; + case HEADER_FIELD: + /* Previous element was the header field, create the header value */ + client->current_header_value = zend_string_init(at, length, /* persistent */ true); + break; + case HEADER_VALUE: { + /* Previous element was part of header value, append content to it */ + size_t old_length = ZSTR_LEN(client->current_header_value); + // TODO Release old value? + client->current_header_value = zend_string_extend(client->current_header_value, old_length + length, /* persistent */ true); + memcpy(ZSTR_VAL(client->current_header_value) + old_length, at, length); + // Null terminate + ZSTR_VAL(client->current_header_value)[ZSTR_LEN(client->current_header_value)] = '\0'; + break; } - break; - case HEADER_NONE: - // can't happen - assert(0); - break; + case HEADER_NONE: + /* Cannot happen as a header field must have been found before */ + assert(0); + break; } client->last_header_element = HEADER_VALUE; return 0; @@ -1688,11 +1692,11 @@ static int php_cli_server_client_read_request_on_headers_complete(php_http_parse case HEADER_NONE: break; case HEADER_FIELD: - client->current_header_value = pemalloc(1, 1); - *client->current_header_value = '\0'; - client->current_header_value_len = 0; + /* Previous element was the header field, set it's value to an empty string */ + client->current_header_value = ZSTR_EMPTY_ALLOC(); ZEND_FALLTHROUGH; case HEADER_VALUE: + /* Save last header value */ php_cli_server_client_save_header(client); break; } @@ -1838,7 +1842,7 @@ static size_t php_cli_server_client_send_through(php_cli_server_client *client, static void php_cli_server_client_populate_request_info(const php_cli_server_client *client, sapi_request_info *request_info) /* {{{ */ { - char *val; + zend_string *val; request_info->request_method = php_http_method_str(client->request.request_method); request_info->proto_num = client->request.protocol_version; @@ -1848,7 +1852,7 @@ static void php_cli_server_client_populate_request_info(const php_cli_server_cli request_info->content_length = client->request.content_len; request_info->auth_user = request_info->auth_password = request_info->auth_digest = NULL; if (NULL != (val = zend_hash_str_find_ptr(&client->request.headers, "content-type", sizeof("content-type")-1))) { - request_info->content_type = val; + request_info->content_type = ZSTR_VAL(val); } } /* }}} */ @@ -1876,7 +1880,6 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se client->last_header_element = HEADER_NONE; client->current_header_name = NULL; client->current_header_value = NULL; - client->current_header_value_len = 0; client->post_read_offset = 0; @@ -2095,10 +2098,10 @@ static zend_result php_cli_server_begin_send_static(php_cli_server *server, php_ /* }}} */ static zend_result php_cli_server_request_startup(php_cli_server *server, php_cli_server_client *client) { /* {{{ */ - char *auth; + zend_string *auth; php_cli_server_client_populate_request_info(client, &SG(request_info)); if (NULL != (auth = zend_hash_str_find_ptr(&client->request.headers, "authorization", sizeof("authorization")-1))) { - php_handle_auth_data(auth); + php_handle_auth_data(ZSTR_VAL(auth)); } SG(sapi_headers).http_response_code = 200; if (FAILURE == php_request_startup()) { diff --git a/sapi/cli/tests/php_cli_server_022.phpt b/sapi/cli/tests/php_cli_server_022.phpt new file mode 100644 index 0000000000000..763a2036fcdcf --- /dev/null +++ b/sapi/cli/tests/php_cli_server_022.phpt @@ -0,0 +1,45 @@ +--TEST-- +Check PHP file body is executed +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +HTTP/1.1 200 OK +Host: %s +Date: %s +Connection: close +X-Powered-By: %s +Bar-Foo: Foo +Content-type: text/html; charset=UTF-8 + +Hello world +bool(true) From 14c592e36e330dc10f7238272a061df5959da245 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Tue, 10 May 2022 01:13:52 +0100 Subject: [PATCH 04/25] Add some assertions --- sapi/cli/php_cli_server.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index b73b367a31012..d1c3a53a5c319 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1898,7 +1898,11 @@ static void php_cli_server_client_dtor(php_cli_server_client *client) /* {{{ */ } pefree(client->addr, 1); zend_string_release_ex(client->addr_str, /* persistent */ true); - // TODO release client->current_header_name ? + + /* Headers must be sent */ + assert(client->current_header_name == NULL); + assert(client->current_header_value == NULL); + if (client->content_sender_initialized) { php_cli_server_content_sender_dtor(&client->content_sender); } From ebf6581fe499a3b92c19ca73a1be60dadcaf23df Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Tue, 10 May 2022 01:14:19 +0100 Subject: [PATCH 05/25] Duplicate original name header --- sapi/cli/php_cli_server.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index d1c3a53a5c319..823ef944ba8f3 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1622,10 +1622,12 @@ static int php_cli_server_client_read_request_on_fragment(php_http_parser *parse static void php_cli_server_client_save_header(php_cli_server_client *client) { /* strip off the colon */ - // TODO Need to duplicate original header and make persistent? + zend_string *perm_header_name = zend_string_dup(client->current_header_name, /* persistent */ true); zend_string *lc_header_name = zend_string_tolower_ex(client->current_header_name, /* persistent */ true); zend_hash_add_ptr(&client->request.headers, lc_header_name, client->current_header_value); - zend_hash_add_ptr(&client->request.headers_original_case, client->current_header_name, client->current_header_value); + zend_hash_add_ptr(&client->request.headers_original_case, perm_header_name, client->current_header_value); + + zend_string_release_ex(client->current_header_name, /* persistent */ false); zend_string_release_ex(lc_header_name, /* persistent */ true); client->current_header_name = NULL; From ab212bedc48da22784923e7876d02573b55995c4 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Wed, 11 May 2022 22:28:29 +0100 Subject: [PATCH 06/25] Use bool instead of unsigned int with a size of 1 bit --- sapi/cli/php_cli_server.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 823ef944ba8f3..4feeebb52f0e6 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -166,13 +166,13 @@ typedef struct php_cli_server_client { socklen_t addr_len; zend_string *addr_str; php_http_parser parser; - unsigned int request_read:1; + bool request_read; zend_string *current_header_name; zend_string *current_header_value; enum { HEADER_NONE=0, HEADER_FIELD, HEADER_VALUE } last_header_element; size_t post_read_offset; php_cli_server_request request; - unsigned int content_sender_initialized:1; + bool content_sender_initialized; php_cli_server_content_sender content_sender; int file_fd; } php_cli_server_client; @@ -1738,7 +1738,7 @@ static int php_cli_server_client_read_request_on_message_complete(php_http_parse } } } - client->request_read = 1; + client->request_read = true; return 0; } @@ -1877,7 +1877,7 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se zend_string_release_ex(tmp_addr, /* persistent */ false); php_http_parser_init(&client->parser, PHP_HTTP_REQUEST); - client->request_read = 0; + client->request_read = false; client->last_header_element = HEADER_NONE; client->current_header_name = NULL; @@ -1887,7 +1887,7 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se php_cli_server_request_ctor(&client->request); - client->content_sender_initialized = 0; + client->content_sender_initialized = false; client->file_fd = -1; } /* }}} */ @@ -1926,7 +1926,7 @@ static zend_result php_cli_server_send_error_page(php_cli_server *server, php_cl assert(status_string && content_template); php_cli_server_content_sender_ctor(&client->content_sender); - client->content_sender_initialized = 1; + client->content_sender_initialized = true; escaped_request_uri = php_escape_html_entities_ex((const unsigned char *) client->request.request_uri, client->request.request_uri_len, 0, ENT_QUOTES, NULL, /* double_encode */ 0, /* quiet */ 0); @@ -2062,7 +2062,7 @@ static zend_result php_cli_server_begin_send_static(php_cli_server *server, php_ } php_cli_server_content_sender_ctor(&client->content_sender); - client->content_sender_initialized = 1; + client->content_sender_initialized = true; client->file_fd = fd; { From 438f0329d34fc4482b3826fe62e6a34e5cbb69c6 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Thu, 12 May 2022 23:19:27 +0100 Subject: [PATCH 07/25] Add break again (don't know why I removed that one) --- sapi/cli/php_cli_server.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 4feeebb52f0e6..688111c4f0c56 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1653,6 +1653,7 @@ static int php_cli_server_client_read_request_on_header_field(php_http_parser *p // Free previous zend_string_release_ex(client->current_header_name, /* persistent */ false); client->current_header_name = field; + break; } } From a014824bf28ff347524c3d8f4e8a649731f32f80 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Thu, 12 May 2022 23:24:10 +0100 Subject: [PATCH 08/25] Add an assertion about the reallocation --- sapi/cli/php_cli_server.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 688111c4f0c56..39a625ed08e69 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1670,9 +1670,11 @@ static int php_cli_server_client_read_request_on_header_value(php_http_parser *p client->current_header_value = zend_string_init(at, length, /* persistent */ true); break; case HEADER_VALUE: { + /* Assert that there is only one reference to the header value, as then zend_string_extends() + * will reallocate it such that we do not need to release the old value. */ + ZEND_ASSERT(GC_REFCOUNT(client->current_header_value) == 1); /* Previous element was part of header value, append content to it */ size_t old_length = ZSTR_LEN(client->current_header_value); - // TODO Release old value? client->current_header_value = zend_string_extend(client->current_header_value, old_length + length, /* persistent */ true); memcpy(ZSTR_VAL(client->current_header_value) + old_length, at, length); // Null terminate From e5dea7e1492e3b3d67db023150ff1fd0bd7873c7 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Thu, 12 May 2022 23:48:11 +0100 Subject: [PATCH 09/25] Actually free persistent header value Still don't understand why ASAN didn't pick this up, probably need to check with Valgrind --- sapi/cli/php_cli_server.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 39a625ed08e69..a896737975f05 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -266,9 +266,11 @@ static bool php_cli_server_get_system_time(char *buf) { } #endif +/* Destructor for php_cli_server_request->headers, this frees header value */ static void char_ptr_dtor_p(zval *zv) /* {{{ */ { - pefree(Z_PTR_P(zv), 1); + // TODO free have zend_string* not as a pointer + zend_string_release_ex(Z_PTR_P(zv), /* persistent */ true); } /* }}} */ static char *get_last_error(void) /* {{{ */ @@ -1339,6 +1341,7 @@ static void php_cli_server_request_ctor(php_cli_server_request *req) /* {{{ */ req->query_string = NULL; req->query_string_len = 0; zend_hash_init(&req->headers, 0, NULL, char_ptr_dtor_p, 1); + /* No destructor is registered as the value pointed by is the same as for &req->headers */ zend_hash_init(&req->headers_original_case, 0, NULL, NULL, 1); req->content = NULL; req->content_len = 0; From 782e0c994341c7dd3bd154bf9335a9855ad26941 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 13 May 2022 01:00:43 +0100 Subject: [PATCH 10/25] Make request->headers store zend_string as IS_STRING zval instead as a IS_POINTER zval --- sapi/cli/php_cli_server.c | 60 +++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index a896737975f05..e4bd3519e3362 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -269,8 +269,7 @@ static bool php_cli_server_get_system_time(char *buf) { /* Destructor for php_cli_server_request->headers, this frees header value */ static void char_ptr_dtor_p(zval *zv) /* {{{ */ { - // TODO free have zend_string* not as a pointer - zend_string_release_ex(Z_PTR_P(zv), /* persistent */ true); + zend_string_release_ex(Z_STR_P(zv), /* persistent */ true); } /* }}} */ static char *get_last_error(void) /* {{{ */ @@ -348,12 +347,12 @@ static void append_http_status_line(smart_str *buffer, int protocol_version, int static void append_essential_headers(smart_str* buffer, php_cli_server_client *client, bool persistent) /* {{{ */ { - zend_string *val; + zval *val; struct timeval tv = {0}; - if (NULL != (val = zend_hash_str_find_ptr(&client->request.headers, "host", sizeof("host")-1))) { + if (NULL != (val = zend_hash_str_find(&client->request.headers, "host", sizeof("host")-1))) { smart_str_appends_ex(buffer, "Host: ", persistent); - smart_str_append_ex(buffer, val, persistent); + smart_str_append_ex(buffer, Z_STR_P(val), persistent); smart_str_appends_ex(buffer, "\r\n", persistent); } @@ -382,9 +381,6 @@ static const char *get_mime_type(const php_cli_server *server, const char *ext, PHP_FUNCTION(apache_request_headers) /* {{{ */ { php_cli_server_client *client; - HashTable *headers; - zend_string *key; - zend_string *value; zval tmp; if (zend_parse_parameters_none() == FAILURE) { @@ -392,15 +388,10 @@ PHP_FUNCTION(apache_request_headers) /* {{{ */ } client = SG(server_context); - headers = &client->request.headers_original_case; - array_init_size(return_value, zend_hash_num_elements(headers)); - - ZEND_HASH_MAP_FOREACH_STR_KEY_PTR(headers, key, value) { - // TODO There must be a better way - ZVAL_STRING(&tmp, ZSTR_VAL(value)); - zend_symtable_update(Z_ARRVAL_P(return_value), key, &tmp); - } ZEND_HASH_FOREACH_END(); + /* Need to copy the HashTable */ + ZVAL_ARR(&tmp,&client->request.headers_original_case); + RETURN_COPY(&tmp); } /* }}} */ @@ -573,11 +564,11 @@ static int sapi_cli_server_send_headers(sapi_headers_struct *sapi_headers) /* {{ static char *sapi_cli_server_read_cookies(void) /* {{{ */ { php_cli_server_client *client = SG(server_context); - zend_string *val; - if (NULL == (val = zend_hash_str_find_ptr(&client->request.headers, "cookie", sizeof("cookie")-1))) { + zval *val; + if (NULL == (val = zend_hash_str_find(&client->request.headers, "cookie", sizeof("cookie")-1))) { return NULL; } - return ZSTR_VAL(val); + return Z_STRVAL_P(val); } /* }}} */ static size_t sapi_cli_server_read_post(char *buf, size_t count_bytes) /* {{{ */ @@ -607,8 +598,12 @@ static void sapi_cli_server_register_variable(zval *track_vars_array, const char } } /* }}} */ -static int sapi_cli_server_register_entry_cb(zend_string **entry, int num_args, va_list args, zend_hash_key *hash_key) /* {{{ */ { +/* The entry zval will always contain a zend_string* */ +static int sapi_cli_server_register_entry_cb(zval *entry, int num_args, va_list args, zend_hash_key *hash_key) /* {{{ */ { zval *track_vars_array = va_arg(args, zval *); + + ZEND_ASSERT(Z_TYPE_P(entry) == IS_STRING); + if (hash_key->key) { char *real_key, *key; uint32_t i; @@ -623,9 +618,9 @@ static int sapi_cli_server_register_entry_cb(zend_string **entry, int num_args, spprintf(&real_key, 0, "%s_%s", "HTTP", key); if (strcmp(key, "CONTENT_TYPE") == 0 || strcmp(key, "CONTENT_LENGTH") == 0) { // TODO make a version specialized for zend_string? - sapi_cli_server_register_variable(track_vars_array, key, ZSTR_VAL(*entry)); + sapi_cli_server_register_variable(track_vars_array, key, Z_STRVAL_P(entry)); } - sapi_cli_server_register_variable(track_vars_array, real_key, ZSTR_VAL(*entry)); + sapi_cli_server_register_variable(track_vars_array, real_key, Z_STRVAL_P(entry)); efree(key); efree(real_key); } @@ -1624,11 +1619,16 @@ static int php_cli_server_client_read_request_on_fragment(php_http_parser *parse static void php_cli_server_client_save_header(php_cli_server_client *client) { + /* Wrap header value in a zval to add is to the HashTable which acts as an array */ + zval tmp; + ZVAL_STR(&tmp, client->current_header_value); /* strip off the colon */ zend_string *perm_header_name = zend_string_dup(client->current_header_name, /* persistent */ true); zend_string *lc_header_name = zend_string_tolower_ex(client->current_header_name, /* persistent */ true); - zend_hash_add_ptr(&client->request.headers, lc_header_name, client->current_header_value); - zend_hash_add_ptr(&client->request.headers_original_case, perm_header_name, client->current_header_value); + + /* Add the wrapped zend_string to the HashTable */ + zend_hash_add(&client->request.headers, lc_header_name, &tmp); + zend_hash_add(&client->request.headers_original_case, perm_header_name, &tmp); zend_string_release_ex(client->current_header_name, /* persistent */ false); zend_string_release_ex(lc_header_name, /* persistent */ true); @@ -1850,7 +1850,7 @@ static size_t php_cli_server_client_send_through(php_cli_server_client *client, static void php_cli_server_client_populate_request_info(const php_cli_server_client *client, sapi_request_info *request_info) /* {{{ */ { - zend_string *val; + zval *val; request_info->request_method = php_http_method_str(client->request.request_method); request_info->proto_num = client->request.protocol_version; @@ -1859,8 +1859,8 @@ static void php_cli_server_client_populate_request_info(const php_cli_server_cli request_info->query_string = client->request.query_string; request_info->content_length = client->request.content_len; request_info->auth_user = request_info->auth_password = request_info->auth_digest = NULL; - if (NULL != (val = zend_hash_str_find_ptr(&client->request.headers, "content-type", sizeof("content-type")-1))) { - request_info->content_type = ZSTR_VAL(val); + if (NULL != (val = zend_hash_str_find(&client->request.headers, "content-type", sizeof("content-type")-1))) { + request_info->content_type = Z_STRVAL_P(val); } } /* }}} */ @@ -2110,10 +2110,10 @@ static zend_result php_cli_server_begin_send_static(php_cli_server *server, php_ /* }}} */ static zend_result php_cli_server_request_startup(php_cli_server *server, php_cli_server_client *client) { /* {{{ */ - zend_string *auth; + zval *auth; php_cli_server_client_populate_request_info(client, &SG(request_info)); - if (NULL != (auth = zend_hash_str_find_ptr(&client->request.headers, "authorization", sizeof("authorization")-1))) { - php_handle_auth_data(ZSTR_VAL(auth)); + if (NULL != (auth = zend_hash_str_find(&client->request.headers, "authorization", sizeof("authorization")-1))) { + php_handle_auth_data(Z_STRVAL_P(auth)); } SG(sapi_headers).http_response_code = 200; if (FAILURE == php_request_startup()) { From 74b237508faf91a7492810d20abda6d5f2842806 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 13 May 2022 01:03:17 +0100 Subject: [PATCH 11/25] Revert "Duplicate original name header" The key has it's refcount increased This reverts commit 4c90ecb05b636ebc425472fae0fd6f1d6e169e0c. --- sapi/cli/php_cli_server.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index e4bd3519e3362..ad305a3580c05 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1623,14 +1623,12 @@ static void php_cli_server_client_save_header(php_cli_server_client *client) zval tmp; ZVAL_STR(&tmp, client->current_header_value); /* strip off the colon */ - zend_string *perm_header_name = zend_string_dup(client->current_header_name, /* persistent */ true); zend_string *lc_header_name = zend_string_tolower_ex(client->current_header_name, /* persistent */ true); /* Add the wrapped zend_string to the HashTable */ zend_hash_add(&client->request.headers, lc_header_name, &tmp); - zend_hash_add(&client->request.headers_original_case, perm_header_name, &tmp); + zend_hash_add(&client->request.headers_original_case, client->current_header_name, &tmp); - zend_string_release_ex(client->current_header_name, /* persistent */ false); zend_string_release_ex(lc_header_name, /* persistent */ true); client->current_header_name = NULL; From fccca66a0ee273a04d5d753bd175ece60a3275e4 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 13 May 2022 11:45:51 +0100 Subject: [PATCH 12/25] Revert "Revert "Duplicate original name header"" Maybe this is needed actually This reverts commit 0162cff742372284da48cec5b2109955cc55fbce. --- sapi/cli/php_cli_server.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index ad305a3580c05..e4bd3519e3362 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1623,12 +1623,14 @@ static void php_cli_server_client_save_header(php_cli_server_client *client) zval tmp; ZVAL_STR(&tmp, client->current_header_value); /* strip off the colon */ + zend_string *perm_header_name = zend_string_dup(client->current_header_name, /* persistent */ true); zend_string *lc_header_name = zend_string_tolower_ex(client->current_header_name, /* persistent */ true); /* Add the wrapped zend_string to the HashTable */ zend_hash_add(&client->request.headers, lc_header_name, &tmp); - zend_hash_add(&client->request.headers_original_case, client->current_header_name, &tmp); + zend_hash_add(&client->request.headers_original_case, perm_header_name, &tmp); + zend_string_release_ex(client->current_header_name, /* persistent */ false); zend_string_release_ex(lc_header_name, /* persistent */ true); client->current_header_name = NULL; From a2d0106161712263187fd9321d42292374c1a13e Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 13 May 2022 19:01:07 +0100 Subject: [PATCH 13/25] Do not duplicate add_str, just add a refcount --- sapi/cli/php_cli_server.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index e4bd3519e3362..27f03b60bb72a 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1876,11 +1876,8 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se client->addr = addr; client->addr_len = addr_len; - // TODO Prevent realloc? - zend_string *tmp_addr = NULL; - php_network_populate_name_from_sockaddr(addr, addr_len, &tmp_addr, NULL, 0); - client->addr_str = zend_string_dup(tmp_addr, /* persistent */ true); - zend_string_release_ex(tmp_addr, /* persistent */ false); + php_network_populate_name_from_sockaddr(addr, addr_len, &client->addr_str, NULL, 0); + GC_ADDREF(client->addr_str); php_http_parser_init(&client->parser, PHP_HTTP_REQUEST); client->request_read = false; @@ -1905,7 +1902,7 @@ static void php_cli_server_client_dtor(php_cli_server_client *client) /* {{{ */ client->file_fd = -1; } pefree(client->addr, 1); - zend_string_release_ex(client->addr_str, /* persistent */ true); + zend_string_release_ex(client->addr_str, /* persistent */ false); /* Headers must be sent */ assert(client->current_header_name == NULL); From b0246d4f052f74481e0766a7da8b71c6f904f16b Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sat, 14 May 2022 00:27:22 +0100 Subject: [PATCH 14/25] Revert "Do not duplicate add_str, just add a refcount" This reverts commit a2d0106161712263187fd9321d42292374c1a13e. --- sapi/cli/php_cli_server.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 27f03b60bb72a..e4bd3519e3362 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1876,8 +1876,11 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se client->addr = addr; client->addr_len = addr_len; - php_network_populate_name_from_sockaddr(addr, addr_len, &client->addr_str, NULL, 0); - GC_ADDREF(client->addr_str); + // TODO Prevent realloc? + zend_string *tmp_addr = NULL; + php_network_populate_name_from_sockaddr(addr, addr_len, &tmp_addr, NULL, 0); + client->addr_str = zend_string_dup(tmp_addr, /* persistent */ true); + zend_string_release_ex(tmp_addr, /* persistent */ false); php_http_parser_init(&client->parser, PHP_HTTP_REQUEST); client->request_read = false; @@ -1902,7 +1905,7 @@ static void php_cli_server_client_dtor(php_cli_server_client *client) /* {{{ */ client->file_fd = -1; } pefree(client->addr, 1); - zend_string_release_ex(client->addr_str, /* persistent */ false); + zend_string_release_ex(client->addr_str, /* persistent */ true); /* Headers must be sent */ assert(client->current_header_name == NULL); From fa21d10ab4df2ca99a1aa03610b8918ca2e7d53c Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sat, 14 May 2022 00:29:58 +0100 Subject: [PATCH 15/25] Improve name + comment --- sapi/cli/php_cli_server.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index e4bd3519e3362..121eaa87c449c 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -267,7 +267,7 @@ static bool php_cli_server_get_system_time(char *buf) { #endif /* Destructor for php_cli_server_request->headers, this frees header value */ -static void char_ptr_dtor_p(zval *zv) /* {{{ */ +static void cli_header_value_dtor(zval *zv) /* {{{ */ { zend_string_release_ex(Z_STR_P(zv), /* persistent */ true); } /* }}} */ @@ -1335,7 +1335,7 @@ static void php_cli_server_request_ctor(php_cli_server_request *req) /* {{{ */ req->path_info_len = 0; req->query_string = NULL; req->query_string_len = 0; - zend_hash_init(&req->headers, 0, NULL, char_ptr_dtor_p, 1); + zend_hash_init(&req->headers, 0, NULL, cli_header_value_dtor, 1); /* No destructor is registered as the value pointed by is the same as for &req->headers */ zend_hash_init(&req->headers_original_case, 0, NULL, NULL, 1); req->content = NULL; @@ -1876,7 +1876,8 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se client->addr = addr; client->addr_len = addr_len; - // TODO Prevent realloc? + // TODO To prevent the reallocation need to retrieve a persistent string + // Create a new php_network_populate_name_from_sockaddr_ex() API with a persistent flag? zend_string *tmp_addr = NULL; php_network_populate_name_from_sockaddr(addr, addr_len, &tmp_addr, NULL, 0); client->addr_str = zend_string_dup(tmp_addr, /* persistent */ true); From cb78ce4540fbd89b11d261134f30308c0409a9fc Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 11:26:49 +0100 Subject: [PATCH 16/25] Server should not crash if request is interupted early --- sapi/cli/php_cli_server.c | 7 +++--- .../cli_server_persistent_string001.phpt | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 sapi/cli/tests/cli_server_persistent_string001.phpt diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 121eaa87c449c..139a4b48d415b 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1908,11 +1908,10 @@ static void php_cli_server_client_dtor(php_cli_server_client *client) /* {{{ */ pefree(client->addr, 1); zend_string_release_ex(client->addr_str, /* persistent */ true); - /* Headers must be sent */ - assert(client->current_header_name == NULL); - assert(client->current_header_value == NULL); - if (client->content_sender_initialized) { + /* Headers must be set if we reached the content initialisation */ + assert(client->current_header_name == NULL); + assert(client->current_header_value == NULL); php_cli_server_content_sender_dtor(&client->content_sender); } } /* }}} */ diff --git a/sapi/cli/tests/cli_server_persistent_string001.phpt b/sapi/cli/tests/cli_server_persistent_string001.phpt new file mode 100644 index 0000000000000..6820c6fc44087 --- /dev/null +++ b/sapi/cli/tests/cli_server_persistent_string001.phpt @@ -0,0 +1,23 @@ +--TEST-- +Close request before server sends a response +--SKIPIF-- + +--FILE-- + +===DONE=== +--EXPECT-- +string(11) "Hello world" +===DONE=== From 36f69ec527502408e0883857cf1a163d7fa0fb12 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 11:37:59 +0100 Subject: [PATCH 17/25] Make header name persistent --- sapi/cli/php_cli_server.c | 22 ++++----- .../cli_server_persistent_string002.phpt | 45 +++++++++++++++++++ 2 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 sapi/cli/tests/cli_server_persistent_string002.phpt diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 139a4b48d415b..81333e2f11cd1 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1623,14 +1623,12 @@ static void php_cli_server_client_save_header(php_cli_server_client *client) zval tmp; ZVAL_STR(&tmp, client->current_header_value); /* strip off the colon */ - zend_string *perm_header_name = zend_string_dup(client->current_header_name, /* persistent */ true); zend_string *lc_header_name = zend_string_tolower_ex(client->current_header_name, /* persistent */ true); /* Add the wrapped zend_string to the HashTable */ zend_hash_add(&client->request.headers, lc_header_name, &tmp); - zend_hash_add(&client->request.headers_original_case, perm_header_name, &tmp); + zend_hash_add(&client->request.headers_original_case, client->current_header_name, &tmp); - zend_string_release_ex(client->current_header_name, /* persistent */ false); zend_string_release_ex(lc_header_name, /* persistent */ true); client->current_header_name = NULL; @@ -1647,15 +1645,19 @@ static int php_cli_server_client_read_request_on_header_field(php_http_parser *p ZEND_FALLTHROUGH; case HEADER_NONE: /* Create new header field */ - client->current_header_name = zend_string_init(at, length, /* persistent */ false); + client->current_header_name = zend_string_init(at, length, /* persistent */ true); break; case HEADER_FIELD: { - /* Append header field */ - zend_string *field = zend_string_concat2( - ZSTR_VAL(client->current_header_name), ZSTR_LEN(client->current_header_name), at, length); - // Free previous - zend_string_release_ex(client->current_header_name, /* persistent */ false); - client->current_header_name = field; + /* Append header to the previous value of it */ + /* Assert that there is only one reference to the header name, as then zend_string_extends() + * will reallocate it such that we do not need to release the old value. */ + ZEND_ASSERT(GC_REFCOUNT(client->current_header_name) == 1); + /* Previous element was part of header value, append content to it */ + size_t old_length = ZSTR_LEN(client->current_header_name); + client->current_header_name = zend_string_extend(client->current_header_name, old_length + length, /* persistent */ true); + memcpy(ZSTR_VAL(client->current_header_name) + old_length, at, length); + // Null terminate + ZSTR_VAL(client->current_header_name)[ZSTR_LEN(client->current_header_name)] = '\0'; break; } } diff --git a/sapi/cli/tests/cli_server_persistent_string002.phpt b/sapi/cli/tests/cli_server_persistent_string002.phpt new file mode 100644 index 0000000000000..1358d15792b96 --- /dev/null +++ b/sapi/cli/tests/cli_server_persistent_string002.phpt @@ -0,0 +1,45 @@ +--TEST-- +Close request before server sends a response +--SKIPIF-- + +--FILE-- + +===DONE=== +--EXPECTF-- +HTTP/1.0 200 OK +Date: %s +Connection: close +X-Powered-By: %s +Content-type: text/html; charset=UTF-8 + +Hello worldHTTP/1.0 200 OK +Host: hello +Date: %s +Connection: close +X-Powered-By: %s +Content-type: text/html; charset=UTF-8 + +Hello world +string(11) "Hello world" +===DONE=== From c381cea91d0c420c3d149d8a598a0794374c1dec Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 11:42:14 +0100 Subject: [PATCH 18/25] Extract common code into a function --- sapi/cli/php_cli_server.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 81333e2f11cd1..8371bef2e8e68 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1635,6 +1635,19 @@ static void php_cli_server_client_save_header(php_cli_server_client *client) client->current_header_value = NULL; } +static void cli_concat_persistent_zstr_with_char(zend_string *str, const char *at, size_t length) +{ + /* Assert that there is only one reference to the string, as then zend_string_extends() + * will reallocate it such that we do not need to release the old value. */ + ZEND_ASSERT(GC_REFCOUNT(str) == 1); + /* Previous element was part of header value, append content to it */ + size_t old_length = ZSTR_LEN(str); + str = zend_string_extend(str, old_length + length, /* persistent */ true); + memcpy(ZSTR_VAL(str) + old_length, at, length); + // Null terminate + ZSTR_VAL(str)[ZSTR_LEN(str)] = '\0'; +} + static int php_cli_server_client_read_request_on_header_field(php_http_parser *parser, const char *at, size_t length) { php_cli_server_client *client = parser->data; @@ -1648,16 +1661,8 @@ static int php_cli_server_client_read_request_on_header_field(php_http_parser *p client->current_header_name = zend_string_init(at, length, /* persistent */ true); break; case HEADER_FIELD: { - /* Append header to the previous value of it */ - /* Assert that there is only one reference to the header name, as then zend_string_extends() - * will reallocate it such that we do not need to release the old value. */ - ZEND_ASSERT(GC_REFCOUNT(client->current_header_name) == 1); - /* Previous element was part of header value, append content to it */ - size_t old_length = ZSTR_LEN(client->current_header_name); - client->current_header_name = zend_string_extend(client->current_header_name, old_length + length, /* persistent */ true); - memcpy(ZSTR_VAL(client->current_header_name) + old_length, at, length); - // Null terminate - ZSTR_VAL(client->current_header_name)[ZSTR_LEN(client->current_header_name)] = '\0'; + /* Append header name to the previous value of it */ + cli_concat_persistent_zstr_with_char(client->current_header_name, at, length); break; } } @@ -1675,15 +1680,8 @@ static int php_cli_server_client_read_request_on_header_value(php_http_parser *p client->current_header_value = zend_string_init(at, length, /* persistent */ true); break; case HEADER_VALUE: { - /* Assert that there is only one reference to the header value, as then zend_string_extends() - * will reallocate it such that we do not need to release the old value. */ - ZEND_ASSERT(GC_REFCOUNT(client->current_header_value) == 1); - /* Previous element was part of header value, append content to it */ - size_t old_length = ZSTR_LEN(client->current_header_value); - client->current_header_value = zend_string_extend(client->current_header_value, old_length + length, /* persistent */ true); - memcpy(ZSTR_VAL(client->current_header_value) + old_length, at, length); - // Null terminate - ZSTR_VAL(client->current_header_value)[ZSTR_LEN(client->current_header_value)] = '\0'; + /* Append header value to the previous value of it */ + cli_concat_persistent_zstr_with_char(client->current_header_value, at, length); break; } case HEADER_NONE: From 5e1655264072d25f190ae0589ee31f413dfb7fa2 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 12:34:49 +0100 Subject: [PATCH 19/25] Output Server errors Thanks to Arnaud for figuring this out --- sapi/cli/tests/php_cli_server.inc | 36 +++++++++++++++++++------------ 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/sapi/cli/tests/php_cli_server.inc b/sapi/cli/tests/php_cli_server.inc index a7d68b16cf259..596c9efef11a5 100644 --- a/sapi/cli/tests/php_cli_server.inc +++ b/sapi/cli/tests/php_cli_server.inc @@ -28,37 +28,40 @@ function php_cli_server_start( $cmd[] = $router; } + $output_file = tempnam(sys_get_temp_dir(), 'cli_server_output'); + $output_file_fd = fopen($output_file, 'ab'); + if ($output_file_fd === false) { + die(sprintf("Failed opening output file %s\n", $output_file)); + } + $descriptorspec = array( 0 => STDIN, - 1 => STDOUT, - 2 => ['pipe', 'w'], + 1 => $output_file_fd, + 2 => $output_file_fd, ); $handle = proc_open($cmd, $descriptorspec, $pipes, $doc_root, null, array("suppress_errors" => true)); // First, wait for the dev server to declare itself ready. $bound = null; - stream_set_blocking($pipes[2], false); for ($i = 0; $i < 60; $i++) { usleep(50000); // 50ms per try $status = proc_get_status($handle); if (empty($status['running'])) { - echo "Server is not running\n"; + echo "Server failed to start\n"; + printf("Server output:\n%s\n", file_get_contents($output_file)); proc_terminate($handle); exit(1); } - while (($line = fgets($pipes[2])) !== false) { - if (preg_match('@PHP \S* Development Server \(https?://(.*?:\d+)\) started@', $line, $matches)) { - $bound = $matches[1]; - // Now that we've identified the listen address, close STDERR. - // Otherwise the pipe may clog up with unread log messages. - fclose($pipes[2]); - break 2; - } + $output = file_get_contents($output_file); + if (preg_match('@PHP \S* Development Server \(https?://(.*?:\d+)\) started@', $output, $matches)) { + $bound = $matches[1]; + break; } } if ($bound === null) { echo "Server did not output startup message"; + printf("Server output:\n%s\n", file_get_contents($output_file)); proc_terminate($handle); exit(1); } @@ -73,7 +76,7 @@ function php_cli_server_start( $fp = @fsockopen("tcp://$bound"); // Failure, the server is no longer running if (!($status && $status['running'])) { - $error = "Server is not running\n"; + $error = sprintf("Server stopped\nServer output:\n%s\n", file_get_contents($output_file)); break; } // Success, Connected to servers @@ -90,8 +93,13 @@ function php_cli_server_start( } register_shutdown_function( - function($handle) use($router, $doc_root) { + function($handle) use($router, $doc_root, $output_file) { proc_terminate($handle); + $status = proc_get_status($handle); + if ($status['exitcode'] !== -1 && $status['exitcode'] !== 0) { + printf("Server exitted with non-zero status: %d\n", $status['exitcode']); + printf("Server output:\n%s\n", file_get_contents($output_file)); + } @unlink(__DIR__ . "/{$router}"); @rmdir($doc_root); }, From 877007ebd5ad29d1a06481f43aabd9495fda0759 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 12:35:52 +0100 Subject: [PATCH 20/25] Spelling --- sapi/cli/tests/php_cli_server.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sapi/cli/tests/php_cli_server.inc b/sapi/cli/tests/php_cli_server.inc index 596c9efef11a5..43b6ec4b8024a 100644 --- a/sapi/cli/tests/php_cli_server.inc +++ b/sapi/cli/tests/php_cli_server.inc @@ -97,7 +97,7 @@ function php_cli_server_start( proc_terminate($handle); $status = proc_get_status($handle); if ($status['exitcode'] !== -1 && $status['exitcode'] !== 0) { - printf("Server exitted with non-zero status: %d\n", $status['exitcode']); + printf("Server exited with non-zero status: %d\n", $status['exitcode']); printf("Server output:\n%s\n", file_get_contents($output_file)); } @unlink(__DIR__ . "/{$router}"); From 866ffc68556913851c76127908386bb826f4c878 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 12:39:33 +0100 Subject: [PATCH 21/25] Fix heap use after free --- sapi/cli/php_cli_server.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index 8371bef2e8e68..c1cbd51934cd7 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1635,17 +1635,18 @@ static void php_cli_server_client_save_header(php_cli_server_client *client) client->current_header_value = NULL; } -static void cli_concat_persistent_zstr_with_char(zend_string *str, const char *at, size_t length) +static zend_string* cli_concat_persistent_zstr_with_char(zend_string *old_str, const char *at, size_t length) { /* Assert that there is only one reference to the string, as then zend_string_extends() * will reallocate it such that we do not need to release the old value. */ - ZEND_ASSERT(GC_REFCOUNT(str) == 1); + ZEND_ASSERT(GC_REFCOUNT(old_str) == 1); /* Previous element was part of header value, append content to it */ - size_t old_length = ZSTR_LEN(str); - str = zend_string_extend(str, old_length + length, /* persistent */ true); + size_t old_length = ZSTR_LEN(old_str); + zend_string *str = zend_string_extend(old_str, old_length + length, /* persistent */ true); memcpy(ZSTR_VAL(str) + old_length, at, length); // Null terminate ZSTR_VAL(str)[ZSTR_LEN(str)] = '\0'; + return str; } static int php_cli_server_client_read_request_on_header_field(php_http_parser *parser, const char *at, size_t length) @@ -1662,7 +1663,7 @@ static int php_cli_server_client_read_request_on_header_field(php_http_parser *p break; case HEADER_FIELD: { /* Append header name to the previous value of it */ - cli_concat_persistent_zstr_with_char(client->current_header_name, at, length); + client->current_header_name = cli_concat_persistent_zstr_with_char(client->current_header_name, at, length); break; } } @@ -1681,7 +1682,7 @@ static int php_cli_server_client_read_request_on_header_value(php_http_parser *p break; case HEADER_VALUE: { /* Append header value to the previous value of it */ - cli_concat_persistent_zstr_with_char(client->current_header_value, at, length); + client->current_header_value = cli_concat_persistent_zstr_with_char(client->current_header_value, at, length); break; } case HEADER_NONE: From ba5d78b8e18af7c6c8945962001c23c35b9233dc Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 12:43:35 +0100 Subject: [PATCH 22/25] Cleanup tests --- sapi/cli/tests/cli_server_persistent_string001.phpt | 4 ---- sapi/cli/tests/cli_server_persistent_string002.phpt | 5 +---- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/sapi/cli/tests/cli_server_persistent_string001.phpt b/sapi/cli/tests/cli_server_persistent_string001.phpt index 6820c6fc44087..6c7e2ef2bf5f1 100644 --- a/sapi/cli/tests/cli_server_persistent_string001.phpt +++ b/sapi/cli/tests/cli_server_persistent_string001.phpt @@ -13,11 +13,7 @@ $fd = stream_socket_client("tcp://" . PHP_CLI_SERVER_ADDRESS); fwrite($fd, "GET /index.php HTTP/1.0\r\nHost: hello"); fclose($fd); -/* This is needed to check that the Server didn't die */ -var_dump(file_get_contents("http://" . PHP_CLI_SERVER_ADDRESS)); - ?> ===DONE=== --EXPECT-- -string(11) "Hello world" ===DONE=== diff --git a/sapi/cli/tests/cli_server_persistent_string002.phpt b/sapi/cli/tests/cli_server_persistent_string002.phpt index 1358d15792b96..9c0ae28514b1d 100644 --- a/sapi/cli/tests/cli_server_persistent_string002.phpt +++ b/sapi/cli/tests/cli_server_persistent_string002.phpt @@ -1,5 +1,5 @@ --TEST-- -Close request before server sends a response +Server processing multiple request at the same time --SKIPIF-- ===DONE=== @@ -41,5 +39,4 @@ X-Powered-By: %s Content-type: text/html; charset=UTF-8 Hello world -string(11) "Hello world" ===DONE=== From 0af9a5e66fc0494348fdcde213bd04f1548d7d3d Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 12:55:01 +0100 Subject: [PATCH 23/25] Fix Clang MSAN --- sapi/cli/php_cli_server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c index c1cbd51934cd7..983d5f88bac62 100644 --- a/sapi/cli/php_cli_server.c +++ b/sapi/cli/php_cli_server.c @@ -1370,7 +1370,7 @@ static void php_cli_server_request_dtor(php_cli_server_request *req) /* {{{ */ static void php_cli_server_request_translate_vpath(php_cli_server_request *request, const char *document_root, size_t document_root_len) /* {{{ */ { - zend_stat_t sb; + zend_stat_t sb = {0}; static const char *index_files[] = { "index.php", "index.html", NULL }; char *buf = safe_pemalloc(1, request->vpath_len, 1 + document_root_len + 1 + sizeof("index.html"), 1); char *p = buf, *prev_path = NULL, *q, *vpath; @@ -2710,7 +2710,7 @@ int do_cli_server(int argc, char **argv) /* {{{ */ } if (document_root) { - zend_stat_t sb; + zend_stat_t sb = {0}; if (php_sys_stat(document_root, &sb)) { fprintf(stderr, "Directory %s does not exist.\n", document_root); From cd7d6950019fd8feefa726373574c7a8e9669040 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 18:02:47 +0100 Subject: [PATCH 24/25] Cater to Windows proc_terminate status code --- sapi/cli/tests/php_cli_server.inc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sapi/cli/tests/php_cli_server.inc b/sapi/cli/tests/php_cli_server.inc index 43b6ec4b8024a..089c3a5d49e10 100644 --- a/sapi/cli/tests/php_cli_server.inc +++ b/sapi/cli/tests/php_cli_server.inc @@ -96,7 +96,8 @@ function php_cli_server_start( function($handle) use($router, $doc_root, $output_file) { proc_terminate($handle); $status = proc_get_status($handle); - if ($status['exitcode'] !== -1 && $status['exitcode'] !== 0) { + if ($status['exitcode'] !== -1 && $status['exitcode'] !== 0 + && !($status['exitcode'] === 255 && PHP_OS == 'Windows')) { printf("Server exited with non-zero status: %d\n", $status['exitcode']); printf("Server output:\n%s\n", file_get_contents($output_file)); } From 1c821d826dd42fa26cbf1b096aaf1344b4840af3 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Sun, 15 May 2022 19:16:01 +0100 Subject: [PATCH 25/25] bleh --- sapi/cli/tests/php_cli_server.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sapi/cli/tests/php_cli_server.inc b/sapi/cli/tests/php_cli_server.inc index 089c3a5d49e10..65f2cc9a85dd4 100644 --- a/sapi/cli/tests/php_cli_server.inc +++ b/sapi/cli/tests/php_cli_server.inc @@ -97,7 +97,7 @@ function php_cli_server_start( proc_terminate($handle); $status = proc_get_status($handle); if ($status['exitcode'] !== -1 && $status['exitcode'] !== 0 - && !($status['exitcode'] === 255 && PHP_OS == 'Windows')) { + && !($status['exitcode'] === 255 && PHP_OS_FAMILY == 'Windows')) { printf("Server exited with non-zero status: %d\n", $status['exitcode']); printf("Server output:\n%s\n", file_get_contents($output_file)); }