You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/04/04 17:43:52 UTC
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Helo,
I revive this thread since headers in modfied by mod_proxy seems wrong
to me, I have to take that into account when, say, analysing access
logs (received X-Forwarded-* or Via headers vs the ones added by
mod_proxy, see also PR 45387), or as said in the title, which is even
worse when backend (recoverable) failures occur in load-balancing
(same headers added multiple times).
I ended up with the following patch that copies r->headers_in before
they are modified and restore them after they have been forwarded
(mod_proxy_http and mod_proxy_wstunnel only seem to be concerned, but
I may be missing something).
Feedbacks welcome, if no one objects I'll commit it to trunk.
Regards,
Yann.
Index: modules/proxy/mod_proxy_wstunnel.c
===================================================================
--- modules/proxy/mod_proxy_wstunnel.c (revision 1584652)
+++ modules/proxy/mod_proxy_wstunnel.c (working copy)
@@ -320,15 +320,26 @@ static int ap_proxy_wstunnel_request(apr_pool_t *p
apr_socket_t *client_socket = ap_get_conn_socket(c);
ws_baton_t *baton = apr_pcalloc(r->pool, sizeof(ws_baton_t));
apr_socket_t *sockets[3] = {NULL, NULL, NULL};
+ apr_table_t *saved_headers_in;
int status;
header_brigade = apr_brigade_create(p, backconn->bucket_alloc);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending request");
+ /*
+ * Save the original headers in before ap_proxy_create_hdrbrgd() and
+ * restore them after, since it will apply proxy purpose only modifications
+ * (eg. cleanup hop-by-hop headers, add Via or X-Forwarded-*...), whereas
+ * the original headers may be needed later to prepare the correct response
+ * or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
+ r->headers_in = saved_headers_in;
if (rv != OK) {
return rv;
}
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1584652)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -717,24 +717,31 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
apr_off_t bytes;
int force10, rv;
conn_rec *origin = p_conn->connection;
+ apr_table_t *saved_headers_in;
- if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
- if (r->expecting_100) {
- return HTTP_EXPECTATION_FAILED;
- }
- force10 = 1;
- } else {
- force10 = 0;
- }
+ /*
+ * Save the original headers in here and restore them when leaving, since
+ * we will apply proxy purpose only modifications (eg. cleanup hop-by-hop
+ * headers, add Via or X-Forwarded-* or Expect...), whereas the original
+ * headers may be needed later to prepare the correct response or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
header_brigade = apr_brigade_create(p, bucket_alloc);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
if (rv != OK) {
- return rv;
+ goto cleanup;
}
+ if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
+ force10 = 1;
+ } else {
+ force10 = 0;
+ }
+
/* We have headers, let's figure out our request body... */
input_brigade = apr_brigade_create(p, bucket_alloc);
@@ -775,7 +782,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
if (old_te_val && strcasecmp(old_te_val, "chunked") != 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01093)
"%s Transfer-Encoding is not supported", old_te_val);
- return HTTP_INTERNAL_SERVER_ERROR;
+ rv = HTTP_INTERNAL_SERVER_ERROR;
+ goto cleanup;
}
if (old_cl_val && old_te_val) {
@@ -808,7 +816,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
- return HTTP_BAD_REQUEST;
+ rv = HTTP_BAD_REQUEST;
+ goto cleanup;
}
apr_brigade_length(temp_brigade, 1, &bytes);
@@ -830,7 +839,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" to %pI (%s) from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
- return HTTP_INTERNAL_SERVER_ERROR;
+ rv = HTTP_INTERNAL_SERVER_ERROR;
+ goto cleanup;
}
/* Ensure we don't hit a wall where we have a buffer too small
@@ -969,10 +979,13 @@ skip_body:
"pass request body failed to %pI (%s) from %s (%s)",
p_conn->addr, p_conn->hostname ? p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
- return rv;
}
-
- return OK;
+cleanup:
+ /* Restore the original headers in (see comment above),
+ * we won't modify them anymore.
+ */
+ r->headers_in = saved_headers_in;
+ return rv;
}
/*
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1584652)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3203,7 +3203,6 @@ 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_bucket *e;
int do_100_continue;
conn_rec *origin = p_conn->connection;
@@ -3363,21 +3362,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
[EOS]
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
Thanks for your comments.
Commited in r1588527.
On Fri, Apr 4, 2014 at 11:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Apr 4, 2014 at 8:38 PM, Ruediger Pluem <rp...@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;
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 4, 2014 at 8:38 PM, Ruediger Pluem <rp...@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;
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Ruediger Pluem <rp...@apache.org>.
Why can't we fix that directly in ap_proxy_create_hdrbrgd?
Regards
Rüdiger
Yann Ylavic wrote:
> Here is the patch not polluted by working collisions or gotos :
>
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
Here is the patch not polluted by working collisions or gotos :
Index: modules/proxy/mod_proxy_wstunnel.c
===================================================================
--- modules/proxy/mod_proxy_wstunnel.c (revision 1584652)
+++ modules/proxy/mod_proxy_wstunnel.c (working copy)
@@ -320,15 +320,26 @@ static int ap_proxy_wstunnel_request(apr_pool_t *p
apr_socket_t *client_socket = ap_get_conn_socket(c);
ws_baton_t *baton = apr_pcalloc(r->pool, sizeof(ws_baton_t));
apr_socket_t *sockets[3] = {NULL, NULL, NULL};
+ apr_table_t *saved_headers_in;
int status;
header_brigade = apr_brigade_create(p, backconn->bucket_alloc);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending request");
+ /*
+ * Save the original headers in before ap_proxy_create_hdrbrgd() and
+ * restore them after, since it will apply proxy purpose only modifications
+ * (eg. cleanup hop-by-hop headers, add Via or X-Forwarded-*...), whereas
+ * the original headers may be needed later to prepare the correct response
+ * or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
+ r->headers_in = saved_headers_in;
if (rv != OK) {
return rv;
}
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1584652)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -717,6 +717,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
apr_off_t bytes;
int force10, rv;
conn_rec *origin = p_conn->connection;
+ apr_table_t *saved_headers_in;
if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
if (r->expecting_100) {
@@ -727,11 +728,21 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
force10 = 0;
}
+ /*
+ * Save the original headers in here and restore them when leaving, since
+ * we will apply proxy purpose only modifications (eg. cleanup hop-by-hop
+ * headers, add Via or X-Forwarded-* or Expect...), whereas the original
+ * headers may be needed later to prepare the correct response or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
+
header_brigade = apr_brigade_create(p, bucket_alloc);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
if (rv != OK) {
+ r->headers_in = saved_headers_in;
return rv;
}
@@ -775,6 +786,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
if (old_te_val && strcasecmp(old_te_val, "chunked") != 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01093)
"%s Transfer-Encoding is not supported", old_te_val);
+ r->headers_in = saved_headers_in;
return HTTP_INTERNAL_SERVER_ERROR;
}
@@ -808,6 +820,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
+ r->headers_in = saved_headers_in;
return HTTP_BAD_REQUEST;
}
@@ -830,6 +843,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" to %pI (%s) from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
+ r->headers_in = saved_headers_in;
return HTTP_INTERNAL_SERVER_ERROR;
}
@@ -963,6 +977,11 @@ skip_body:
break;
}
+ /* Restore the original headers in (see comment above),
+ * we won't modify them anymore.
+ */
+ r->headers_in = saved_headers_in;
+
if (rv != OK) {
/* apr_status_t value has been logged in lower level method */
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01097)
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1584652)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3203,7 +3203,6 @@ 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_bucket *e;
int do_100_continue;
conn_rec *origin = p_conn->connection;
@@ -3363,21 +3362,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
[EOS]
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 4, 2014 at 6:22 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> This seems to change some logic which appear only tangentially
> associated w/ the "save headers" issue... why is that?
Do you mean, ...
>
> On Apr 4, 2014, at 11:43 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> Index: modules/proxy/mod_proxy_http.c
>> ===================================================================
>> --- modules/proxy/mod_proxy_http.c (revision 1584652)
>> +++ modules/proxy/mod_proxy_http.c (working copy)
>> @@ -717,24 +717,31 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
>> apr_off_t bytes;
>> int force10, rv;
>> conn_rec *origin = p_conn->connection;
>> + apr_table_t *saved_headers_in;
>>
>> - if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
>> - if (r->expecting_100) {
>> - return HTTP_EXPECTATION_FAILED;
>> - }
>> - force10 = 1;
>> - } else {
>> - force10 = 0;
>> - }
... this change?
Yes, sorry about it, it is part of another patch I'm working on
regarding 100-continue forwarding issue.
Please ignore this change, it is harmless (since
ap_proxy_create_hdrbrgd() already checks the expectation on HTTP/1.0),
but really not related to this thread.
>> +cleanup:
>> + /* Restore the original headers in (see comment above),
>> + * we won't modify them anymore.
>> + */
>> + r->headers_in = saved_headers_in;
>> + return rv;
>> }
>>
>
> Uggg... I hate gotos. We only use them sparingly and when
> breaking out of nasty, nasty code. Here, we are just doing so
> to safe some cut/paste... -0
>
I think I can avoid that, and don't like them either :p
This was to keep the patch simple for review (I can see gotos used not
far from here, and took the easy way).
Re: mod_proxy duplicated its headers on next balancer's worker or 100-continue ping retries
Posted by Jim Jagielski <ji...@jaguNET.com>.
This seems to change some logic which appear only tangentially
associated w/ the "save headers" issue... why is that?
On Apr 4, 2014, at 11:43 AM, Yann Ylavic <yl...@gmail.com> wrote:
> Index: modules/proxy/mod_proxy_http.c
> ===================================================================
> --- modules/proxy/mod_proxy_http.c (revision 1584652)
> +++ modules/proxy/mod_proxy_http.c (working copy)
> @@ -717,24 +717,31 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
> apr_off_t bytes;
> int force10, rv;
> conn_rec *origin = p_conn->connection;
> + apr_table_t *saved_headers_in;
>
> - if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
> - if (r->expecting_100) {
> - return HTTP_EXPECTATION_FAILED;
> - }
> - force10 = 1;
> - } else {
> - force10 = 0;
> - }
> + /*
> + * Save the original headers in here and restore them when leaving, since
> + * we will apply proxy purpose only modifications (eg. cleanup hop-by-hop
> + * headers, add Via or X-Forwarded-* or Expect...), whereas the original
> + * headers may be needed later to prepare the correct response or logging.
> + */
> + saved_headers_in = r->headers_in;
> + r->headers_in = apr_table_copy(r->pool, saved_headers_in);
>
> header_brigade = apr_brigade_create(p, bucket_alloc);
> rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
> worker, conf, uri, url, server_portstr,
> &old_cl_val, &old_te_val);
> if (rv != OK) {
> - return rv;
> + goto cleanup;
> +cleanup:
> + /* Restore the original headers in (see comment above),
> + * we won't modify them anymore.
> + */
> + r->headers_in = saved_headers_in;
> + return rv;
> }
>
Uggg... I hate gotos. We only use them sparingly and when
breaking out of nasty, nasty code. Here, we are just doing so
to safe some cut/paste... -0