Unreproducible proxy connection destructor/cleanup freed memory access

[Yann Ylavic <yl...@gmail.com>]

Helo,

here are 2 patches against mod_proxy I would hope they prevent crashes but
this isn't the case, they both address unreachable code.
Let me explain a bit...


In 2.2/2.4/trunk, connection_destructor/cleanup() both check the
worker->cp[->pool] to determine whether the connection is being destroyed
with the reslist (cp->pool == NULL) or at runtime (non NULL).

In the former case, they ought to do nothing since everything has been
cleared already (as a subpool of the worker's pool), hence even accessing
*conn is invalid.
In the latter case, the cleanup/destroy job has to be done.

The problem is that I am unable to reproduce the former case since for any
MPM I use, the reslist's pool is pconf, which has been created in the
parent process, and will never be cleared/destroyed in the children (where
only the connections live), nor will the reslist be destroyed explicitely.

Unless I'm missing an MPM which destroys the reslist in the children, the
former case cannot occur, so I guess the safety checks are useless, and so
are my patches (I could propose another one to remove the checks instead).

Otherwise, in the case where an existing MPM can trigger a non-empty
reslist cleanup, or if the reslists were to be explicitely cleared when
children exit (this isn't the case actually, but could be usefull to
shutdown SSL connections properly for example), I guess the 2 paches below
should be applied.


1. This patch is against connection_destructor(), which checks
conn->worker->cp->pool to determine whereas it has been NULLified.
The problem is that if this occurs, *conn is not accessible, hence
conn->worker is undefined.
The patch uses *(proxy_worker *)params instead, which is a valid reference.

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 1543625)
+++ modules/proxy/proxy_util.c    (working copy)
@@ -1481,9 +1481,10 @@ static apr_status_t connection_destructor(void *re
                                           apr_pool_t *pool)
 {
     proxy_conn_rec *conn = (proxy_conn_rec *)resource;
+    proxy_worker *worker = (proxy_worker *)params;

     /* Destroy the pool only if not called from reslist_destroy */
-    if (conn->worker->cp->pool) {
+    if (worker->cp->pool) {
         apr_pool_destroy(conn->pool);
     }

[END]


2. This patch is against connection_cleanup(), which checks worker->cp
instead of worker->cp->pool.

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 1556194)
+++ modules/proxy/proxy_util.c    (working copy)
@@ -1356,7 +1356,7 @@ static apr_status_t connection_cleanup(void *theco
      * If the connection pool is NULL the worker
      * cleanup has been run. Just return.
      */
-    if (!worker->cp) {
+    if (!worker->cp->pool) {
         return APR_SUCCESS;
     }

[END]


Thanks for your feedbacks,
regards,
Yann.