Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: domain of ylavic.dev@gmail.com
 designates 209.85.213.172 as permitted sender)
MIME-Version: 1.0
Date: Mon, 13 Jan 2014 15:30:35 +0100
Message-ID: 
 <CAKQ1sVNfB8Yrqi387vi0do2xW0P9rpOEn8iChKa-e9muwMyVHw@mail.gmail.com>
Subject: mod_proxy duplicated its headers on next balancer's worker or
 100-continue ping retries
From: Yann Ylavic <ylavic.dev@gmail.com>
To: httpd <dev@httpd.apache.org>
Content-Type: multipart/mixed; boundary=e89a8f3ba17d3f5e2304efdaebbf

--e89a8f3ba17d3f5e2304efdaebbf
Content-Type: text/plain; charset=ISO-8859-1

Hi,

when mod_proxy(_http) has to forward the same request multiple times
(next balancer's worker / 100-continue ping), it duplicates
(re-merges) the same Via and X-Forwarded-* values as many times.

This is because ap_proxy_create_hdrbrgd() works directly on
r->headers_in before constructing the headers brigade to be sent, and
this function is called for each try.

The patch below addresses this by moving the (existing) table copy
before modifying the headers, but by doing so, any log format which
currently rely on these headers containing mod_proxy's merged values
will not work anymore.

The question is, should "%{X-Forwarded-*|Via}i" contain mod_proxy's
values or not?
My feeling is that it should not, but working things should not be
broken either.
(there is PR 45387 about this (from 2008-07-12), but still NEW with no
answer...).

If the response is yes (and duplication is a real issue), I could
propose another patch that still include the proxy related headers in
the client's r->headers_in, but only once.

Please let me know,
regards,
Yann.

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 1557687)
+++ modules/proxy/proxy_util.c    (working copy)
@@ -3200,10 +3200,18 @@ 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);

+    /* Make a copy of the to-be-modified headers_in table as we need the
+     * original one later for logging, and to prepare the correct response
+     * headers in the http output filter (eg. hop-by-hop headers), or else
+     * to reenter with "fresh" headers should the same request be sent
+     * multiple times (balancer's current worker or http ping failures).
+     */
+    headers_in_copy = apr_table_copy(r->pool, r->headers_in);
+
     /* handle Via */
     if (conf->viaopt == via_block) {
         /* Block all outgoing Via: headers */
-        apr_table_unset(r->headers_in, "Via");
+        apr_table_unset(headers_in_copy, "Via");
     } else if (conf->viaopt != via_off) {
         const char *server_name = ap_get_server_name(r);
         /* If USE_CANONICAL_NAME_OFF was configured for the proxy virtual host,
@@ -3215,7 +3223,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
             server_name = r->server->server_hostname;
         /* Create a "Via:" request header entry and merge it */
         /* Generate outgoing Via: header with/without server comment: */
-        apr_table_mergen(r->headers_in, "Via",
+        apr_table_mergen(headers_in_copy, "Via",
                          (conf->viaopt == via_full)
                          ? apr_psprintf(p, "%d.%d %s%s (%s)",
                                         HTTP_VERSION_MAJOR(r->proto_num),
@@ -3233,7 +3241,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
      * to backend
      */
     if (do_100_continue) {
-        apr_table_mergen(r->headers_in, "Expect", "100-Continue");
+        apr_table_mergen(headers_in_copy, "Expect", "100-Continue");
         r->expecting_100 = 1;
     }

@@ -3264,38 +3272,35 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
             /* Add X-Forwarded-For: so that the upstream has a chance to
              * determine, where the original request came from.
              */
-            apr_table_mergen(r->headers_in, "X-Forwarded-For",
+            apr_table_mergen(headers_in_copy, "X-Forwarded-For",
                              r->useragent_ip);

             /* Add X-Forwarded-Host: so that upstream knows what the
              * original request hostname was.
              */
-            if ((buf = apr_table_get(r->headers_in, "Host"))) {
-                apr_table_mergen(r->headers_in, "X-Forwarded-Host", buf);
+            if ((buf = apr_table_get(headers_in_copy, "Host"))) {
+                apr_table_mergen(headers_in_copy, "X-Forwarded-Host", buf);
             }

             /* Add X-Forwarded-Server: so that upstream knows what the
              * name of this proxy server is (if there are more than one)
              * XXX: This duplicates Via: - do we strictly need it?
              */
-            apr_table_mergen(r->headers_in, "X-Forwarded-Server",
+            apr_table_mergen(headers_in_copy, "X-Forwarded-Server",
                              r->server->server_hostname);
         }
     }

-    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.
+    /* Temporarily swap headers_in pointers for fixups hooks to work
+     * with/on the new headers (while still preserving r->headers_in).
      */
-    headers_in_copy = apr_table_copy(r->pool, r->headers_in);
+    {
+        apr_table_t *t = r->headers_in;
+        r->headers_in = headers_in_copy;
+        proxy_run_fixups(r);
+        r->headers_in = t;
+    }
+
     ap_proxy_clear_connection(r, headers_in_copy);
     /* send request headers */
     headers_in_array = apr_table_elts(headers_in_copy);
[EOS]

--e89a8f3ba17d3f5e2304efdaebbf
Content-Type: text/x-diff; charset=US-ASCII;
	name="httpd-trunk-mod_proxy_preserve_headers_in.patch"
Content-Disposition: attachment;
	filename="httpd-trunk-mod_proxy_preserve_headers_in.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_hqdtp1670

SW5kZXg6IG1vZHVsZXMvcHJveHkvcHJveHlfdXRpbC5jCj09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIG1vZHVsZXMv
cHJveHkvcHJveHlfdXRpbC5jCShyZXZpc2lvbiAxNTU3Njg3KQorKysgbW9kdWxlcy9wcm94eS9w
cm94eV91dGlsLmMJKHdvcmtpbmcgY29weSkKQEAgLTMyMDAsMTAgKzMyMDAsMTggQEAgUFJPWFlf
REVDTEFSRShpbnQpIGFwX3Byb3h5X2NyZWF0ZV9oZHJicmdkKGFwcl9wb28KICAgICBlID0gYXBy
X2J1Y2tldF9wb29sX2NyZWF0ZShidWYsIHN0cmxlbihidWYpLCBwLCBjLT5idWNrZXRfYWxsb2Mp
OwogICAgIEFQUl9CUklHQURFX0lOU0VSVF9UQUlMKGhlYWRlcl9icmlnYWRlLCBlKTsKIAorICAg
IC8qIE1ha2UgYSBjb3B5IG9mIHRoZSB0by1iZS1tb2RpZmllZCBoZWFkZXJzX2luIHRhYmxlIGFz
IHdlIG5lZWQgdGhlCisgICAgICogb3JpZ2luYWwgb25lIGxhdGVyIGZvciBsb2dnaW5nLCBhbmQg
dG8gcHJlcGFyZSB0aGUgY29ycmVjdCByZXNwb25zZQorICAgICAqIGhlYWRlcnMgaW4gdGhlIGh0
dHAgb3V0cHV0IGZpbHRlciAoZWcuIGhvcC1ieS1ob3AgaGVhZGVycyksIG9yIGVsc2UKKyAgICAg
KiB0byByZWVudGVyIHdpdGggImZyZXNoIiBoZWFkZXJzIHNob3VsZCB0aGUgc2FtZSByZXF1ZXN0
IGJlIHNlbnQKKyAgICAgKiBtdWx0aXBsZSB0aW1lcyAoYmFsYW5jZXIncyBjdXJyZW50IHdvcmtl
ciBvciBodHRwIHBpbmcgZmFpbHVyZXMpLgorICAgICAqLworICAgIGhlYWRlcnNfaW5fY29weSA9
IGFwcl90YWJsZV9jb3B5KHItPnBvb2wsIHItPmhlYWRlcnNfaW4pOworCiAgICAgLyogaGFuZGxl
IFZpYSAqLwogICAgIGlmIChjb25mLT52aWFvcHQgPT0gdmlhX2Jsb2NrKSB7CiAgICAgICAgIC8q
IEJsb2NrIGFsbCBvdXRnb2luZyBWaWE6IGhlYWRlcnMgKi8KLSAgICAgICAgYXByX3RhYmxlX3Vu
c2V0KHItPmhlYWRlcnNfaW4sICJWaWEiKTsKKyAgICAgICAgYXByX3RhYmxlX3Vuc2V0KGhlYWRl
cnNfaW5fY29weSwgIlZpYSIpOwogICAgIH0gZWxzZSBpZiAoY29uZi0+dmlhb3B0ICE9IHZpYV9v
ZmYpIHsKICAgICAgICAgY29uc3QgY2hhciAqc2VydmVyX25hbWUgPSBhcF9nZXRfc2VydmVyX25h
bWUocik7CiAgICAgICAgIC8qIElmIFVTRV9DQU5PTklDQUxfTkFNRV9PRkYgd2FzIGNvbmZpZ3Vy
ZWQgZm9yIHRoZSBwcm94eSB2aXJ0dWFsIGhvc3QsCkBAIC0zMjE1LDcgKzMyMjMsNyBAQCBQUk9Y
WV9ERUNMQVJFKGludCkgYXBfcHJveHlfY3JlYXRlX2hkcmJyZ2QoYXByX3BvbwogICAgICAgICAg
ICAgc2VydmVyX25hbWUgPSByLT5zZXJ2ZXItPnNlcnZlcl9ob3N0bmFtZTsKICAgICAgICAgLyog
Q3JlYXRlIGEgIlZpYToiIHJlcXVlc3QgaGVhZGVyIGVudHJ5IGFuZCBtZXJnZSBpdCAqLwogICAg
ICAgICAvKiBHZW5lcmF0ZSBvdXRnb2luZyBWaWE6IGhlYWRlciB3aXRoL3dpdGhvdXQgc2VydmVy
IGNvbW1lbnQ6ICovCi0gICAgICAgIGFwcl90YWJsZV9tZXJnZW4oci0+aGVhZGVyc19pbiwgIlZp
YSIsCisgICAgICAgIGFwcl90YWJsZV9tZXJnZW4oaGVhZGVyc19pbl9jb3B5LCAiVmlhIiwKICAg
ICAgICAgICAgICAgICAgICAgICAgICAoY29uZi0+dmlhb3B0ID09IHZpYV9mdWxsKQogICAgICAg
ICAgICAgICAgICAgICAgICAgID8gYXByX3BzcHJpbnRmKHAsICIlZC4lZCAlcyVzICglcykiLAog
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIEhUVFBfVkVSU0lPTl9NQUpP
UihyLT5wcm90b19udW0pLApAQCAtMzIzMyw3ICszMjQxLDcgQEAgUFJPWFlfREVDTEFSRShpbnQp
IGFwX3Byb3h5X2NyZWF0ZV9oZHJicmdkKGFwcl9wb28KICAgICAgKiB0byBiYWNrZW5kCiAgICAg
ICovCiAgICAgaWYgKGRvXzEwMF9jb250aW51ZSkgewotICAgICAgICBhcHJfdGFibGVfbWVyZ2Vu
KHItPmhlYWRlcnNfaW4sICJFeHBlY3QiLCAiMTAwLUNvbnRpbnVlIik7CisgICAgICAgIGFwcl90
YWJsZV9tZXJnZW4oaGVhZGVyc19pbl9jb3B5LCAiRXhwZWN0IiwgIjEwMC1Db250aW51ZSIpOwog
ICAgICAgICByLT5leHBlY3RpbmdfMTAwID0gMTsKICAgICB9CiAKQEAgLTMyNjQsMzggKzMyNzIs
MzUgQEAgUFJPWFlfREVDTEFSRShpbnQpIGFwX3Byb3h5X2NyZWF0ZV9oZHJicmdkKGFwcl9wb28K
ICAgICAgICAgICAgIC8qIEFkZCBYLUZvcndhcmRlZC1Gb3I6IHNvIHRoYXQgdGhlIHVwc3RyZWFt
IGhhcyBhIGNoYW5jZSB0bwogICAgICAgICAgICAgICogZGV0ZXJtaW5lLCB3aGVyZSB0aGUgb3Jp
Z2luYWwgcmVxdWVzdCBjYW1lIGZyb20uCiAgICAgICAgICAgICAgKi8KLSAgICAgICAgICAgIGFw
cl90YWJsZV9tZXJnZW4oci0+aGVhZGVyc19pbiwgIlgtRm9yd2FyZGVkLUZvciIsCisgICAgICAg
ICAgICBhcHJfdGFibGVfbWVyZ2VuKGhlYWRlcnNfaW5fY29weSwgIlgtRm9yd2FyZGVkLUZvciIs
CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHItPnVzZXJhZ2VudF9pcCk7CiAKICAgICAg
ICAgICAgIC8qIEFkZCBYLUZvcndhcmRlZC1Ib3N0OiBzbyB0aGF0IHVwc3RyZWFtIGtub3dzIHdo
YXQgdGhlCiAgICAgICAgICAgICAgKiBvcmlnaW5hbCByZXF1ZXN0IGhvc3RuYW1lIHdhcy4KICAg
ICAgICAgICAgICAqLwotICAgICAgICAgICAgaWYgKChidWYgPSBhcHJfdGFibGVfZ2V0KHItPmhl
YWRlcnNfaW4sICJIb3N0IikpKSB7Ci0gICAgICAgICAgICAgICAgYXByX3RhYmxlX21lcmdlbihy
LT5oZWFkZXJzX2luLCAiWC1Gb3J3YXJkZWQtSG9zdCIsIGJ1Zik7CisgICAgICAgICAgICBpZiAo
KGJ1ZiA9IGFwcl90YWJsZV9nZXQoaGVhZGVyc19pbl9jb3B5LCAiSG9zdCIpKSkgeworICAgICAg
ICAgICAgICAgIGFwcl90YWJsZV9tZXJnZW4oaGVhZGVyc19pbl9jb3B5LCAiWC1Gb3J3YXJkZWQt
SG9zdCIsIGJ1Zik7CiAgICAgICAgICAgICB9CiAKICAgICAgICAgICAgIC8qIEFkZCBYLUZvcndh
cmRlZC1TZXJ2ZXI6IHNvIHRoYXQgdXBzdHJlYW0ga25vd3Mgd2hhdCB0aGUKICAgICAgICAgICAg
ICAqIG5hbWUgb2YgdGhpcyBwcm94eSBzZXJ2ZXIgaXMgKGlmIHRoZXJlIGFyZSBtb3JlIHRoYW4g
b25lKQogICAgICAgICAgICAgICogWFhYOiBUaGlzIGR1cGxpY2F0ZXMgVmlhOiAtIGRvIHdlIHN0
cmljdGx5IG5lZWQgaXQ/CiAgICAgICAgICAgICAgKi8KLSAgICAgICAgICAgIGFwcl90YWJsZV9t
ZXJnZW4oci0+aGVhZGVyc19pbiwgIlgtRm9yd2FyZGVkLVNlcnZlciIsCisgICAgICAgICAgICBh
cHJfdGFibGVfbWVyZ2VuKGhlYWRlcnNfaW5fY29weSwgIlgtRm9yd2FyZGVkLVNlcnZlciIsCiAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIHItPnNlcnZlci0+c2VydmVyX2hvc3RuYW1lKTsK
ICAgICAgICAgfQogICAgIH0KIAotICAgIHByb3h5X3J1bl9maXh1cHMocik7Ci0gICAgLyoKLSAg
ICAgKiBNYWtlIGEgY29weSBvZiB0aGUgaGVhZGVyc19pbiB0YWJsZSBiZWZvcmUgY2xlYXJpbmcg
dGhlIGNvbm5lY3Rpb24KLSAgICAgKiBoZWFkZXJzIGFzIHdlIG5lZWQgdGhlIGNvbm5lY3Rpb24g
aGVhZGVycyBsYXRlciBpbiB0aGUgaHR0cCBvdXRwdXQKLSAgICAgKiBmaWx0ZXIgdG8gcHJlcGFy
ZSB0aGUgY29ycmVjdCByZXNwb25zZSBoZWFkZXJzLgotICAgICAqCi0gICAgICogTm90ZTogV2Ug
bmVlZCB0byB0YWtlIHItPnBvb2wgZm9yIGFwcl90YWJsZV9jb3B5IGFzIHRoZSBrZXkgLyB2YWx1
ZQotICAgICAqIHBhaXJzIGluIHItPmhlYWRlcnNfaW4gaGF2ZSBiZWVuIGNyZWF0ZWQgb3V0IG9m
IHItPnBvb2wgYW5kCi0gICAgICogcCBtaWdodCBiZSAoYW5kIGFjdHVhbGx5IGlzKSBhIGxvbmdl
ciBsaXZpbmcgcG9vbC4KLSAgICAgKiBUaGlzIHdvdWxkIHRyaWdnZXIgdGhlIGJhZCBwb29sIGFu
Y2VzdHJ5IGFib3J0IGluIGFwcl90YWJsZV9jb3B5IGlmCi0gICAgICogYXByIGlzIGNvbXBpbGVk
IHdpdGggQVBSX1BPT0xfREVCVUcuCisgICAgLyogVGVtcG9yYXJpbHkgc3dhcCBoZWFkZXJzX2lu
IHBvaW50ZXJzIGZvciBmaXh1cHMgaG9va3MgdG8gd29yaworICAgICAqIHdpdGgvb24gdGhlIG5l
dyBoZWFkZXJzICh3aGlsZSBzdGlsbCBwcmVzZXJ2aW5nIHItPmhlYWRlcnNfaW4pLgogICAgICAq
LwotICAgIGhlYWRlcnNfaW5fY29weSA9IGFwcl90YWJsZV9jb3B5KHItPnBvb2wsIHItPmhlYWRl
cnNfaW4pOworICAgIHsKKyAgICAgICAgYXByX3RhYmxlX3QgKnQgPSByLT5oZWFkZXJzX2luOwor
ICAgICAgICByLT5oZWFkZXJzX2luID0gaGVhZGVyc19pbl9jb3B5OworICAgICAgICBwcm94eV9y
dW5fZml4dXBzKHIpOworICAgICAgICByLT5oZWFkZXJzX2luID0gdDsKKyAgICB9CisKICAgICBh
cF9wcm94eV9jbGVhcl9jb25uZWN0aW9uKHIsIGhlYWRlcnNfaW5fY29weSk7CiAgICAgLyogc2Vu
ZCByZXF1ZXN0IGhlYWRlcnMgKi8KICAgICBoZWFkZXJzX2luX2FycmF5ID0gYXByX3RhYmxlX2Vs
dHMoaGVhZGVyc19pbl9jb3B5KTsK
--e89a8f3ba17d3f5e2304efdaebbf--