View Issue Details

IDProjectCategoryView StatusLast Update
0007308libmicrohttpdperformancepublic2024-02-11 11:46
Reporterryan.hope Assigned ToChristian Grothoff  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionno change required 
Product Version0.9.75 
Target Version0.9.76Fixed in Version0.9.76 
Summary0007308: Potential heap exhaustion when using thread per-connection and TLS
Descriptionlibgnutls provides an API for generating random numbers. This adds some context information globally to the heap and is done per-thread. The context information is then supposed to be cleaned up with gnutls_global_deinit() or when the lib destructor runs.
libmicrohttpd creates a thread per-connection and only calls gnutls_global_deinit() when the daemon closes. This means that we slowly accumulate random context information in libgnutls that isn't freed until program exit, eventually exhausting the heap.

Is this a known issue/expected behaviour? If so what can we do to deal with this if we want to keep using a thread per-connection?

For now our company has applied a patch to libgnutls/libmicrohttpd to free each threads random context when the connection closes, but we're wondering if there is a better approach for dealing this.
Steps To ReproduceStart a libmicrohttpd daemon with HTTPS support.
Spam with HTTPS connections.
Memory usage gradually increases.
Additional InformationAttached screenshot of valgrind massif output showing memory usage gradually increasing.
TagsNo tags attached.
Attached Files

Activities

Christian Grothoff

2022-08-26 09:26

manager   ~0019060

I wasn't aware of this (not using thread-per-connection with TLS right now at scale). Your approach sounds reasonable, alas I'd have to see where exactly you hooked the gnutls cleanup logic. Could you share your patch?

ryan.hope

2022-08-26 10:08

reporter   ~0019061

Here is a copy of our patch to both libmicrohttpd and libgnutls. This is the first time I've had to look at either of these libraries, so there is probably a much better way of doing this.
libmicrohttpd_rnd_ctx_free.patch (642 bytes)   
diff --git a/src/microhttpd/connection_https.c b/src/microhttpd/connection_https.c
old mode 100644
new mode 100755
index 0d69170..7434444
--- a/src/microhttpd/connection_https.c
+++ b/src/microhttpd/connection_https.c
@@ -200,6 +200,15 @@ MHD_tls_connection_shutdown (struct MHD_Connection *connection)
   {
     const int res =
       gnutls_bye (connection->tls_session, GNUTLS_SHUT_WR);
+
+    /**
+     * Modification to prevent heap exhaustion by
+     * freeing this threads random generator context.
+     */
+    gnutls_rnd_ctx_free();
+
     if (GNUTLS_E_SUCCESS == res)
     {
       connection->tls_state = MHD_TLS_CONN_WR_CLOSED;
libgnutls_rnd_ctx_free.patch (1,899 bytes)   
diff --git a/lib/includes/gnutls/crypto.h b/lib/includes/gnutls/crypto.h
old mode 100644
new mode 100755
index 4d4926c..fa6b004
--- a/lib/includes/gnutls/crypto.h
+++ b/lib/includes/gnutls/crypto.h
@@ -162,6 +162,7 @@ int gnutls_rnd(gnutls_rnd_level_t level, void *data, size_t len);
 
 void gnutls_rnd_refresh(void);
 
+void gnutls_rnd_ctx_free(void);
 
 /* API to override ciphers and MAC algorithms 
  */
diff --git a/lib/libgnutls.map b/lib/libgnutls.map
old mode 100644
new mode 100755
index 6e1da85..cb4a18f
--- a/lib/libgnutls.map
+++ b/lib/libgnutls.map
@@ -861,6 +861,7 @@ GNUTLS_3_4
 	gnutls_pkcs11_token_get_flags;
 	gnutls_pubkey_import_ecc_x962;
 	gnutls_rnd_refresh;
+	gnutls_rnd_ctx_free;
 	gnutls_mac_get_nonce_size;
 	gnutls_x509_crl_get_raw_issuer_dn;
 	gnutls_certificate_get_crt_raw;
diff --git a/lib/random.c b/lib/random.c
old mode 100644
new mode 100755
index 6462738..db85868
--- a/lib/random.c
+++ b/lib/random.c
@@ -191,3 +191,40 @@ void gnutls_rnd_refresh(void)
 	if (rnd_initialized && _gnutls_rnd_ops.rnd_refresh)
 		_gnutls_rnd_ops.rnd_refresh(gnutls_rnd_ctx);
 }
+
+/**
+ * gnutls_rnd_ctx_free:
+ *
+ * Modification to prevent heap exhaustion by
+ * freeing this threads random generator context.
+ * Only necessary to call in long running processes that
+ * don't routinely free the global list of contexts.
+ */
+void gnutls_rnd_ctx_free(void)
+{
+	GNUTLS_STATIC_MUTEX_LOCK(gnutls_rnd_ctx_list_mutex);
+	if (_gnutls_rnd_ops.deinit != NULL) {
+		struct rnd_ctx_list_st *e = head, *prev = head, *next;
+
+		while (e != NULL) {
+			next = e->next;
+			if (e->ctx == gnutls_rnd_ctx) {
+				if (e == head) {
+					head = next;
+				} else {
+					prev->next = next;
+				}
+				_gnutls_rnd_ops.deinit(e->ctx);
+				gnutls_free(e);
+				break;
+			}
+			prev = e;
+			e = next;
+		}
+	}
+
+	rnd_initialized = 0;
+	GNUTLS_STATIC_MUTEX_UNLOCK(gnutls_rnd_ctx_list_mutex);
+}
libgnutls_rnd_ctx_free.patch (1,899 bytes)   

ryan.hope

2022-08-26 10:33

reporter   ~0019062

Should also note that I was unsure whether to raise this issue with libmicrohttpd, libgnutls, or both. It's not immediately obvious why libgnutls stores these contexts and what they expect long running processes with many threads to do with these contexts.

Christian Grothoff

2022-08-27 11:06

manager   ~0019065

I think this is the "wrong" solution. gnutls should probably use a "pthread_key_create()" to install a hook to clean up thread-specific data exactly when the thread exits (see https://pubs.opengroup.org/onlinepubs/007908799/xsh/pthread_key_create.html). Anyway, I'll pass this on to gnutls people.

ryan.hope

2022-08-29 09:58

reporter   ~0019071

That does sound like a much better solution. If there is an issue opened against gnutls could you link it here please? Would be useful for me to track its progress. Appreciate the assistance!

ryan.hope

2022-09-09 12:52

reporter   ~0019110

https://gitlab.com/gnutls/gnutls/-/issues/1401

ryan.hope

2022-10-25 12:30

reporter   ~0019304

Official gnutls fix merged https://gitlab.com/gnutls/gnutls/-/merge_requests/1647

Christian Grothoff

2022-10-25 13:36

manager   ~0019305

No further change required in MHD ;-).

Issue History

Date Modified Username Field Change
2022-08-25 13:05 ryan.hope New Issue
2022-08-25 13:05 ryan.hope Tag Attached: messages
2022-08-25 13:05 ryan.hope File Added: Screenshot 2022-08-19 135758.png
2022-08-26 09:26 Christian Grothoff Note Added: 0019060
2022-08-26 09:26 Christian Grothoff Assigned To => Christian Grothoff
2022-08-26 09:26 Christian Grothoff Status new => feedback
2022-08-26 10:08 ryan.hope Note Added: 0019061
2022-08-26 10:08 ryan.hope File Added: libmicrohttpd_rnd_ctx_free.patch
2022-08-26 10:08 ryan.hope File Added: libgnutls_rnd_ctx_free.patch
2022-08-26 10:08 ryan.hope Status feedback => assigned
2022-08-26 10:33 ryan.hope Note Added: 0019062
2022-08-27 11:06 Christian Grothoff Note Added: 0019065
2022-08-29 09:58 ryan.hope Note Added: 0019071
2022-09-09 12:52 ryan.hope Note Added: 0019110
2022-10-25 12:30 ryan.hope Note Added: 0019304
2022-10-25 13:36 Christian Grothoff Status assigned => resolved
2022-10-25 13:36 Christian Grothoff Resolution open => no change required
2022-10-25 13:36 Christian Grothoff Fixed in Version => 0.9.76
2022-10-25 13:36 Christian Grothoff Note Added: 0019305
2022-10-25 13:37 Christian Grothoff Target Version => 0.9.76
2024-01-21 13:33 Christian Grothoff Status resolved => closed
2024-02-11 11:46 Christian Grothoff Tag Detached: messages