Thanks for your comments.
Commited in r1588527.
On Fri, Apr 4, 2014 at 11:39 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
> On Fri, Apr 4, 2014 at 8:38 PM, Ruediger Pluem <rpl...@apache.org> wrote:
>> Why can't we fix that directly in ap_proxy_create_hdrbrgd?
>
> Actually we can, and that's indeed a much simpler patch.
>
> I was worried about modifications of "Content-Length" and/or
> "Transfer-Encoding" outside ap_proxy_create_hdrbrgd() (ie. in
> ap_proxy_http_request() itself) that would be needed by
> ap_http_filter(), but in fact all is good now regarding interactions
> between the two (recent draft-ietf-httpbis-p1-messaging-23 conformance
> commits/backport).
>
> Thus, we can simply not touch headers_in in ap_proxy_http_request(),
> simply resetting old_cl_val and old_te_val when needed.
>
>
> Thanks for your feedback, below is the corresponding patch (nearly
> back to the original patch of this thread).
>
> Regards,
> Yann.
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1584652)
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -3203,7 +3203,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> char *buf;
> const apr_array_header_t *headers_in_array;
> const apr_table_entry_t *headers_in;
> - apr_table_t *headers_in_copy;
> + apr_table_t *saved_headers_in;
> apr_bucket *e;
> int do_100_continue;
> conn_rec *origin = p_conn->connection;
> @@ -3279,6 +3279,21 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(header_brigade, e);
>
> + /*
> + * Save the original headers in here and restore them when leaving, since
> + * we will apply proxy purpose only modifications (eg. clearing
> hop-by-hop
> + * headers, add Via or X-Forwarded-* or Expect...), whereas the originals
> + * will be needed later to prepare the correct response and logging.
> + *
> + * Note: We need to take r->pool for apr_table_copy as the key / value
> + * pairs in r->headers_in have been created out of r->pool and
> + * p might be (and actually is) a longer living pool.
> + * This would trigger the bad pool ancestry abort in apr_table_copy if
> + * apr is compiled with APR_POOL_DEBUG.
> + */
> + saved_headers_in = r->headers_in;
> + r->headers_in = apr_table_copy(r->pool, saved_headers_in);
> +
> /* handle Via */
> if (conf->viaopt == via_block) {
> /* Block all outgoing Via: headers */
> @@ -3363,21 +3378,10 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> }
>
> proxy_run_fixups(r);
> - /*
> - * Make a copy of the headers_in table before clearing the connection
> - * headers as we need the connection headers later in the http output
> - * filter to prepare the correct response headers.
> - *
> - * Note: We need to take r->pool for apr_table_copy as the key / value
> - * pairs in r->headers_in have been created out of r->pool and
> - * p might be (and actually is) a longer living pool.
> - * This would trigger the bad pool ancestry abort in apr_table_copy if
> - * apr is compiled with APR_POOL_DEBUG.
> - */
> - headers_in_copy = apr_table_copy(r->pool, r->headers_in);
> - ap_proxy_clear_connection(r, headers_in_copy);
> + ap_proxy_clear_connection(r, r->headers_in);
> +
> /* send request headers */
> - headers_in_array = apr_table_elts(headers_in_copy);
> + headers_in_array = apr_table_elts(r->headers_in);
> headers_in = (const apr_table_entry_t *) headers_in_array->elts;
> for (counter = 0; counter < headers_in_array->nelts; counter++) {
> if (headers_in[counter].key == NULL
> @@ -3439,6 +3443,11 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(header_brigade, e);
> }
> +
> + /* Restore the original headers in (see comment above),
> + * we won't modify them anymore.
> + */
> + r->headers_in = saved_headers_in;
> return OK;
> }
>
> Index: modules/proxy/mod_proxy_http.c
> ===================================================================
> --- modules/proxy/mod_proxy_http.c (revision 1584652)
> +++ modules/proxy/mod_proxy_http.c (working copy)
> @@ -750,14 +750,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
> if (!r->kept_body && r->main) {
> /* XXX: Why DON'T sub-requests use keepalives? */
> p_conn->close = 1;
> - if (old_cl_val) {
> - old_cl_val = NULL;
> - apr_table_unset(r->headers_in, "Content-Length");
> - }
> - if (old_te_val) {
> - old_te_val = NULL;
> - apr_table_unset(r->headers_in, "Transfer-Encoding");
> - }
> + old_cl_val = NULL;
> + old_te_val = NULL;
> rb_method = RB_STREAM_CL;
> e = apr_bucket_eos_create(input_brigade->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(input_brigade, e);
> @@ -783,7 +777,6 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
> "client %s (%s) requested Transfer-Encoding "
> "chunked body with Content-Length (C-L ignored)",
> c->client_ip, c->remote_host ? c->remote_host: "");
> - apr_table_unset(r->headers_in, "Content-Length");
> old_cl_val = NULL;
> origin->keepalive = AP_CONN_CLOSE;
> p_conn->close = 1;
