httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject mod_proxy duplicated its headers on next balancer's worker or 100-continue ping retries
Date Mon, 13 Jan 2014 14:30:35 GMT
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]

Mime
View raw message