From: William A. Rowe Jr Date: Sat, 6 Aug 2005 21:29:05 +0000 (+0000) Subject: As much as it pains me, seriously, it seems that reviewing the re-backport X-Git-Tag: 2.0.55~103 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d9552365a9aeac0ec908863e0b09c4bd2939de98;p=thirdparty%2Fapache%2Fhttpd.git As much as it pains me, seriously, it seems that reviewing the re-backport of this code was too illegible for review, so it seems we will need to re-review a fresh backport from httpd trunk. Vetoed this patch as it does not follow RFC 2616. Specifically; * it does not respect the T-E over the C-L header * it introduces edge cases that allow the administrator to override the C-L T-E elections, without enough information to prevent splitting the request body due to misaligned C-L's * it does not attempt to determine if there is a 'small request body' to prefer a legitimate C-L when that option might be available to us A revised patch combining Jeff's efforts and mine is available at: http://people.apache.org/~wrowe/httpd-2.0.54-proxy-request.patch for review, which can be applied to httpd-2.0.54 and now, branches/2.0.x, which addresses all of the issues cited above. Note that I do not consider the correction of protocol.c to strip the C-L where T-E is present to fulfill the need for a correct implementation in proxy_http.c; modules have too much flexibility to modify the headers before they hit this logic in the proxy request body handling. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.0.x@230592 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 6cf4d938602..e7abdbaa843 100644 --- a/CHANGES +++ b/CHANGES @@ -42,11 +42,6 @@ Changes with Apache 2.0.55 *) mod_rewrite: use buffered I/O to improve performance with large RewriteMap txt: files. [Greg Ames] - *) proxy HTTP: Rework the handling of request bodies to handle - chunked input and input filters which modify content length, and - avoid spooling arbitrary-sized request bodies in memory. - PR 15859. [Jeff Trawick] - Changes with Apache 2.0.54 *) mod_cache: Add CacheIgnoreHeaders directive. PR 30399. diff --git a/STATUS b/STATUS index 407082c2c68..d837007ecb6 100644 --- a/STATUS +++ b/STATUS @@ -232,6 +232,24 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK: nd: I'm going to reverse the default jerenkrantz, striker: I'm confused as to the status of this backport. + * Rewrite how proxy sends its request to allow input bodies to + morph the request bodies. Previously, if an input filter + changed the request body, the original C-L would be sent which + would be incorrect. + + Due to HTTP compliance, we must either send the body T-E: chunked + or include a C-L for the request body. Connection: Close is not + an option. + + A newer version of this fix that acts even better (spools to disk) has + been committed to trunk. The equivalent patch for 2.0.x is at: + http://www.apache.org/~trawick/20reqbody.txt + + +1: trawick, jerenkrantz, jim + Previous votes (before trawick's recent 2.1 commits): + +1: stoddard, striker, jim + -1: brianp (we need a more robust solution than what's in 2.1 right now), + * support/check_forensic: Fix tempfile usage svn rev 125495, 126224 jerenkrantz says: r126224 fixes brokenness with r125495 on Solaris. diff --git a/modules/proxy/proxy_http.c b/modules/proxy/proxy_http.c index 57e31d99a23..e238649922e 100644 --- a/modules/proxy/proxy_http.c +++ b/modules/proxy/proxy_http.c @@ -384,480 +384,6 @@ apr_status_t ap_proxy_http_create_connection(apr_pool_t *p, request_rec *r, return OK; } -static void add_te_chunked(apr_pool_t *p, - apr_bucket_alloc_t *bucket_alloc, - apr_bucket_brigade *header_brigade) -{ - apr_bucket *e; - char *buf; - const char te_hdr[] = "Transfer-Encoding: chunked" CRLF; - - buf = apr_pmemdup(p, te_hdr, sizeof(te_hdr)-1); - ap_xlate_proto_to_ascii(buf, sizeof(te_hdr)-1); - - e = apr_bucket_pool_create(buf, sizeof(te_hdr)-1, p, bucket_alloc); - APR_BRIGADE_INSERT_TAIL(header_brigade, e); -} - -static void add_cl(apr_pool_t *p, - apr_bucket_alloc_t *bucket_alloc, - apr_bucket_brigade *header_brigade, - const char *cl_val) -{ - apr_bucket *e; - char *buf; - - buf = apr_pstrcat(p, "Content-Length: ", - cl_val, - CRLF, - NULL); - ap_xlate_proto_to_ascii(buf, strlen(buf)); - e = apr_bucket_pool_create(buf, strlen(buf), p, bucket_alloc); - APR_BRIGADE_INSERT_TAIL(header_brigade, e); -} - -#define ASCII_CRLF "\015\012" -#define ASCII_ZERO "\060" - -static void terminate_headers(apr_bucket_alloc_t *bucket_alloc, - apr_bucket_brigade *header_brigade) -{ - apr_bucket *e; - - /* add empty line at the end of the headers */ - e = apr_bucket_immortal_create(ASCII_CRLF, 2, bucket_alloc); - APR_BRIGADE_INSERT_TAIL(header_brigade, e); -} - -static apr_status_t pass_brigade(apr_bucket_alloc_t *bucket_alloc, - request_rec *r, proxy_http_conn_t *p_conn, - conn_rec *origin, apr_bucket_brigade *b, - int flush) -{ - apr_status_t status; - - if (flush) { - apr_bucket *e = apr_bucket_flush_create(bucket_alloc); - APR_BRIGADE_INSERT_TAIL(b, e); - } - status = ap_pass_brigade(origin->output_filters, b); - if (status != APR_SUCCESS) { - ap_log_error(APLOG_MARK, APLOG_ERR, status, r->server, - "proxy: pass request data failed to %pI (%s)", - p_conn->addr, p_conn->name); - return status; - } - apr_brigade_cleanup(b); - return APR_SUCCESS; -} - -static apr_status_t stream_reqbody_chunked(apr_pool_t *p, - request_rec *r, - proxy_http_conn_t *p_conn, - conn_rec *origin, - apr_bucket_brigade *header_brigade) -{ - int seen_eos = 0; - apr_size_t hdr_len; - apr_off_t bytes; - apr_status_t status; - apr_bucket_alloc_t *bucket_alloc = r->connection->bucket_alloc; - apr_bucket_brigade *b, *input_brigade; - apr_bucket *e; - - input_brigade = apr_brigade_create(p, bucket_alloc); - - do { - char chunk_hdr[20]; /* must be here due to transient bucket. */ - - status = ap_get_brigade(r->input_filters, input_brigade, - AP_MODE_READBYTES, APR_BLOCK_READ, - HUGE_STRING_LEN); - - if (status != APR_SUCCESS) { - return status; - } - - /* If this brigade contains EOS, either stop or remove it. */ - if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) { - seen_eos = 1; - - /* As a shortcut, if this brigade is simply an EOS bucket, - * don't send anything down the filter chain. - */ - if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade))) { - break; - } - - /* We can't pass this EOS to the output_filters. */ - e = APR_BRIGADE_LAST(input_brigade); - apr_bucket_delete(e); - } - - apr_brigade_length(input_brigade, 1, &bytes); - - hdr_len = apr_snprintf(chunk_hdr, sizeof(chunk_hdr), - "%" APR_UINT64_T_HEX_FMT CRLF, - (apr_uint64_t)bytes); - - ap_xlate_proto_to_ascii(chunk_hdr, hdr_len); - e = apr_bucket_transient_create(chunk_hdr, hdr_len, - bucket_alloc); - APR_BRIGADE_INSERT_HEAD(input_brigade, e); - - /* - * Append the end-of-chunk CRLF - */ - e = apr_bucket_immortal_create(ASCII_CRLF, 2, bucket_alloc); - APR_BRIGADE_INSERT_TAIL(input_brigade, e); - - if (header_brigade) { - /* we never sent the header brigade, so go ahead and - * take care of that now - */ - add_te_chunked(p, bucket_alloc, header_brigade); - terminate_headers(bucket_alloc, header_brigade); - b = header_brigade; - APR_BRIGADE_CONCAT(b, input_brigade); - header_brigade = NULL; - } - else { - b = input_brigade; - } - - status = pass_brigade(bucket_alloc, r, p_conn, origin, b, 0); - if (status != APR_SUCCESS) { - return status; - } - } while (!seen_eos); - - if (header_brigade) { - /* we never sent the header brigade because there was no request body; - * send it now without T-E - */ - terminate_headers(bucket_alloc, header_brigade); - b = header_brigade; - } - else { - if (!APR_BRIGADE_EMPTY(input_brigade)) { - /* input brigade still has an EOS which we can't pass to the output_filters. */ - e = APR_BRIGADE_LAST(input_brigade); - AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(e)); - apr_bucket_delete(e); - } - e = apr_bucket_immortal_create(ASCII_ZERO ASCII_CRLF - /* */ - ASCII_CRLF, - 5, bucket_alloc); - APR_BRIGADE_INSERT_TAIL(input_brigade, e); - b = input_brigade; - } - - status = pass_brigade(bucket_alloc, r, p_conn, origin, b, 1); - return status; -} - -static apr_status_t stream_reqbody_cl(apr_pool_t *p, - request_rec *r, - proxy_http_conn_t *p_conn, - conn_rec *origin, - apr_bucket_brigade *header_brigade, - const char *old_cl_val) -{ - int seen_eos = 0; - apr_status_t status; - apr_bucket_alloc_t *bucket_alloc = r->connection->bucket_alloc; - apr_bucket_brigade *b, *input_brigade; - apr_bucket *e; - - input_brigade = apr_brigade_create(p, bucket_alloc); - - do { - status = ap_get_brigade(r->input_filters, input_brigade, - AP_MODE_READBYTES, APR_BLOCK_READ, - HUGE_STRING_LEN); - - if (status != APR_SUCCESS) { - return status; - } - - /* If this brigade contains EOS, either stop or remove it. */ - if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) { - seen_eos = 1; - - /* As a shortcut, if this brigade is simply an EOS bucket, - * don't send anything down the filter chain. - */ - if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade))) { - break; - } - - /* We can't pass this EOS to the output_filters. */ - e = APR_BRIGADE_LAST(input_brigade); - apr_bucket_delete(e); - } - - if (header_brigade) { - /* we never sent the header brigade, so go ahead and - * take care of that now - */ - add_cl(p, bucket_alloc, header_brigade, old_cl_val); - terminate_headers(bucket_alloc, header_brigade); - b = header_brigade; - APR_BRIGADE_CONCAT(b, input_brigade); - header_brigade = NULL; - } - else { - b = input_brigade; - } - - status = pass_brigade(bucket_alloc, r, p_conn, origin, b, 0); - if (status != APR_SUCCESS) { - return status; - } - } while (!seen_eos); - - if (header_brigade) { - /* we never sent the header brigade since there was no request - * body; send it now, and only specify C-L if client specified - * C-L: 0 - */ - if (!strcmp(old_cl_val, "0")) { - add_cl(p, bucket_alloc, header_brigade, old_cl_val); - } - terminate_headers(bucket_alloc, header_brigade); - b = header_brigade; - } - else { - /* need to flush any pending data */ - b = input_brigade; /* empty now; pass_brigade() will add flush */ - } - status = pass_brigade(bucket_alloc, r, p_conn, origin, b, 1); - return status; -} - -#define MAX_MEM_SPOOL 16384 - -static apr_status_t spool_reqbody_cl(apr_pool_t *p, - request_rec *r, - proxy_http_conn_t *p_conn, - conn_rec *origin, - apr_bucket_brigade *header_brigade) -{ - int seen_eos = 0; - apr_status_t status; - apr_bucket_alloc_t *bucket_alloc = r->connection->bucket_alloc; - apr_bucket_brigade *body_brigade, *input_brigade; - apr_bucket *e; - apr_off_t bytes, bytes_spooled = 0, fsize = 0; - apr_file_t *tmpfile = NULL; - - body_brigade = apr_brigade_create(p, bucket_alloc); - input_brigade = apr_brigade_create(p, bucket_alloc); - - do { - status = ap_get_brigade(r->input_filters, input_brigade, - AP_MODE_READBYTES, APR_BLOCK_READ, - HUGE_STRING_LEN); - - if (status != APR_SUCCESS) { - return status; - } - - /* If this brigade contains EOS, either stop or remove it. */ - if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) { - seen_eos = 1; - - /* As a shortcut, if this brigade is simply an EOS bucket, - * don't send anything down the filter chain. - */ - if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade))) { - break; - } - - /* We can't pass this EOS to the output_filters. */ - e = APR_BRIGADE_LAST(input_brigade); - apr_bucket_delete(e); - } - - apr_brigade_length(input_brigade, 1, &bytes); - - if (bytes_spooled + bytes > MAX_MEM_SPOOL) { - /* can't spool any more in memory; write latest brigade to disk */ - if (tmpfile == NULL) { - const char *temp_dir; - char *template; - - status = apr_temp_dir_get(&temp_dir, p); - if (status != APR_SUCCESS) { - ap_log_error(APLOG_MARK, APLOG_ERR, status, r->server, - "proxy: search for temporary directory failed"); - return status; - } - apr_filepath_merge(&template, temp_dir, - "modproxy.tmp.XXXXXX", - APR_FILEPATH_NATIVE, p); - status = apr_file_mktemp(&tmpfile, template, 0, p); - if (status != APR_SUCCESS) { - ap_log_error(APLOG_MARK, APLOG_ERR, status, r->server, - "proxy: creation of temporary file in directory %s failed", - temp_dir); - return status; - } - } - for (e = APR_BRIGADE_FIRST(input_brigade); - e != APR_BRIGADE_SENTINEL(input_brigade); - e = APR_BUCKET_NEXT(e)) { - const char *data; - apr_size_t bytes_read, bytes_written; - - apr_bucket_read(e, &data, &bytes_read, APR_BLOCK_READ); - status = apr_file_write_full(tmpfile, data, bytes_read, &bytes_written); - if (status != APR_SUCCESS) { - const char *tmpfile_name; - - if (apr_file_name_get(&tmpfile_name, tmpfile) != APR_SUCCESS) { - tmpfile_name = "(unknown)"; - } - ap_log_error(APLOG_MARK, APLOG_ERR, status, r->server, - "proxy: write to temporary file %s failed", - tmpfile_name); - return status; - } - AP_DEBUG_ASSERT(bytes_read == bytes_written); - fsize += bytes_written; - } - apr_brigade_cleanup(input_brigade); - } - else { - APR_BRIGADE_CONCAT(body_brigade, input_brigade); - } - - bytes_spooled += bytes; - - } while (!seen_eos); - - if (bytes_spooled) { - add_cl(p, bucket_alloc, header_brigade, apr_off_t_toa(p, bytes_spooled)); - } - terminate_headers(bucket_alloc, header_brigade); - APR_BRIGADE_CONCAT(header_brigade, body_brigade); - if (tmpfile) { - /* For platforms where the size of the file may be larger than - * that which can be stored in a single bucket (where the - * length field is an apr_size_t), split it into several - * buckets: */ - if (sizeof(apr_off_t) > sizeof(apr_size_t) - && fsize > AP_MAX_SENDFILE) { - e = apr_bucket_file_create(tmpfile, 0, AP_MAX_SENDFILE, p, - bucket_alloc); - while (fsize > AP_MAX_SENDFILE) { - apr_bucket *ce; - apr_bucket_copy(e, &ce); - APR_BRIGADE_INSERT_TAIL(header_brigade, ce); - e->start += AP_MAX_SENDFILE; - fsize -= AP_MAX_SENDFILE; - } - e->length = (apr_size_t)fsize; /* Resize just the last bucket */ - } - else { - e = apr_bucket_file_create(tmpfile, 0, (apr_size_t)fsize, p, - bucket_alloc); - } - APR_BRIGADE_INSERT_TAIL(header_brigade, e); - } - status = pass_brigade(bucket_alloc, r, p_conn, origin, header_brigade, 1); - return status; -} - -static apr_status_t send_request_body(apr_pool_t *p, - request_rec *r, - proxy_http_conn_t *p_conn, - conn_rec *origin, - apr_bucket_brigade *header_brigade, - int force10) -{ - enum {RB_INIT, RB_STREAM_CL, RB_STREAM_CHUNKED, RB_SPOOL_CL} rb_method = RB_INIT; - const char *old_cl_val, *te_val; - int cl_zero; /* client sent "Content-Length: 0", which we forward on to server */ - apr_status_t status; - - /* send CL or use chunked encoding? - * - * . CL is the most friendly to the origin server since it is the - * most widely supported - * . CL stinks if we don't know the length since we have to buffer - * the data in memory or on disk until we get the entire data - * - * special cases to check for: - * . if we're using HTTP/1.0 to origin server, then we must send CL - * . if client sent C-L and there are no input resource filters, the - * the body size can't change so we send the same CL and stream the - * body - * . if client used chunked or proxy-sendchunks is set, we'll also - * use chunked - * - * normal case: - * we have to compute content length by reading the entire request - * body; if request body is not small, we'll spool the remaining input - * to a temporary file - * - * special envvars to override the normal decision: - * . proxy-sendchunks - * use chunked encoding; not compatible with force-proxy-request-1.0 - * . proxy-sendcl - * spool the request body to compute C-L - * . proxy-sendunchangedcl - * use C-L from client and spool the request body - */ - old_cl_val = apr_table_get(r->headers_in, "Content-Length"); - cl_zero = old_cl_val && !strcmp(old_cl_val, "0"); - - if (!force10 - && !cl_zero - && apr_table_get(r->subprocess_env, "proxy-sendchunks")) { - rb_method = RB_STREAM_CHUNKED; - } - else if (!cl_zero - && apr_table_get(r->subprocess_env, "proxy-sendcl")) { - rb_method = RB_SPOOL_CL; - } - else { - if (old_cl_val && - (r->input_filters == r->proto_input_filters - || cl_zero - || apr_table_get(r->subprocess_env, "proxy-sendunchangedcl"))) { - rb_method = RB_STREAM_CL; - } - else if (force10) { - rb_method = RB_SPOOL_CL; - } - else if ((te_val = apr_table_get(r->headers_in, "Transfer-Encoding")) - && !strcasecmp(te_val, "chunked")) { - rb_method = RB_STREAM_CHUNKED; - } - else { - rb_method = RB_SPOOL_CL; - } - } - - switch(rb_method) { - case RB_STREAM_CHUNKED: - status = stream_reqbody_chunked(p, r, p_conn, origin, header_brigade); - break; - case RB_STREAM_CL: - status = stream_reqbody_cl(p, r, p_conn, origin, header_brigade, old_cl_val); - break; - case RB_SPOOL_CL: - status = spool_reqbody_cl(p, r, p_conn, origin, header_brigade); - break; - default: - ap_assert(1 != 1); - } - - return status; -} - static apr_status_t ap_proxy_http_request(apr_pool_t *p, request_rec *r, proxy_http_conn_t *p_conn, conn_rec *origin, @@ -870,9 +396,8 @@ apr_status_t ap_proxy_http_request(apr_pool_t *p, request_rec *r, apr_bucket *e; const apr_array_header_t *headers_in_array; const apr_table_entry_t *headers_in; - int counter; + int counter, seen_eos; apr_status_t status; - int force10; /* * Send the HTTP/1.1 request to the remote server @@ -899,10 +424,8 @@ apr_status_t ap_proxy_http_request(apr_pool_t *p, request_rec *r, if ( apr_table_get(r->subprocess_env,"force-proxy-request-1.0")) { buf = apr_pstrcat(p, r->method, " ", url, " HTTP/1.0" CRLF, NULL); - force10 = 1; } else { buf = apr_pstrcat(p, r->method, " ", url, " HTTP/1.1" CRLF, NULL); - force10 = 0; } if ( apr_table_get(r->subprocess_env,"proxy-nokeepalive")) { apr_table_unset(r->headers_in, "Connection"); @@ -1024,10 +547,6 @@ apr_status_t ap_proxy_http_request(apr_pool_t *p, request_rec *r, || !apr_strnatcasecmp(headers_in[counter].key, "Transfer-Encoding") || !apr_strnatcasecmp(headers_in[counter].key, "Upgrade") - /* We'll add appropriate Content-Length later, if appropriate. - */ - || !apr_strnatcasecmp(headers_in[counter].key, "Content-Length") - /* XXX: @@@ FIXME: "Proxy-Authorization" should *only* be * suppressed if THIS server requested the authentication, * not when a frontend proxy requested it! @@ -1080,8 +599,18 @@ apr_status_t ap_proxy_http_request(apr_pool_t *p, request_rec *r, APR_BRIGADE_INSERT_TAIL(bb, e); } - /* send the request data, if any. */ - status = send_request_body(p, r, p_conn, origin, bb, force10); + /* add empty line at the end of the headers */ +#if APR_CHARSET_EBCDIC + e = apr_bucket_immortal_create("\015\012", 2, c->bucket_alloc); +#else + e = apr_bucket_immortal_create(CRLF, sizeof(CRLF)-1, c->bucket_alloc); +#endif + APR_BRIGADE_INSERT_TAIL(bb, e); + e = apr_bucket_flush_create(c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(bb, e); + + status = ap_pass_brigade(origin->output_filters, bb); + if (status != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_ERR, status, r->server, "proxy: request failed to %pI (%s)", @@ -1089,6 +618,44 @@ apr_status_t ap_proxy_http_request(apr_pool_t *p, request_rec *r, return status; } + /* send the request data, if any. */ + seen_eos = 0; + do { + status = ap_get_brigade(r->input_filters, bb, AP_MODE_READBYTES, + APR_BLOCK_READ, HUGE_STRING_LEN); + + if (status != APR_SUCCESS) { + return status; + } + + /* If this brigade contain EOS, either stop or remove it. */ + if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) { + /* As a shortcut, if this brigade is simply an EOS bucket, + * don't send anything down the filter chain. + */ + if (APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(bb))) { + break; + } + + /* We can't pass this EOS to the output_filters. */ + e = APR_BRIGADE_LAST(bb); + apr_bucket_delete(e); + seen_eos = 1; + } + + e = apr_bucket_flush_create(c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(bb, e); + + status = ap_pass_brigade(origin->output_filters, bb); + if (status != APR_SUCCESS) { + ap_log_error(APLOG_MARK, APLOG_ERR, status, r->server, + "proxy: pass request data failed to %pI (%s)", + p_conn->addr, p_conn->name); + return status; + } + apr_brigade_cleanup(bb); + } while (!seen_eos); + return APR_SUCCESS; }