From 97597452bbdadcbe561cd14fc3ee9049e7d371dd Mon Sep 17 00:00:00 2001 From: Graham Leggett Date: Sat, 16 Jan 2021 13:49:50 +0000 Subject: [PATCH] Backport to 2.4: *) mod_proxy_http: Fix 100-continue deadlock for spooled request bodies. BZ 63855. trunk patch: https://svn.apache.org/r1769760 https://svn.apache.org/r1770220 https://svn.apache.org/r1868576 https://svn.apache.org/r1868653 https://svn.apache.org/r1883639 https://svn.apache.org/r1883640 https://svn.apache.org/r1884218 https://svn.apache.org/r1884220 2.4.x patch: http://people.apache.org/~ylavic/patches/2.4.x-bz63855-1on5.patch https://github.com/apache/httpd/pull/154 +1: ylavic, covener, minfrin git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1885571 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 6 +++ STATUS | 12 ----- include/ap_mmn.h | 3 +- include/http_protocol.h | 11 +++++ modules/http/http_filters.c | 83 ++++++++++++++++++---------------- modules/http/http_protocol.c | 35 +++++++++++--- modules/proxy/mod_proxy_http.c | 34 +++----------- server/protocol.c | 15 ++++-- 8 files changed, 109 insertions(+), 90 deletions(-) diff --git a/CHANGES b/CHANGES index c11d634a14f..c11fef28f38 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,12 @@ -*- coding: utf-8 -*- Changes with Apache 2.4.47 + *) http: Allow unknown response status' lines returned in the form of + "HTTP/x.x xxx Status xxx". [Yann Ylavic] + + *) mod_proxy_http: Fix 100-continue deadlock for spooled request bodies, + leading to Request Timeout (408). PR 63855. [Yann Ylavic] + *) core: Remove headers on 304 Not Modified as specified by RFC7234, as opposed to passing an explicit subset of headers. PR 61820. [Giovanni Bechis] diff --git a/STATUS b/STATUS index f1c4de2261c..45985241f12 100644 --- a/STATUS +++ b/STATUS @@ -138,18 +138,6 @@ RELEASE SHOWSTOPPERS: PATCHES ACCEPTED TO BACKPORT FROM TRUNK: [ start all new proposals below, under PATCHES PROPOSED. ] - *) mod_proxy_http: Fix 100-continue deadlock for spooled request bodies. BZ 63855. - trunk patch: https://svn.apache.org/r1769760 - https://svn.apache.org/r1770220 - https://svn.apache.org/r1868576 - https://svn.apache.org/r1868653 - https://svn.apache.org/r1883639 - https://svn.apache.org/r1883640 - https://svn.apache.org/r1884218 - https://svn.apache.org/r1884220 - 2.4.x patch: http://people.apache.org/~ylavic/patches/2.4.x-bz63855-1on5.patch - https://github.com/apache/httpd/pull/154 - +1: ylavic, covener, minfrin PATCHES PROPOSED TO BACKPORT FROM TRUNK: diff --git a/include/ap_mmn.h b/include/ap_mmn.h index e23d79ce867..af464772f3f 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -537,6 +537,7 @@ * 20120211.93 (2.4.44-dev) Add ap_parse_strict_length() * 20120211.94 (2.4.47-dev) Add ap_proxy_define_match_worker() * 20120211.95 (2.4.47-dev) Add proxy check_trans hook + * 20120211.96 (2.4.47-dev) Add ap_get_status_line_ex() */ #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */ @@ -544,7 +545,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20120211 #endif -#define MODULE_MAGIC_NUMBER_MINOR 95 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 96 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/include/http_protocol.h b/include/http_protocol.h index 1ce1e7a9d11..48c54ada7af 100644 --- a/include/http_protocol.h +++ b/include/http_protocol.h @@ -466,6 +466,17 @@ AP_DECLARE(int) ap_index_of_response(int status); */ AP_DECLARE(const char *) ap_get_status_line(int status); +/** + * Return the Status-Line for a given status code (excluding the + * HTTP-Version field). If an invalid status code is passed, + * "500 Internal Server Error" will be returned, whereas an unknown + * status will be returned like "xxx Status xxx". + * @param p The pool to allocate from when status is unknown + * @param status The HTTP status code + * @return The Status-Line + */ +AP_DECLARE(const char *) ap_get_status_line_ex(apr_pool_t *p, int status); + /* Reading a block of data from the client connection (e.g., POST arg) */ /** diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index 251b0ff73d9..2823e270ba1 100644 --- a/modules/http/http_filters.c +++ b/modules/http/http_filters.c @@ -79,7 +79,8 @@ typedef struct http_filter_ctx BODY_CHUNK_END_LF, /* got CR after data, expect LF */ BODY_CHUNK_TRAILER /* trailers */ } state; - unsigned int eos_sent :1; + unsigned int eos_sent :1, + seen_data:1; apr_bucket_brigade *bb; } http_ctx_t; @@ -348,7 +349,6 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, http_ctx_t *ctx = f->ctx; apr_status_t rv; int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE; - apr_bucket_brigade *bb; int again; /* just get out of the way of things we don't want. */ @@ -361,7 +361,6 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(*ctx)); ctx->state = BODY_NONE; ctx->bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc); - bb = ctx->bb; /* LimitRequestBody does not apply to proxied responses. * Consider implementing this check in its own filter. @@ -448,42 +447,43 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ctx->eos_sent = 1; return APR_SUCCESS; } + } - /* Since we're about to read data, send 100-Continue if needed. - * Only valid on chunked and C-L bodies where the C-L is > 0. */ - if ((ctx->state == BODY_CHUNK || - (ctx->state == BODY_LENGTH && ctx->remaining > 0)) && - f->r->expecting_100 && f->r->proto_num >= HTTP_VERSION(1,1) && - !(f->r->eos_sent || f->r->bytes_sent)) { - if (!ap_is_HTTP_SUCCESS(f->r->status)) { - ctx->state = BODY_NONE; - ctx->eos_sent = 1; - } - else { - char *tmp; - int len; - - /* if we send an interim response, we're no longer - * in a state of expecting one. - */ - f->r->expecting_100 = 0; - tmp = apr_pstrcat(f->r->pool, AP_SERVER_PROTOCOL " ", - ap_get_status_line(HTTP_CONTINUE), CRLF CRLF, - NULL); - len = strlen(tmp); - ap_xlate_proto_to_ascii(tmp, len); - apr_brigade_cleanup(bb); - e = apr_bucket_pool_create(tmp, len, f->r->pool, - f->c->bucket_alloc); - APR_BRIGADE_INSERT_HEAD(bb, e); - e = apr_bucket_flush_create(f->c->bucket_alloc); - APR_BRIGADE_INSERT_TAIL(bb, e); - - rv = ap_pass_brigade(f->c->output_filters, bb); - if (rv != APR_SUCCESS) { - return AP_FILTER_ERROR; - } - } + /* Since we're about to read data, send 100-Continue if needed. + * Only valid on chunked and C-L bodies where the C-L is > 0. + * + * If the read is to be nonblocking though, the caller may not want to + * handle this just now (e.g. mod_proxy_http), and is prepared to read + * nothing if the client really waits for 100 continue, so we don't + * send it now and wait for later blocking read. + * + * In any case, even if r->expecting remains set at the end of the + * request handling, ap_set_keepalive() will finally do the right + * thing (i.e. "Connection: close" the connection). + */ + if (block == APR_BLOCK_READ + && (ctx->state == BODY_CHUNK + || (ctx->state == BODY_LENGTH && ctx->remaining > 0)) + && f->r->expecting_100 && f->r->proto_num >= HTTP_VERSION(1,1) + && !(ctx->eos_sent || f->r->eos_sent || f->r->bytes_sent)) { + if (!ap_is_HTTP_SUCCESS(f->r->status)) { + ctx->state = BODY_NONE; + ctx->eos_sent = 1; /* send EOS below */ + } + else if (!ctx->seen_data) { + int saved_status = f->r->status; + f->r->status = HTTP_CONTINUE; + ap_send_interim_response(f->r, 0); + AP_DEBUG_ASSERT(!f->r->expecting_100); + f->r->status = saved_status; + } + else { + /* https://tools.ietf.org/html/rfc7231#section-5.1.1 + * A server MAY omit sending a 100 (Continue) response if it + * has already received some or all of the message body for + * the corresponding request [...] + */ + f->r->expecting_100 = 0; } } @@ -534,9 +534,11 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, int parsing = 0; rv = apr_bucket_read(e, &buffer, &len, APR_BLOCK_READ); - if (rv == APR_SUCCESS) { parsing = 1; + if (len > 0) { + ctx->seen_data = 1; + } rv = parse_chunk_size(ctx, buffer, len, f->r->server->limit_req_fieldsize, strict); } @@ -598,6 +600,9 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, /* How many bytes did we just read? */ apr_brigade_length(b, 0, &totalread); + if (totalread > 0) { + ctx->seen_data = 1; + } /* If this happens, we have a bucket of unknown length. Die because * it means our assumptions have changed. */ diff --git a/modules/http/http_protocol.c b/modules/http/http_protocol.c index 6e0301abf4c..9c73baa7fc6 100644 --- a/modules/http/http_protocol.c +++ b/modules/http/http_protocol.c @@ -986,14 +986,17 @@ AP_DECLARE(const char *) ap_method_name_of(apr_pool_t *p, int methnum) * from status_lines[shortcut[i]] to status_lines[shortcut[i+1]-1]; * or use NULL to fill the gaps. */ -AP_DECLARE(int) ap_index_of_response(int status) +static int index_of_response(int status) { - static int shortcut[6] = {0, LEVEL_200, LEVEL_300, LEVEL_400, - LEVEL_500, RESPONSE_CODES}; + static int shortcut[6] = {0, LEVEL_200, LEVEL_300, LEVEL_400, LEVEL_500, + RESPONSE_CODES}; int i, pos; - if (status < 100) { /* Below 100 is illegal for HTTP status */ - return LEVEL_500; + if (status < 100) { /* Below 100 is illegal for HTTP status */ + return -1; + } + if (status > 999) { /* Above 999 is also illegal for HTTP status */ + return -1; } for (i = 0; i < 5; i++) { @@ -1004,11 +1007,29 @@ AP_DECLARE(int) ap_index_of_response(int status) return pos; } else { - return LEVEL_500; /* status unknown (falls in gap) */ + break; } } } - return LEVEL_500; /* 600 or above is also illegal */ + return -2; /* Status unknown (falls in gap) or above 600 */ +} + +AP_DECLARE(int) ap_index_of_response(int status) +{ + int index = index_of_response(status); + return (index < 0) ? LEVEL_500 : index; +} + +AP_DECLARE(const char *) ap_get_status_line_ex(apr_pool_t *p, int status) +{ + int index = index_of_response(status); + if (index >= 0) { + return status_lines[index]; + } + else if (index == -2) { + return apr_psprintf(p, "%i Status %i", status, status); + } + return status_lines[LEVEL_500]; } AP_DECLARE(const char *) ap_get_status_line(int status) diff --git a/modules/proxy/mod_proxy_http.c b/modules/proxy/mod_proxy_http.c index 044bfcd7ab4..7e35429c02a 100644 --- a/modules/proxy/mod_proxy_http.c +++ b/modules/proxy/mod_proxy_http.c @@ -261,7 +261,6 @@ typedef struct { rb_methods rb_method; - int expecting_100; unsigned int do_100_continue:1, prefetch_nonblocking:1; } proxy_http_req_t; @@ -580,7 +579,7 @@ static int ap_proxy_http_prefetch(proxy_http_req_t *req, conn_rec *origin = p_conn->connection; if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) { - if (req->expecting_100) { + if (r->expecting_100) { return HTTP_EXPECTATION_FAILED; } force10 = 1; @@ -1499,8 +1498,9 @@ int ap_proxy_http_process_response(proxy_http_req_t *req) * * So let's make it configurable. * - * We need to set "r->expecting_100 = 1" otherwise origin - * server behaviour will apply. + * We need to force "r->expecting_100 = 1" for RFC behaviour + * otherwise ap_send_interim_response() does nothing when + * the client did not ask for 100-continue. */ const char *policy = apr_table_get(r->subprocess_env, "proxy-interim-response"); @@ -1509,11 +1509,7 @@ int ap_proxy_http_process_response(proxy_http_req_t *req) if (!policy || (!strcasecmp(policy, "RFC") && (proxy_status != HTTP_CONTINUE - || (req->expecting_100 = 1)))) { - if (proxy_status == HTTP_CONTINUE) { - r->expecting_100 = req->expecting_100; - req->expecting_100 = 0; - } + || (r->expecting_100 = 1)))) { ap_send_interim_response(r, 1); } /* FIXME: refine this to be able to specify per-response-status @@ -1601,12 +1597,10 @@ int ap_proxy_http_process_response(proxy_http_req_t *req) * here though, because error_override or a potential retry on * another backend could finally read that data and finalize * the request processing, making keep-alive possible. So what - * we do is restoring r->expecting_100 for ap_set_keepalive() - * to do the right thing according to the final response and + * we do is leaving r->expecting_100 alone, ap_set_keepalive() + * will do the right thing according to the final response and * any later update of r->expecting_100. */ - r->expecting_100 = req->expecting_100; - req->expecting_100 = 0; } /* Once only! */ @@ -2010,17 +2004,7 @@ static int proxy_http_handler(request_rec *r, proxy_worker *worker, /* Should we handle end-to-end or ping 100-continue? */ if ((r->expecting_100 && (dconf->forward_100_continue || input_brigade)) || PROXY_DO_100_CONTINUE(worker, r)) { - /* We need to reset r->expecting_100 or prefetching will cause - * ap_http_filter() to send "100 Continue" response by itself. So - * we'll use req->expecting_100 in mod_proxy_http to determine whether - * the client should be forwarded "100 continue", and r->expecting_100 - * will be restored at the end of the function with the actual value of - * req->expecting_100 (i.e. cleared only if mod_proxy_http sent the - * "100 Continue" according to its policy). - */ req->do_100_continue = req->prefetch_nonblocking = 1; - req->expecting_100 = r->expecting_100; - r->expecting_100 = 0; } /* Should we block while prefetching the body or try nonblocking and flush * data to the backend ASAP? @@ -2155,10 +2139,6 @@ cleanup: req->backend->close = 1; ap_proxy_http_cleanup(proxy_function, r, req->backend); } - if (req->expecting_100) { - /* Restore r->expecting_100 if we didn't touch it */ - r->expecting_100 = req->expecting_100; - } return status; } diff --git a/server/protocol.c b/server/protocol.c index d6c67934f52..d32ea1abb53 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -2201,7 +2201,8 @@ static int send_header(void *data, const char *key, const char *val) AP_DECLARE(void) ap_send_interim_response(request_rec *r, int send_headers) { hdr_ptr x; - char *status_line = NULL; + char *response_line = NULL; + const char *status_line; request_rec *rr; if (r->proto_num < HTTP_VERSION(1,1)) { @@ -2232,13 +2233,19 @@ AP_DECLARE(void) ap_send_interim_response(request_rec *r, int send_headers) } } - status_line = apr_pstrcat(r->pool, AP_SERVER_PROTOCOL, " ", r->status_line, CRLF, NULL); - ap_xlate_proto_to_ascii(status_line, strlen(status_line)); + status_line = r->status_line; + if (status_line == NULL) { + status_line = ap_get_status_line_ex(r->pool, r->status); + } + response_line = apr_pstrcat(r->pool, + AP_SERVER_PROTOCOL " ", status_line, CRLF, + NULL); + ap_xlate_proto_to_ascii(response_line, strlen(response_line)); x.f = r->connection->output_filters; x.bb = apr_brigade_create(r->pool, r->connection->bucket_alloc); - ap_fputs(x.f, x.bb, status_line); + ap_fputs(x.f, x.bb, response_line); if (send_headers) { apr_table_do(send_header, &x, r->headers_out, NULL); apr_table_clear(r->headers_out); -- 2.47.3