Changeset e2ba939 in mod_gnutls

Timestamp:

Jun 15, 2016, 1:32:14 PM (6 years ago)

Author:

Thomas Klute <thomas2.klute@…>

Branches:

asyncio, debian/master, debian/stretch-backports, master, proxy-ticket, upstream

Children:

81433f1

Parents:

317b569

git-author:

Thomas Klute <thomas2.klute@…> (06/15/16 01:52:34)

git-committer:

Thomas Klute <thomas2.klute@…> (06/15/16 13:32:14)

Message:

Prevent memory leaks in post_conf hook

Valgrind indicated memory leaks from GnuTLS structure initializations
in mgs_load_files(). This method is called from the post_config hook,
so there was no continuous leak, but graceful restart might have been
affected. There were two main reasons for the leaks:

• GnuTLS does not use the APR memory pool infrastructure, so its structures are not automatically deinitialized when the pool lifetime ends. Registering a cleanup hook with the pool solves this problem.

• Memory for the X.509 certificate chain (sc->certs_x509_crt_chain) is allocated from the config pool at the beginning of mgs_load_files(), but the certificates were loaded using gnutls_x509_crt_list_import2(), which allocates memory on its own, the pointer to which was lost when the server config pool was destroyed. Switch to gnutls_x509_crt_list_import() to use the pool memory.

Additionally, all pointers to the structures released by the pool
cleanup hook must be properly initialized to NULL (and reset on
deinit) to make sure they are only deinitialized if they have been
actually been initialized. Not doing that results in interesting stack
traces, but not a working server.

Note that this commit fixes only the leaks visible in my Valgrind
analysis of a server with an X.509 identity and an anonymous
client. Other structures will be covered by follow-up commits after
additional review.

File:

• src/gnutls_config.c (modified) (9 diffs)

src/gnutls_config.c

@@ -95 +95 @@
         "-----END DH PARAMETERS-----\n";
 
+/*
+ * Clean up the various GnuTLS data structures allocated from
+ * mgs_load_files()
+ */
+static apr_status_t mgs_pool_free_credentials(void *arg)
+{
+    mgs_srvconf_rec *sc = (mgs_srvconf_rec *) arg;
+
+    if (sc->certs)
+    {
+        gnutls_certificate_free_credentials(sc->certs);
+        sc->certs = NULL;
+    }
+
+    if (sc->anon_creds)
+    {
+        gnutls_anon_free_server_credentials(sc->anon_creds);
+        sc->anon_creds = NULL;
+    }
+
+#ifdef ENABLE_SRP
+    if (sc->srp_creds)
+    {
+        gnutls_srp_free_server_credentials(sc->srp_creds);
+        sc->srp_creds = NULL;
+    }
+#endif
+
+    if (sc->dh_params)
+    {
+        gnutls_dh_params_deinit(sc->dh_params);
+        sc->dh_params = NULL;
+    }
+
+    for (unsigned int i = 0; i < sc->certs_x509_chain_num; i++)
+    {
+        gnutls_pcert_deinit(&sc->certs_x509_chain[i]);
+        gnutls_x509_crt_deinit(sc->certs_x509_crt_chain[i]);
+    }
+
+    if (sc->privkey_x509)
+    {
+        gnutls_privkey_deinit(sc->privkey_x509);
+        sc->privkey_x509 = NULL;
+    }
+
+    if (sc->priorities)
+    {
+        gnutls_priority_deinit(sc->priorities);
+        sc->priorities = NULL;
+    }
+
+    return APR_SUCCESS;
+}
+
 int mgs_load_files(apr_pool_t * p, server_rec * s)
 {
@@ -110 +165 @@
     sc->cert_crt_pgp = apr_pcalloc(p, sizeof(sc->cert_crt_pgp[0]));
     sc->certs_x509_chain =
-        apr_pcalloc(p, MAX_CHAIN_SIZE * sizeof(sc->certs_x509_chain[0]));
+        apr_pcalloc(p, MAX_CHAIN_SIZE * sizeof(sc->certs_x509_chain[0]));
     sc->certs_x509_crt_chain =
-        apr_pcalloc(p,
-                    MAX_CHAIN_SIZE * sizeof(sc->certs_x509_crt_chain[0]));
-
-    ret = gnutls_certificate_allocate_credentials(&sc->certs);
-    if (ret < 0) {
-        ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
-                     "GnuTLS: Failed to initialize" ": (%d) %s", ret,
-                     gnutls_strerror(ret));
-        ret = -1;
-        goto cleanup;
-    }
-
-    ret = gnutls_anon_allocate_server_credentials(&sc->anon_creds);
-    if (ret < 0) {
-        ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
-                     "GnuTLS: Failed to initialize" ": (%d) %s", ret,
-                     gnutls_strerror(ret));
-        ret = -1;
-        goto cleanup;
+        apr_pcalloc(p, MAX_CHAIN_SIZE * sizeof(sc->certs_x509_crt_chain[0]));
+
+    /* Cleanup function for the GnuTLS structures allocated below */
+    apr_pool_cleanup_register(p, sc, mgs_pool_free_credentials,
+                              apr_pool_cleanup_null);
+
+    if (sc->certs == NULL)
+    {
+        ret = gnutls_certificate_allocate_credentials(&sc->certs);
+        if (ret < 0) {
+            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
+                         "GnuTLS: Failed to initialize" ": (%d) %s", ret,
+                         gnutls_strerror(ret));
+            ret = -1;
+            goto cleanup;
+        }
+    }
+
+    if (sc->anon_creds == NULL)
+    {
+        ret = gnutls_anon_allocate_server_credentials(&sc->anon_creds);
+        if (ret < 0) {
+            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
+                         "GnuTLS: Failed to initialize" ": (%d) %s", ret,
+                         gnutls_strerror(ret));
+            ret = -1;
+            goto cleanup;
+        }
     }
 
     /* Load SRP parameters */
 #ifdef ENABLE_SRP
-    ret = gnutls_srp_allocate_server_credentials(&sc->srp_creds);
-    if (ret < 0) {
-        ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
-                     "GnuTLS: Failed to initialize" ": (%d) %s", ret,
-                     gnutls_strerror(ret));
-        ret = -1;
-        goto cleanup;
+    if (sc->srp_creds == NULL)
+    {
+        ret = gnutls_srp_allocate_server_credentials(&sc->srp_creds);
+        if (ret < 0) {
+            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
+                         "GnuTLS: Failed to initialize" ": (%d) %s", ret,
+                         gnutls_strerror(ret));
+            ret = -1;
+            goto cleanup;
+        }
     }
 
@@ -161 +228 @@
 #endif
 
-    ret = gnutls_dh_params_init(&sc->dh_params);
-    if (ret < 0) {
-            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
-                         "GnuTLS: Failed to initialize"
-                         ": (%d) %s", ret, gnutls_strerror(ret));
-            ret = -1;
-            goto cleanup;
-    }
+    if (sc->dh_params == NULL)
+    {
+        ret = gnutls_dh_params_init(&sc->dh_params);
+        if (ret < 0) {
+            ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, s,
+                         "GnuTLS: Failed to initialize"
+                         ": (%d) %s", ret, gnutls_strerror(ret));
+            ret = -1;
+            goto cleanup;
+        }
 
     /* Load DH parameters */
@@ -205 +274 @@
         }
     }
-
-    if (sc->x509_cert_file != NULL) {
-        unsigned int chain_num, i;
-        unsigned format = GNUTLS_X509_FMT_PEM;
+    }
+
+    if (sc->x509_cert_file != NULL && sc->certs_x509_crt_chain[0] == NULL)
+    {
+        unsigned int chain_num = MAX_CHAIN_SIZE;
+        unsigned format = GNUTLS_X509_FMT_PEM;
 
         /* Load X.509 certificate */
@@ -259 +330 @@
         }
 
-        ret =
-            gnutls_x509_crt_list_import2(&sc->certs_x509_crt_chain,
-                                        &chain_num, &data, format,
-                                        GNUTLS_X509_CRT_LIST_FAIL_IF_UNSORTED);
+        ret = gnutls_x509_crt_list_import(sc->certs_x509_crt_chain,
+                                      &chain_num, &data, format,
+                                      GNUTLS_X509_CRT_LIST_FAIL_IF_UNSORTED);
         gnutls_free(data.data);
         sc->certs_x509_chain_num = chain_num;
@@ -274 +344 @@
         }
 
-        for (i = 0; i < chain_num; i++) {
+    for (unsigned int i = 0; i < chain_num; i++)
+    {
             ret =
                 gnutls_pcert_import_x509(&sc->certs_x509_chain[i],
@@ -289 +360 @@
     }
 
-    if (sc->x509_key_file) {
+    if (sc->x509_key_file && sc->privkey_x509 == NULL)
+    {
         ret = gnutls_privkey_init(&sc->privkey_x509);
         if (ret < 0) {
@@ -513 +585 @@
     }
 
-    if (sc->priorities_str) {
+    if (sc->priorities_str && sc->priorities == NULL)
+    {
         const char *err;
         ret = gnutls_priority_init(&sc->priorities, sc->priorities_str, &err);
@@ -957 +1030 @@
     sc->privkey_x509 = NULL;
     sc->privkey_pgp = NULL;
+    sc->anon_creds = NULL;
+#ifdef ENABLE_SRP
+    sc->srp_creds = NULL;
+#endif
+    sc->certs = NULL;
     sc->certs_x509_chain_num = 0;
     sc->p11_modules = NULL;