From b157ae396cd69db81fe47d25ae173e4b9e60bf8d Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Fri, 17 Apr 2020 13:07:46 +0000 Subject: [PATCH] core, h2: send EOR for early HTTP request failure. The core output filters depend on EOR being sent at some point for correct accounting of setaside limits and lifetime. Rework ap_read_request() early failure (including in post_read_request() hooks) so that it always sends the EOR after ap_die(). Apply the same scheme in h2_request_create_rec() which is the HTTP/2 to HTTP/1 counterpart. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1876664 13f79535-47bb-0310-9956-ffa450edef68 --- modules/http2/h2_request.c | 58 +++++++++------ server/protocol.c | 142 +++++++++++++++++++++---------------- 2 files changed, 120 insertions(+), 80 deletions(-) diff --git a/modules/http2/h2_request.c b/modules/http2/h2_request.c index 639450243e..d8a1b7b4d2 100644 --- a/modules/http2/h2_request.c +++ b/modules/http2/h2_request.c @@ -267,7 +267,7 @@ static request_rec *my_ap_create_request(conn_rec *c) request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) { int access_status = HTTP_OK; - const char *rpath; + const char *rpath = req->path ? req->path : ""; const char *s; #if AP_MODULE_MAGIC_AT_LEAST(20150222, 13) @@ -276,8 +276,6 @@ request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) request_rec *r = my_ap_create_request(c); #endif - r->headers_in = apr_table_clone(r->pool, req->headers); - ap_run_pre_read_request(r, c); /* Time to populate r with the data we have. */ @@ -288,15 +286,19 @@ request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) if (r->method_number == M_GET && r->method[0] == 'H') { r->header_only = 1; } - - rpath = (req->path ? req->path : ""); - ap_parse_uri(r, rpath); - r->protocol = (char*)"HTTP/2.0"; r->proto_num = HTTP_VERSION(2, 0); - + r->protocol = apr_pstrdup(r->pool, "HTTP/2.0"); r->the_request = apr_psprintf(r->pool, "%s %s %s", r->method, rpath, r->protocol); - + r->headers_in = apr_table_clone(r->pool, req->headers); + + ap_parse_uri(r, rpath); + if (r->status != HTTP_OK) { + access_status = r->status; + r->status = HTTP_OK; + goto die; + } + /* update what we think the virtual host is based on the headers we've * now read. may update status. * Leave r->hostname empty, vhost will parse if form our Host: header, @@ -304,6 +306,11 @@ request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) */ r->hostname = NULL; ap_update_vhost_from_headers(r); + if (r->status != HTTP_OK) { + access_status = r->status; + r->status = HTTP_OK; + goto die; + } /* we may have switched to another server */ r->per_dir_config = r->server->lookup_defaults; @@ -314,8 +321,8 @@ request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) r->expecting_100 = 1; } else { - r->status = HTTP_EXPECTATION_FAILED; - ap_send_error_response(r, 0); + access_status = HTTP_EXPECTATION_FAILED; + goto die; } } @@ -328,28 +335,39 @@ request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) ap_add_input_filter_handle(ap_http_input_filter_handle, NULL, r, r->connection); - if (access_status != HTTP_OK - || (access_status = ap_run_post_read_request(r))) { + if ((access_status = ap_run_post_read_request(r))) { /* Request check post hooks failed. An example of this would be a * request for a vhost where h2 is disabled --> 421. */ ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(03367) "h2_request: access_status=%d, request_create failed", access_status); - ap_die(access_status, r); - ap_update_child_status(c->sbh, SERVER_BUSY_LOG, r); - ap_run_log_transaction(r); - r = NULL; - goto traceout; + goto die; } AP_READ_REQUEST_SUCCESS((uintptr_t)r, (char *)r->method, (char *)r->uri, (char *)r->server->defn_name, r->status); return r; -traceout: + +die: + ap_die(access_status, r); + + /* ap_die() sent the response through the output filters, we must now + * end the request with an EOR bucket for stream/pipeline accounting. + */ + { + apr_bucket_brigade *tmp_bb; + tmp_bb = ap_acquire_brigade(c); + APR_BRIGADE_INSERT_TAIL(tmp_bb, + ap_bucket_eor_create(c->bucket_alloc, r)); + ap_pass_brigade(c->output_filters, tmp_bb); + ap_release_brigade(c, tmp_bb); + } + + r = NULL; AP_READ_REQUEST_FAILURE((uintptr_t)r); - return r; + return NULL; } diff --git a/server/protocol.c b/server/protocol.c index 323b83e5f4..d8e4d0ae45 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -1352,6 +1352,7 @@ request_rec *ap_read_request(conn_rec *conn) apr_socket_t *csd; apr_interval_time_t cur_timeout; core_server_config *conf; + int strict_host_check; request_rec *r = ap_create_request(conn); @@ -1362,6 +1363,7 @@ request_rec *ap_read_request(conn_rec *conn) /* Get the request... */ if (!read_request_line(r, tmp_bb)) { + apr_brigade_cleanup(tmp_bb); switch (r->status) { case HTTP_REQUEST_URI_TOO_LARGE: case HTTP_BAD_REQUEST: @@ -1377,25 +1379,21 @@ request_rec *ap_read_request(conn_rec *conn) "request failed: malformed request line"); } access_status = r->status; - r->status = HTTP_OK; - ap_die(access_status, r); - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r); - ap_run_log_transaction(r); - r = NULL; - apr_brigade_destroy(tmp_bb); - goto traceout; + goto die_early; + case HTTP_REQUEST_TIME_OUT: + /* Just log, no further action on this connection. */ ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, NULL); if (!r->connection->keepalives) ap_run_log_transaction(r); - apr_brigade_destroy(tmp_bb); - goto traceout; - default: - apr_brigade_destroy(tmp_bb); - r = NULL; - goto traceout; + break; } + /* Not worth dying with. */ + conn->keepalive = AP_CONN_CLOSE; + apr_pool_destroy(r->pool); + goto ignore; } + apr_brigade_cleanup(tmp_bb); /* We may have been in keep_alive_timeout mode, so toggle back * to the normal timeout mode as we fetch the header lines, @@ -1412,14 +1410,12 @@ request_rec *ap_read_request(conn_rec *conn) const char *tenc; ap_get_mime_headers_core(r, tmp_bb); + apr_brigade_cleanup(tmp_bb); if (r->status != HTTP_OK) { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00567) "request failed: error reading the headers"); - ap_send_error_response(r, 0); - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r); - ap_run_log_transaction(r); - apr_brigade_destroy(tmp_bb); - goto traceout; + access_status = r->status; + goto die_early; } tenc = apr_table_get(r->headers_in, "Transfer-Encoding"); @@ -1434,13 +1430,8 @@ request_rec *ap_read_request(conn_rec *conn) ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02539) "client sent unknown Transfer-Encoding " "(%s): %s", tenc, r->uri); - r->status = HTTP_BAD_REQUEST; - conn->keepalive = AP_CONN_CLOSE; - ap_send_error_response(r, 0); - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r); - ap_run_log_transaction(r); - apr_brigade_destroy(tmp_bb); - goto traceout; + access_status = HTTP_BAD_REQUEST; + goto die_early; } /* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23 @@ -1453,14 +1444,12 @@ request_rec *ap_read_request(conn_rec *conn) } } - apr_brigade_destroy(tmp_bb); - /* update what we think the virtual host is based on the headers we've * now read. may update status. */ - - access_status = ap_update_vhost_from_headers_ex(r, conf->strict_host_check == AP_CORE_CONFIG_ON); - if (conf->strict_host_check == AP_CORE_CONFIG_ON && access_status != HTTP_OK) { + strict_host_check = (conf->strict_host_check == AP_CORE_CONFIG_ON); + access_status = ap_update_vhost_from_headers_ex(r, strict_host_check); + if (strict_host_check && access_status != HTTP_OK) { if (r->server == ap_server_conf) { ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10156) "Requested hostname '%s' did not match any ServerName/ServerAlias " @@ -1472,22 +1461,13 @@ request_rec *ap_read_request(conn_rec *conn) "current connection is %s:%u)", r->hostname, r->server->defn_name, r->server->defn_line_number); } - r->status = access_status; + goto die_early; } - - access_status = r->status; - - /* Toggle to the Host:-based vhost's timeout mode to fetch the - * request body and send the response body, if needed. - */ - if (cur_timeout != r->server->timeout) { - apr_socket_timeout_set(csd, r->server->timeout); - cur_timeout = r->server->timeout; + if (r->status != HTTP_OK) { + access_status = r->status; + goto die_early; } - /* we may have switched to another server */ - r->per_dir_config = r->server->lookup_defaults; - if ((!r->hostname && (r->proto_num >= HTTP_VERSION(1, 1))) || ((r->proto_num == HTTP_VERSION(1, 1)) && !apr_table_get(r->headers_in, "Host"))) { @@ -1498,10 +1478,23 @@ request_rec *ap_read_request(conn_rec *conn) * HTTP/1.1 mentions twice (S9, S14.23) that a request MUST contain * a Host: header, and the server MUST respond with 400 if it doesn't. */ - access_status = HTTP_BAD_REQUEST; ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569) "client sent HTTP/1.1 request without hostname " "(see RFC2616 section 14.23): %s", r->uri); + access_status = HTTP_BAD_REQUEST; + goto die_early; + } + + /* we may have switched to another server */ + r->per_dir_config = r->server->lookup_defaults; + conf = ap_get_core_module_config(r->server->module_config); + + /* Toggle to the Host:-based vhost's timeout mode to fetch the + * request body and send the response body, if needed. + */ + if (cur_timeout != r->server->timeout) { + apr_socket_timeout_set(csd, r->server->timeout); + cur_timeout = r->server->timeout; } /* @@ -1510,17 +1503,11 @@ request_rec *ap_read_request(conn_rec *conn) * status codes that do not cause the connection to be dropped and * in situations where the connection should be kept alive. */ - ap_add_input_filter_handle(ap_http_input_filter_handle, NULL, r, r->connection); - if (access_status != HTTP_OK - || (access_status = ap_run_post_read_request(r))) { - ap_die(access_status, r); - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r); - ap_run_log_transaction(r); - r = NULL; - goto traceout; + if ((access_status = ap_run_post_read_request(r))) { + goto die; } if (((expect = apr_table_get(r->headers_in, "Expect")) != NULL) @@ -1536,14 +1523,11 @@ request_rec *ap_read_request(conn_rec *conn) } else { if (conf->http_expect_strict != AP_HTTP_EXPECT_STRICT_DISABLE) { - r->status = HTTP_EXPECTATION_FAILED; ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00570) "client sent an unrecognized expectation value " "of Expect: %s", expect); - ap_send_error_response(r, 0); - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r); - ap_run_log_transaction(r); - goto traceout; + access_status = HTTP_EXPECTATION_FAILED; + goto die; } else { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02595) @@ -1553,11 +1537,49 @@ request_rec *ap_read_request(conn_rec *conn) } } - AP_READ_REQUEST_SUCCESS((uintptr_t)r, (char *)r->method, (char *)r->uri, (char *)r->server->defn_name, r->status); + AP_READ_REQUEST_SUCCESS((uintptr_t)r, (char *)r->method, + (char *)r->uri, (char *)r->server->defn_name, + r->status); return r; - traceout: + +die_early: + /* Input filters are in an undeterminate state, cleanup (including + * CORE_IN's socket) such that any further attempt to read is EOF. + */ + { + ap_filter_t *f = conn->input_filters; + while (f) { + ap_filter_reinstate_brigade(f, tmp_bb, NULL); + apr_brigade_cleanup(tmp_bb); + if (f->frec == ap_core_input_filter_handle) { + break; + } + ap_remove_input_filter(f); + f = f->next; + } + conn->input_filters = r->input_filters = f; + conn->keepalive = AP_CONN_CLOSE; + } + + /* fallthru ap_die() (non recursive) */ + r->status = HTTP_OK; +die: + ap_die(access_status, r); + + /* ap_die() sent the response through the output filters, we must now + * end the request with an EOR bucket for stream/pipeline accounting. + */ + tmp_bb = ap_acquire_brigade(conn); + APR_BRIGADE_INSERT_TAIL(tmp_bb, + ap_bucket_eor_create(conn->bucket_alloc, r)); + ap_pass_brigade(conn->output_filters, tmp_bb); + ap_release_brigade(conn, tmp_bb); + + /* fallthru */ +ignore: + r = NULL; AP_READ_REQUEST_FAILURE((uintptr_t)r); - return r; + return NULL; } /* if a request with a body creates a subrequest, remove original request's