Re: Memory leaks are observed for libgnutls in multi-thread mode

[Nikos Mavrogiannopoulos]

tangtong wrote:
> Hi,Nikos
> 
> The attach is the log for 8tps/20tps, both have memory-leaks.

Hi Tony,
 Thank you for the report. I managed to reproduce the error by modifying
mini-egain to fail on handshake. Could you please try the attached
patch? It makes the hash functions used during handshake a bit more
conservative in use and are now always released on deinit.


best regards,
Nikos

diff --git a/lib/gnutls_cipher.c b/lib/gnutls_cipher.c
index 0d8c5e1..5f2ed62 100644
--- a/lib/gnutls_cipher.c
+++ b/lib/gnutls_cipher.c
@@ -418,6 +418,10 @@ _gnutls_compressed2ciphertext (gnutls_session_t session,
   return length;
 }
 
+#define DEINIT_MAC(td, ver, algo) \
+          if (algo != GNUTLS_MAC_NULL) \
+            mac_deinit (&td, NULL, ver);
+
 /* Deciphers the ciphertext packet, and puts the result to compress_data, of 
compress_size.
  * Returns the actual compressed packet size.
  */
@@ -473,6 +477,9 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
                                   ciphertext.size)) < 0)
        {
          gnutls_assert ();
+         DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
+
+
          return ret;
        }
 
@@ -483,6 +490,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
       if ((ciphertext.size < blocksize) || (ciphertext.size % blocksize != 0))
        {
          gnutls_assert ();
+         DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
          return GNUTLS_E_DECRYPTION_FAILED;
        }
 
@@ -492,6 +500,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
                                   ciphertext.size)) < 0)
        {
          gnutls_assert ();
+         DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
          return ret;
        }
 
@@ -505,6 +514,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
          if (ciphertext.size == 0)
            {
              gnutls_assert ();
+              DEINIT_MAC(td, ver, 
session->security_parameters.write_mac_algorithm);
              return GNUTLS_E_DECRYPTION_FAILED;
            }
        }
@@ -537,6 +547,7 @@ _gnutls_ciphertext2compressed (gnutls_session_t session,
       break;
     default:
       gnutls_assert ();
+      DEINIT_MAC(td, ver, session->security_parameters.write_mac_algorithm);
       return GNUTLS_E_INTERNAL_ERROR;
     }
 
diff --git a/lib/gnutls_handshake.c b/lib/gnutls_handshake.c
index 83dc54e..28443d1 100644
--- a/lib/gnutls_handshake.c
+++ b/lib/gnutls_handshake.c
@@ -69,7 +69,7 @@ int _gnutls_server_select_comp_method (gnutls_session_t 
session,
 
 /* Clears the handshake hash buffers and handles.
  */
-static void
+void
 _gnutls_handshake_hash_buffers_clear (gnutls_session_t session)
 {
   _gnutls_hash_deinit (&session->internals.handshake_mac_handle_md5, NULL);
@@ -223,7 +223,8 @@ _gnutls_finished (gnutls_session_t session, int type, void 
*ret)
   if (rc < 0)
     {
       gnutls_assert ();
-      _gnutls_hash_deinit (&td_md5, NULL);
+      if (!_gnutls_version_has_selectable_prf(ver))
+        _gnutls_hash_deinit (&td_md5, NULL);
       return rc;
     }
 
@@ -2197,6 +2198,7 @@ _gnutls_handshake_hash_init (gnutls_session_t session)
       if (ret < 0)
        {
          gnutls_assert ();
+         _gnutls_hash_deinit (&session->internals.handshake_mac_handle_md5, 
NULL);
          return GNUTLS_E_MEMORY_ERROR;
        }
 
diff --git a/lib/gnutls_handshake.h b/lib/gnutls_handshake.h
index f1b1bd6..2f484f5 100644
--- a/lib/gnutls_handshake.h
+++ b/lib/gnutls_handshake.h
@@ -55,6 +55,8 @@ int _gnutls_negotiate_version (gnutls_session_t session,
 int _gnutls_user_hello_func (gnutls_session_t session,
                             gnutls_protocol_t adv_version);
 
+void _gnutls_handshake_hash_buffers_clear (gnutls_session_t session);
+
 #define STATE session->internals.handshake_state
 /* This returns true if we have got there
  * before (and not finished due to an interrupt).
diff --git a/lib/gnutls_hash_int.c b/lib/gnutls_hash_int.c
index e55ae54..c57e324 100644
--- a/lib/gnutls_hash_int.c
+++ b/lib/gnutls_hash_int.c
@@ -77,6 +77,7 @@ _gnutls_hash_init (digest_hd_st * dig, 
gnutls_digest_algorithm_t algorithm)
          gnutls_assert ();
          return GNUTLS_E_HASH_FAILED;
        }
+      dig->active = 1;
       return 0;
     }
 
@@ -89,6 +90,7 @@ _gnutls_hash_init (digest_hd_st * dig, 
gnutls_digest_algorithm_t algorithm)
       return result;
     }
 
+  dig->active = 1;
   return 0;
 }
 
@@ -119,9 +121,8 @@ _gnutls_hash_copy (digest_hd_st * dst, digest_hd_st * src)
 {
   int result;
 
+  memset(dst, 0, sizeof(*dst));
   dst->algorithm = src->algorithm;
-  dst->key = NULL;             /* it's a hash anyway */
-  dst->keysize = 0;
   dst->registered = src->registered;
 
   if (src->registered)
@@ -165,7 +166,13 @@ _gnutls_hash_output (digest_hd_st * handle, void *digest)
 void
 _gnutls_hash_deinit (digest_hd_st * handle, void *digest)
 {
-  _gnutls_hash_output (handle, digest);
+  if (handle->active != 1)
+    return;
+
+  if (digest != NULL)
+    _gnutls_hash_output (handle, digest);
+
+  handle->active = 0;
 
   if (handle->registered && handle->hd.rh.ctx != NULL)
     {
@@ -269,6 +276,7 @@ _gnutls_hmac_init (digest_hd_st * dig, 
gnutls_mac_algorithm_t algorithm,
          return GNUTLS_E_HASH_FAILED;
        }
 
+      dig->active = 1;
       return 0;
     }
 
@@ -283,6 +291,7 @@ _gnutls_hmac_init (digest_hd_st * dig, 
gnutls_mac_algorithm_t algorithm,
 
   _gnutls_mac_ops.setkey (dig->hd.gc, key, keylen);
 
+  dig->active = 1;
   return 0;
 }
 
@@ -323,8 +332,13 @@ _gnutls_hmac_output (digest_hd_st * handle, void *digest)
 void
 _gnutls_hmac_deinit (digest_hd_st * handle, void *digest)
 {
-  _gnutls_hmac_output (handle, digest);
+  if (handle->active != 1)
+    return;
+
+  if (digest)
+    _gnutls_hmac_output (handle, digest);
 
+  handle->active = 0;
   if (handle->registered && handle->hd.rh.ctx != NULL)
     {
       handle->hd.rh.cc->deinit (handle->hd.rh.ctx);
@@ -398,6 +412,7 @@ _gnutls_mac_deinit_ssl3 (digest_hd_st * handle, void 
*digest)
   if (padsize == 0)
     {
       gnutls_assert ();
+      _gnutls_hash_deinit (handle, NULL);
       return;
     }
 
@@ -407,6 +422,7 @@ _gnutls_mac_deinit_ssl3 (digest_hd_st * handle, void 
*digest)
   if (rc < 0)
     {
       gnutls_assert ();
+      _gnutls_hash_deinit (handle, NULL);
       return;
     }
 
diff --git a/lib/gnutls_hash_int.h b/lib/gnutls_hash_int.h
index 8017d12..d915af5 100644
--- a/lib/gnutls_hash_int.h
+++ b/lib/gnutls_hash_int.h
@@ -52,6 +52,7 @@ typedef struct
   gnutls_mac_algorithm_t algorithm;
   const void *key;
   int keysize;
+  int active;
 } digest_hd_st;
 
 int _gnutls_hmac_init (digest_hd_st*, gnutls_mac_algorithm_t algorithm,
diff --git a/lib/gnutls_state.c b/lib/gnutls_state.c
index fede2a0..6934aa1 100644
--- a/lib/gnutls_state.c
+++ b/lib/gnutls_state.c
@@ -201,6 +201,8 @@ deinit_internal_params (gnutls_session_t session)
   if (session->internals.params.free_rsa_params)
     gnutls_rsa_params_deinit (session->internals.params.rsa_params);
 
+  _gnutls_handshake_hash_buffers_clear(session);
+
   memset (&session->internals.params, 0, sizeof (session->internals.params));
 }