Merge GH PR #454

  *) mod_proxy: Fix DNS requests and connections closed before the
     configured addressTTL.  BZ 69126.  [Yann Ylavic]

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1918543 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/changes-entries/fix_proxy_determine_address.txt b/changes-entries/fix_proxy_determine_address.txt
new file mode 100644 (file)
index 0000000..9f5f33a
--- /dev/null
+++ b/changes-entries/fix_proxy_determine_address.txt
@@ -0,0 +1,2 @@
+  *) mod_proxy: Fix DNS requests and connections closed before the
+     configured addressTTL.  BZ 69126.  [Yann Ylavic]

diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c
index a54a4face217ea132b1ce29d97750a14a71febcc..3da6ebf2a1521ebb97b933aa9f2b25ca65ab037a 100644 (file)
--- a/modules/proxy/proxy_util.c
+++ b/modules/proxy/proxy_util.c
@@ -1512,8 +1512,9 @@ static void socket_cleanup(proxy_conn_rec *conn)
     apr_pool_clear(conn->scpool);
 }
 
-static void address_cleanup(proxy_conn_rec *conn)
+static void conn_cleanup(proxy_conn_rec *conn)
 {
+    socket_cleanup(conn);
     conn->address = NULL;
     conn->addr = NULL;
     conn->hostname = NULL;
@@ -1522,9 +1523,6 @@ static void address_cleanup(proxy_conn_rec *conn)
     if (conn->uds_pool) {
         apr_pool_clear(conn->uds_pool);
     }
-    if (conn->sock) {
-        socket_cleanup(conn);
-    }
 }
 
 static apr_status_t conn_pool_cleanup(void *theworker)
@@ -2784,8 +2782,8 @@ static apr_status_t worker_address_resolve(proxy_worker *worker,
         apr_sockaddr_t *addr = *paddr;
         for (; addr; addr = addr->next) {
             addrs = apr_psprintf(pool, "%s%s%pI",
-                                 addrs ? ", " : "",
                                  addrs ? addrs : "",
+                                 addrs ? ", " : "",
                                  addr);
         }
         if (r) {
@@ -2959,15 +2957,16 @@ PROXY_DECLARE(apr_status_t) ap_proxy_determine_address(const char *proxy_functio
                      */
                     address->expiry = apr_atomic_read32(&worker->s->address_expiry);
                     if (address->expiry <= now) {
-                        apr_uint32_t new_expiry = address->expiry + ttl;
-                        while (new_expiry <= now) {
-                            new_expiry += ttl;
-                        }
-                        new_expiry = apr_atomic_cas32(&worker->s->address_expiry,
-                                                      new_expiry, address->expiry);
-                        /* race lost? well the expiry should grow anyway.. */
-                        AP_DEBUG_ASSERT(new_expiry > now);
-                        address->expiry = new_expiry;
+                        apr_uint32_t prev, next = (now + ttl) - (now % ttl);
+                        do {
+                            prev = apr_atomic_cas32(&worker->s->address_expiry,
+                                                    next, address->expiry);
+                            if (prev == address->expiry) {
+                                address->expiry = next;
+                                break;
+                            }
+                            address->expiry = prev;
+                        } while (prev <= now);
                     }
                 }
                 else {
@@ -3008,13 +3007,40 @@ PROXY_DECLARE(apr_status_t) ap_proxy_determine_address(const char *proxy_functio
 
             PROXY_THREAD_UNLOCK(worker);
 
-            /* Kill any socket using the old address */
-            if (conn->sock) {
-                if (r ? APLOGrdebug(r) : APLOGdebug(s)) {
-                    /* XXX: this requires the old conn->addr[ess] to still
-                     * be alive since it's not copied by apr_socket_connect()
-                     * in ap_proxy_connect_backend().
-                     */
+            /* Release the old conn address */
+            if (conn->address) {
+                /* On Windows and OS/2, apr_socket_connect() called from
+                 * ap_proxy_connect_backend() does a simple pointer copy of
+                 * its given conn->addr[->next] into conn->sock->remote_addr.
+                 * Thus conn->addr cannot be freed if the conn->sock should be
+                 * kept alive (same new and old addresses) and the old address
+                 * is still in conn->sock->remote_addr. In this case we rather
+                 * delay the release of the old address by moving the cleanup
+                 * to conn->scpool such that it runs when the socket is closed.
+                 * In any other case, including other platforms, just release
+                 * the old address now since conn->sock->remote_addr is either
+                 * obsolete (socket forcibly closed) or a copy on conn->scpool
+                 * already (not a dangling pointer).
+                 */
+                int keep_addr_alive = 0,
+                    keep_conn_alive = (conn->sock && conn->addr &&
+                                       proxy_addrs_equal(conn->addr,
+                                                         address->addr));
+                if (keep_conn_alive) {
+#if defined(WIN32) || defined(OS2)
+                    apr_sockaddr_t *remote_addr = NULL;
+                    apr_socket_addr_get(&remote_addr, APR_REMOTE, conn->sock);
+                    for (addr = conn->addr; addr; addr = addr->next) {
+                        if (addr == remote_addr) {
+                            keep_addr_alive = 1;
+                            break;
+                        }
+                    }
+#else
+                    /* Nothing to do, keep_addr_alive = 0 */
+#endif
+                }
+                else if (conn->sock && (r ? APLOGrdebug(r) : APLOGdebug(s))) {
                     apr_sockaddr_t *local_addr = NULL;
                     apr_sockaddr_t *remote_addr = NULL;
                     apr_socket_addr_get(&local_addr, APR_LOCAL, conn->sock);
@@ -3032,18 +3058,26 @@ PROXY_DECLARE(apr_status_t) ap_proxy_determine_address(const char *proxy_functio
                                      local_addr, remote_addr);
                     }
                 }
-                socket_cleanup(conn);
+                if (keep_addr_alive) {
+                    apr_pool_cleanup_kill(conn->pool, conn->address,
+                                          proxy_address_cleanup);
+                    apr_pool_cleanup_register(conn->scpool, conn->address,
+                                              proxy_address_cleanup,
+                                              apr_pool_cleanup_null);
+                }
+                else {
+                    apr_pool_cleanup_run(conn->pool, conn->address,
+                                         proxy_address_cleanup);
+                    if (!keep_conn_alive) {
+                        conn_cleanup(conn);
+                    }
+                }
             }
 
-            /* Kill the old address (if any) and use the new one */
-            if (conn->address) {
-                apr_pool_cleanup_run(conn->pool, conn->address,
-                                     proxy_address_cleanup);
-            }
+            /* Use the new address */
             apr_pool_cleanup_register(conn->pool, address,
                                       proxy_address_cleanup,
                                       apr_pool_cleanup_null);
-            address_cleanup(conn);
             conn->address = address;
             conn->hostname = address->hostname;
             conn->port = address->hostport;
@@ -3125,7 +3159,7 @@ ap_proxy_determine_connection(apr_pool_t *p, request_rec *r,
         if (!conn->uds_path || strcmp(conn->uds_path, uds_path) != 0) {
             apr_pool_t *pool = conn->pool;
             if (conn->uds_path) {
-                address_cleanup(conn);
+                conn_cleanup(conn);
                 if (!conn->uds_pool) {
                     apr_pool_create(&conn->uds_pool, worker->cp->dns_pool);
                 }
@@ -3226,7 +3260,7 @@ ap_proxy_determine_connection(apr_pool_t *p, request_rec *r,
         if (conn->hostname
             && (conn->port != hostport
                 || ap_cstr_casecmp(conn->hostname, hostname) != 0)) {
-            address_cleanup(conn);
+            conn_cleanup(conn);
         }
 
         /* Resolve the connection address with the determined hostname/port */