mod_proxy: Ignore (and warn about) enablereuse=on for ProxyPassMatch when

           some dollar substitution (backreference) happens in the hostname
           or port part of the URL.

Address or connection reuse can't work when the autority part of the URL is
dynamic (single origin server[:port] handled/assumed in the reslist). Detect
such cases and unset worker->s->is_address_reusable to disable reuse regardless
of enablereuse/disablereuse.

* modules/proxy/proxy_util.c(ap_proxy_define_worker_ex):
  Lookup for $n substitution in the hostname[:port] when parsing the URL and
  if present, set worker->->is_address_reusable=0 / worker->s->disablereuse=1.

* modules/proxy/proxy_util.c(ap_proxy_initialize_worker):
  Don't overwrite worker->s->is_address_reusable from enablereuse/disablereuse
  parameters, and set both consistently.

* docs/manual/mod/mod_proxy.xml:
  Add ProxyPassMatch compatibility note about key=value parameters handled with
  $n substitutions since 2.4.47.
  Document the specificities of enablereuse/disablereuse w.r.t. $n subsitutions
  in the different part of the URL.
  Axe the note about unparsable URLs when the $n substitution happens in the
  port, this has been addressed in 2.4.47 too (and works now).



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1904513 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 71 insertions, 33 deletions

diff --git a/changes-entries/enablereuse.txt b/changes-entries/enablereuse.txt
new file mode 100644
index 0000000000..b8e1a3f617
--- /dev/null
+++ b/changes-entries/enablereuse.txt
@@ -0,0 +1,3 @@
+  *) mod_proxy: Ignore (and warn about) enablereuse=on for ProxyPassMatch when
+     some dollar substitution (backreference) happens in the hostname or port
+     part of the URL.  [Yann Ylavic]
diff --git a/docs/manual/mod/mod_proxy.xml b/docs/manual/mod/mod_proxy.xml
index a440f99ccc..811c242d3f 100644
--- a/docs/manual/mod/mod_proxy.xml
+++ b/docs/manual/mod/mod_proxy.xml
@@ -1511,6 +1511,9 @@ ProxyPassReverse  "/mirror/foo/" "https://backend.example.com/"
 <contextlist><context>server config</context><context>virtual host</context>
 <context>directory</context>
 </contextlist>
+<compatibility>Since 2.4.47 the <var>key=value</var> Parameters are honored
+when the <var>url</var> parameter contains backreference(s) (see note below).
+</compatibility>
 
 <usage>
     <p>This directive is equivalent to <directive module="mod_proxy">ProxyPass</directive>
@@ -1532,20 +1535,7 @@ ProxyPassMatch "^/(.*\.gif)$" "http://backend.example.com/$1"
     <p>will cause a local request for
     <code>http://example.com/foo/bar.gif</code> to be internally converted
     into a proxy request to <code>http://backend.example.com/foo/bar.gif</code>.</p>
-    <note><title>Note</title>
-      <p>The URL argument must be parsable as a URL <em>before</em> regexp
-      substitutions (as well as after).  This limits the matches you can use.
-      For instance, if we had used</p>
-      <highlight language="config">
-ProxyPassMatch "^(/.*\.gif)$" "http://backend.example.com:8000$1"
-      </highlight>
-      <p>in our previous example, it would fail with a syntax error
-      at server startup.  This is a bug (PR 46665 in the ASF bugzilla),
-      and the workaround is to reformulate the match:</p>
-      <highlight language="config">
-ProxyPassMatch "^/(.*\.gif)$" "http://backend.example.com:8000/$1"
-      </highlight>
-    </note>
+
     <p>The <code>!</code> directive is useful in situations where you don't want
     to reverse-proxy a subdirectory.</p>
 
@@ -1564,6 +1554,20 @@ ProxyPassMatch "^/(.*\.gif)$" "http://backend.example.com:8000/$1"
       expression, the original URL will be appended to the URL parameter.
       </p>
     </note>
+    <note>
+        <title><var><code>key=value</code> Parameters versus <var>url</var> with backreference(s)</title>
+      <p>Since Apache HTTP Server 2.4.47, the <code>key=value</code> Parameters
+      are no longer ignored in a <directive>ProxyPassMatch</directive> using
+      an <var>url</var> with backreference(s). However to keep the existing
+      behavior regarding reuse/keepalive of backend connections (which were
+      never reused before for these URLs), the parameter <var>enablereuse</var>
+      (or <var>disablereuse</var>) default to <code>off</code> (resp. <code>on</code>)
+      in this case. Setting <code>enablereuse=on</code> explicitely allows to
+      reuse connections <strong>unless</strong> some backreference(s) belong in
+      the <code>authority</code> part (hostname and/or port) of the <var>url</var>
+      (this condition is enforced since Apache HTTP Server 2.4.55, and produces
+      a warning at startup because these URLs are not reusable per se).</p>
+    </note>
 
     <note type="warning">
       <title>Security Warning</title>
diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c
index 434e37c9d4..e4d9df9c48 100644
--- a/modules/proxy/proxy_util.c
+++ b/modules/proxy/proxy_util.c
@@ -1844,6 +1844,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p,
     proxy_worker_shared *wshared;
     const char *ptr = NULL, *sockpath = NULL, *pdollars = NULL;
     apr_port_t port_of_scheme;
+    int disable_reuse = 0;
     apr_uri_t uri;
 
     /*
@@ -1872,12 +1873,21 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p,
          * to fail (e.g. "ProxyPassMatch ^/(a|b)(/.*)? http://host:port$2").
          * So we trim all the $n from the :port and prepend them in uri.path
          * afterward for apr_uri_unparse() to restore the original URL below.
+         * If a dollar substitution is found in the hostname[:port] part of
+         * the URL, reusing address and connections in the same worker is not
+         * possible (the current implementation of active connections cache
+         * handles/assumes a single origin server:port per worker only), so
+         * we set disable_reuse here during parsing to take that into account
+         * in the worker settings below.
          */
 #define IS_REF(x) (x[0] == '$' && apr_isdigit(x[1]))
         const char *pos = ap_strstr_c(ptr, "://");
         if (pos) {
             pos += 3;
             while (*pos && *pos != ':' && *pos != '/') {
+                if (*pos == '$') {
+                    disable_reuse = 1;
+                }
                 pos++;
             }
             if (*pos == ':') {
@@ -1897,6 +1907,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p,
                     vec[1].iov_base = (void *)path;
                     vec[1].iov_len = strlen(path);
                     ptr = apr_pstrcatv(p, vec, 2, NULL);
+                    disable_reuse = 1;
                 }
             }
         }
@@ -1987,7 +1998,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p,
     wshared->port = (uri.port) ? uri.port : port_of_scheme;
     wshared->flush_packets = flush_off;
     wshared->flush_wait = PROXY_FLUSH_WAIT;
-    wshared->is_address_reusable = 1;
+    wshared->is_address_reusable = !disable_reuse;
     wshared->lbfactor = 100;
     wshared->passes = 1;
     wshared->fails = 1;
@@ -1996,7 +2007,31 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p,
     wshared->hash.def = ap_proxy_hashfunc(wshared->name, PROXY_HASHFUNC_DEFAULT);
     wshared->hash.fnv = ap_proxy_hashfunc(wshared->name, PROXY_HASHFUNC_FNV);
     wshared->was_malloced = (mask & AP_PROXY_WORKER_IS_MALLOCED) != 0;
-    wshared->is_name_matchable = 0;
+    if (mask & AP_PROXY_WORKER_IS_MATCH) {
+        wshared->is_name_matchable = 1;
+
+        /* Before AP_PROXY_WORKER_IS_MATCH (< 2.4.47), a regex worker with
+         * dollar substitution was never matched against any actual URL, thus
+         * the requests fell through the generic worker. Now if a ProyPassMatch
+         * matches, a worker (and its parameters) is always used to determine
+         * the properties of the connection with the origin server. So for
+         * instance the same "timeout=" will be enforced for all the requests
+         * matched by the same ProyPassMatch worker, which is an improvement
+         * compared to the global/vhost [Proxy]Timeout applied by the generic
+         * worker. Likewise, address and connection reuse is the default for
+         * a ProyPassMatch worker with no dollar substitution, just like a
+         * "normal" worker. However to avoid DNS and connection reuse compat
+         * issues, connection reuse is disabled by default if there is any
+         * substitution in the uri-path (an explicit enablereuse=on can still
+         * opt-in), and reuse is even disabled definitively for substitutions
+         * happening in the hostname[:port] (disable_reuse was set above so
+         * address reuse is also disabled which will prevent enablereuse=on
+         * to apply anyway).
+         */
+        if (disable_reuse || ap_strchr_c(wshared->name, '$')) {
+            wshared->disablereuse = 1;
+        }
+    }
     if (sockpath) {
         if (PROXY_STRNCPY(wshared->uds_path, sockpath) != APR_SUCCESS) {
             return apr_psprintf(p, "worker uds path (%s) too long", sockpath);
@@ -2016,20 +2051,6 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p,
     (*worker)->balancer = balancer;
     (*worker)->s = wshared;
 
-    if (mask & AP_PROXY_WORKER_IS_MATCH) {
-        (*worker)->s->is_name_matchable = 1;
-        if (ap_strchr_c((*worker)->s->name, '$')) {
-            /* Before AP_PROXY_WORKER_IS_MATCH (< 2.4.47), a regex worker
-             * with dollar substitution was never matched against the actual
-             * URL thus the request fell through the generic worker. To avoid
-             * dns and connection reuse compat issues, let's disable connection
-             * reuse by default, it can still be overwritten by an explicit
-             * enablereuse=on.
-             */
-            (*worker)->s->disablereuse = 1;
-        }
-    }
-
     return NULL;
 }
 
@@ -2115,12 +2136,22 @@ PROXY_DECLARE(apr_status_t) ap_proxy_initialize_worker(proxy_worker *worker, ser
         if (!worker->s->retry_set) {
             worker->s->retry = apr_time_from_sec(PROXY_WORKER_DEFAULT_RETRY);
         }
-        /* By default address is reusable unless DisableReuse is set */
+        /* Consistently set address and connection reusabilty: when reuse
+         * is disabled by configuration, or when the address is known already
+         * to not be reusable for this worker (in any case, thus ignore/force
+         * DisableReuse).
+         */
         if (worker->s->disablereuse) {
             worker->s->is_address_reusable = 0;
         }
-        else {
-            worker->s->is_address_reusable = 1;
+        else if (!worker->s->is_address_reusable) {
+            /* Explicit enablereuse=on can't work in this case, warn user. */
+            if (worker->s->disablereuse_set && !worker->s->disablereuse) {
+                ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s, APLOGNO(10400)
+                             "enablereuse/disablereuse ignored for worker %s",
+                             ap_proxy_worker_name(p, worker));
+            }
+            worker->s->disablereuse = 1;
         }
 
         /*