Merge r1781187, r1781190, r1781312 from trunk:

mod_ssl: work around leaks on (graceful) restart.

Tested with valgrind and --with-ssl shared/static.

mod_ssl: follow up to r1781187.
The ssl_util_thread_*() functions are not necessary with openssl-1.1+

mod_ssl: follow up to r1781187.
Address SSL_CTX leak in (merged) proxy_ctx.

Reviewed by: ylavic, jim, wrowe

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

diff --git a/CHANGES b/CHANGES
index b5bda84fa7d7545bd87dc7bb58d498d22ce36ad5..b4761769132cb0396178b29c4cf617da3ef2cd9d 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,8 @@
 
 Changes with Apache 2.4.26
 
+  *) mod_ssl: work around leaks on (graceful) restart. [Yann Ylavic]
+
   *) mod_ssl: Add support for OpenSSL 1.1.0. [Rainer Jung]
 
   *) Don't set SO_REUSEPORT unless ListenCoresBucketsRatio is greater

diff --git a/STATUS b/STATUS
index 1fe109d1fda407e4e18245e66a4ab84f514bea6f..21b19e4f030dc0a4ae960c088516eb40f8369157 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -150,13 +150,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  *) mod_ssl: work around leaks on (graceful) restart.
-     trunk patch: http://svn.apache.org/r1781187
-                  http://svn.apache.org/r1781190
-                  http://svn.apache.org/r1781312
-     2.4.x patch: http://home.apache.org/~ylavic/patches/httpd-2.4.x-mod_ssl-restart_leaks-v2.patch
-     +1: ylavic, jim, wrowe
-
   *) core: %{DOCUMENT_URI} used in nested SSI expressions should point to the
      URI originally requsted by the user, not the nested documents URI. This
      restores the behavior of this variable to match the "legacy" SSI parser.

diff --git a/modules/ssl/mod_ssl.c b/modules/ssl/mod_ssl.c
index bf9edb28d8cd0a76ab8748961d27b3b7c7405453..f58e145f193657d75b11a5a86bf6beab5ecbb02d 100644 (file)
--- a/modules/ssl/mod_ssl.c
+++ b/modules/ssl/mod_ssl.c
@@ -30,9 +30,12 @@
 #include "util_md5.h"
 #include "util_mutex.h"
 #include "ap_provider.h"
+#include "http_config.h"
 
 #include <assert.h>
 
+static int modssl_running_statically = 0;
+
 APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(ssl, SSL, int, pre_handshake,
                                     (conn_rec *c,SSL *ssl,int is_proxy),
                                     (c,ssl,is_proxy), OK, DECLINED);
@@ -297,21 +300,40 @@ static const command_rec ssl_config_cmds[] = {
 /*
  *  the various processing hooks
  */
+static int modssl_is_prelinked(void)
+{
+    apr_size_t i = 0;
+    const module *mod;
+    while ((mod = ap_prelinked_modules[i++])) {
+        if (strcmp(mod->name, "mod_ssl.c") == 0) {
+            return 1;
+        }
+    }
+    return 0;
+}
+
 static apr_status_t ssl_cleanup_pre_config(void *data)
 {
     /*
      * Try to kill the internals of the SSL library.
      */
-    /* Corresponds to OPENSSL_load_builtin_modules():
-     * XXX: borrowed from apps.h, but why not CONF_modules_free()
-     * which also invokes CONF_modules_finish()?
-     */
-    CONF_modules_unload(1);
+#ifdef HAVE_FIPS
+    FIPS_mode_set(0);
+#endif
+    /* Corresponds to OBJ_create()s */
+    OBJ_cleanup();
+    /* Corresponds to OPENSSL_load_builtin_modules() */
+    CONF_modules_free();
     /* Corresponds to SSL_library_init: */
     EVP_cleanup();
 #if HAVE_ENGINE_LOAD_BUILTIN_ENGINES
     ENGINE_cleanup();
 #endif
+#if OPENSSL_VERSION_NUMBER >= 0x1000200fL
+    SSL_COMP_free_compression_methods();
+#endif
+
+    /* Usually needed per thread, but this parent process is single-threaded */
 #if OPENSSL_VERSION_NUMBER < 0x10100000L
 #if OPENSSL_VERSION_NUMBER >= 0x1000000fL
     ERR_remove_thread_state(NULL);
@@ -327,11 +349,14 @@ static apr_status_t ssl_cleanup_pre_config(void *data)
     ERR_free_strings();
 #endif
 
-    /* Also don't call CRYPTO_cleanup_all_ex_data here; any registered
-     * ex_data indices may have been cached in static variables in
-     * OpenSSL; removing them may cause havoc.  Notably, with OpenSSL
+    /* Also don't call CRYPTO_cleanup_all_ex_data when linked statically here;
+     * any registered ex_data indices may have been cached in static variables
+     * in OpenSSL; removing them may cause havoc.  Notably, with OpenSSL
      * versions >= 0.9.8f, COMP_CTX cleanups would not be run, which
      * could result in a per-connection memory leak (!). */
+    if (!modssl_running_statically) {
+        CRYPTO_cleanup_all_ex_data();
+    }
 
     /*
      * TODO: determine somewhere we can safely shove out diagnostics
@@ -345,6 +370,15 @@ static int ssl_hook_pre_config(apr_pool_t *pconf,
                                apr_pool_t *plog,
                                apr_pool_t *ptemp)
 {
+    modssl_running_statically = modssl_is_prelinked();
+
+    /* Some OpenSSL internals are allocated per-thread, make sure they
+     * are associated to the/our same thread-id until cleaned up.
+     */
+#if APR_HAS_THREADS && OPENSSL_VERSION_NUMBER < 0x10100000L
+    ssl_util_thread_id_setup(pconf);
+#endif
+
     /* We must register the library in full, to ensure our configuration
      * code can successfully test the SSL environment.
      */
@@ -367,6 +401,9 @@ static int ssl_hook_pre_config(apr_pool_t *pconf,
                          "SRVName otherName form");
     }
 
+    /* Start w/o errors (e.g. OBJ_txt2nid() above) */
+    ERR_clear_error();
+
     /*
      * Let us cleanup the ssl library when the module is unloaded
      */

diff --git a/modules/ssl/ssl_engine_config.c b/modules/ssl/ssl_engine_config.c
index 129a01ff545525b43e71c02fefed5a8838a49714..392c9ac7d9463d8680d75f31e9fa735bc6fbe908 100644 (file)
--- a/modules/ssl/ssl_engine_config.c
+++ b/modules/ssl/ssl_engine_config.c
@@ -98,6 +98,14 @@ BOOL ssl_config_global_isfixed(SSLModConfigRec *mc)
 **  _________________________________________________________________
 */
 
+#ifdef HAVE_SSL_CONF_CMD
+static apr_status_t modssl_ctx_config_cleanup(void *ctx)
+{
+    SSL_CONF_CTX_free(ctx);
+    return APR_SUCCESS;
+}
+#endif
+
 static void modssl_ctx_init(modssl_ctx_t *mctx, apr_pool_t *p)
 {
     mctx->sc                  = NULL; /* set during module init */
@@ -157,6 +165,9 @@ static void modssl_ctx_init(modssl_ctx_t *mctx, apr_pool_t *p)
 #endif
 #ifdef HAVE_SSL_CONF_CMD
     mctx->ssl_ctx_config = SSL_CONF_CTX_new();
+    apr_pool_cleanup_register(p, mctx->ssl_ctx_config,
+                              modssl_ctx_config_cleanup,
+                              apr_pool_cleanup_null);
     SSL_CONF_CTX_set_flags(mctx->ssl_ctx_config, SSL_CONF_FLAG_FILE);
     SSL_CONF_CTX_set_flags(mctx->ssl_ctx_config, SSL_CONF_FLAG_SERVER);
     SSL_CONF_CTX_set_flags(mctx->ssl_ctx_config, SSL_CONF_FLAG_CERTIFICATE);

diff --git a/modules/ssl/ssl_engine_init.c b/modules/ssl/ssl_engine_init.c
index e60ac30434671047e9af0fa4c1997d3e4ab65913..5683c552d067ef8a72f9d306f4a44f51cbf48226 100644 (file)
--- a/modules/ssl/ssl_engine_init.c
+++ b/modules/ssl/ssl_engine_init.c
@@ -257,11 +257,9 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_pool_t *plog,
 #endif
     }
 
-#if OPENSSL_VERSION_NUMBER < 0x10100000L
-#if APR_HAS_THREADS
+#if APR_HAS_THREADS && OPENSSL_VERSION_NUMBER < 0x10100000L
     ssl_util_thread_setup(p);
 #endif
-#endif /* #if OPENSSL_VERSION_NUMBER < 0x10100000L */
 
     /*
      * SSL external crypto device ("engine") support
@@ -1641,7 +1639,6 @@ static apr_status_t ssl_init_server_ctx(server_rec *s,
             ssl_log_ssl_error(SSLLOG_MARK, APLOG_EMERG, s);
             return ssl_die(s);
     }
-    SSL_CONF_CTX_free(cctx);
 #endif
 
     if (SSL_CTX_check_private_key(sc->server->ssl_ctx) != 1) {

diff --git a/modules/ssl/ssl_private.h b/modules/ssl/ssl_private.h
index 08de3a1bf43adb793df63d7d8b4e49329fe963c3..b6483a127f118499715cab97fc40300d0ccdda96 100644 (file)
--- a/modules/ssl/ssl_private.h
+++ b/modules/ssl/ssl_private.h
@@ -922,9 +922,12 @@ void         ssl_util_ppclose(server_rec *, apr_pool_t *, apr_file_t *);
 char        *ssl_util_readfilter(server_rec *, apr_pool_t *, const char *,
                                  const char * const *);
 BOOL         ssl_util_path_check(ssl_pathcheck_t, const char *, apr_pool_t *);
+#if APR_HAS_THREADS
 #if OPENSSL_VERSION_NUMBER < 0x10100000L
 void         ssl_util_thread_setup(apr_pool_t *);
 #endif
+void         ssl_util_thread_id_setup(apr_pool_t *);
+#endif
 int          ssl_init_ssl_connection(conn_rec *c, request_rec *r);
 
 BOOL         ssl_util_vhost_matches(const char *servername, server_rec *s);

diff --git a/modules/ssl/ssl_util.c b/modules/ssl/ssl_util.c
index 052d23e82205bed510022595d3cf2eafbc1aa81d..9e4e719f4bc9928a3651b3bee25ef06f0793d731 100644 (file)
--- a/modules/ssl/ssl_util.c
+++ b/modules/ssl/ssl_util.c
@@ -383,6 +383,12 @@ static void ssl_util_thr_id(CRYPTO_THREADID *id)
 #endif
 }
 
+static apr_status_t ssl_util_thr_id_cleanup(void *old)
+{
+    CRYPTO_THREADID_set_callback(old);
+    return APR_SUCCESS;
+}
+
 #else
 
 static unsigned long ssl_util_thr_id(void)
@@ -403,16 +409,17 @@ static unsigned long ssl_util_thr_id(void)
 #endif
 }
 
+static apr_status_t ssl_util_thr_id_cleanup(void *old)
+{
+    CRYPTO_set_id_callback(old);
+    return APR_SUCCESS;
+}
+
 #endif
 
 static apr_status_t ssl_util_thread_cleanup(void *data)
 {
     CRYPTO_set_locking_callback(NULL);
-#if OPENSSL_VERSION_NUMBER >= 0x10000000L
-    CRYPTO_THREADID_set_callback(NULL);
-#else
-    CRYPTO_set_id_callback(NULL);
-#endif
 
     CRYPTO_set_dynlock_create_callback(NULL);
     CRYPTO_set_dynlock_lock_callback(NULL);
@@ -436,12 +443,6 @@ void ssl_util_thread_setup(apr_pool_t *p)
         apr_thread_mutex_create(&(lock_cs[i]), APR_THREAD_MUTEX_DEFAULT, p);
     }
 
-#if OPENSSL_VERSION_NUMBER >= 0x10000000L
-    CRYPTO_THREADID_set_callback(ssl_util_thr_id);
-#else
-    CRYPTO_set_id_callback(ssl_util_thr_id);
-#endif
-
     CRYPTO_set_locking_callback(ssl_util_thr_lock);
 
     /* Set up dynamic locking scaffolding for OpenSSL to use at its
@@ -455,5 +456,16 @@ void ssl_util_thread_setup(apr_pool_t *p)
     apr_pool_cleanup_register(p, NULL, ssl_util_thread_cleanup,
                                        apr_pool_cleanup_null);
 }
+
+void ssl_util_thread_id_setup(apr_pool_t *p)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x10000000L
+    CRYPTO_THREADID_set_callback(ssl_util_thr_id);
+#else
+    CRYPTO_set_id_callback(ssl_util_thr_id);
+#endif
+    apr_pool_cleanup_register(p, NULL, ssl_util_thr_id_cleanup,
+                                       apr_pool_cleanup_null);
+}
 #endif /* #if OPENSSL_VERSION_NUMBER < 0x10100000L */
 #endif /* #if APR_HAS_THREADS */