View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0007308 | libmicrohttpd | performance | public | 2022-08-25 13:05 | 2024-02-11 11:46 |
Reporter | ryan.hope | Assigned To | Christian Grothoff | ||
Priority | normal | Severity | minor | Reproducibility | always |
Status | closed | Resolution | no change required | ||
Product Version | 0.9.75 | ||||
Target Version | 0.9.76 | Fixed in Version | 0.9.76 | ||
Summary | 0007308: Potential heap exhaustion when using thread per-connection and TLS | ||||
Description | libgnutls 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 Reproduce | Start a libmicrohttpd daemon with HTTPS support. Spam with HTTPS connections. Memory usage gradually increases. | ||||
Additional Information | Attached screenshot of valgrind massif output showing memory usage gradually increasing. | ||||
Tags | No tags attached. | ||||
Attached Files | |||||
|
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? |
|
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); +} |
|
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. |
|
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. |
|
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! |
|
https://gitlab.com/gnutls/gnutls/-/issues/1401 |
|
Official gnutls fix merged https://gitlab.com/gnutls/gnutls/-/merge_requests/1647 |
|
No further change required in MHD ;-). |
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 |