Unclear on how to use s2n_cleanup from multiple threads #649
Comments
|
Hello @64, You are correct. As is,
This is also interesting. Do you still have the leak after calling |
|
Will send a PR soon. I still have the leak; here is the valgrind output: You can safely ignore the This is the output after opening 3 sockets (one per worker thread) - each socket gets queued onto a different thread for handling (the only calls made to s2n on each worker thread are s2n_connection_* and s2n_negotiate - I don't have to call send/recv in order to trigger a leak). If I open another socket then no additional memory is lost (since the new socket gets scheduled onto the first thread again, which has already allocated). BTW, I'm using a very slightly patched version of s2n - I suspect this is the cause of the issue. Here is the diff: diff --git a/tls/s2n_cipher_preferences.c b/tls/s2n_cipher_preferences.c
index 14649e2..97d1f0d 100644
--- a/tls/s2n_cipher_preferences.c
+++ b/tls/s2n_cipher_preferences.c
@@ -21,6 +21,18 @@
#include "error/s2n_errno.h"
+struct s2n_cipher_suite *cipher_suites_h2[] = {
+ &s2n_ecdhe_rsa_with_aes_128_gcm_sha256,
+ &s2n_ecdhe_rsa_with_aes_256_gcm_sha384,
+ &s2n_ecdhe_rsa_with_chacha20_poly1305_sha256
+};
+
+const struct s2n_cipher_preferences cipher_preferences_h2 = {
+ .count = sizeof(cipher_suites_h2) / sizeof(cipher_suites_h2[0]),
+ .suites = cipher_suites_h2,
+ .minimum_protocol_version = S2N_TLS12
+};
+
/* s2n's list of cipher suites, in order of preference, as of 2014-06-01 */
struct s2n_cipher_suite *cipher_suites_20140601[] = {
&s2n_dhe_rsa_with_aes_128_cbc_sha256,
@@ -260,6 +272,7 @@ struct {
} selection[] = {
{ "default", &cipher_preferences_20170210 },
{ "default_fips", &cipher_preferences_20170405},
+ { "h2", &cipher_preferences_h2 },
{ "20140601", &cipher_preferences_20140601 },
{ "20141001", &cipher_preferences_20141001 },
{ "20150202", &cipher_preferences_20150202 },
diff --git a/tls/s2n_cipher_preferences.h b/tls/s2n_cipher_preferences.h
index b66b797..4d6b80d 100644
--- a/tls/s2n_cipher_preferences.h
+++ b/tls/s2n_cipher_preferences.h
@@ -25,6 +25,7 @@ struct s2n_cipher_preferences {
int minimum_protocol_version;
};
+extern const struct s2n_cipher_preferences cipher_preferences_h2;
extern const struct s2n_cipher_preferences cipher_preferences_20140601;
extern const struct s2n_cipher_preferences cipher_preferences_20141001;
extern const struct s2n_cipher_preferences cipher_preferences_20150202;OpenSSL / libcrypto version: Hope this helps. EDIT: Interesting to note that valgrind reports all the leaks as occuring on thread 1, despite the evidence that it's calling s2n functions from threads for the first time that causes the leaks to occur. That information might prove useful. |
|
My apologies @64 ! My previous advice was incorrect. There is per-thread DRBG state: https://github.com/awslabs/s2n/blob/master/utils/s2n_random.c#L60 . This state indeed needs to be cleaned up by every thread that uses s2n(via Back to your original issue |
|
Yeah, that's the code I linked to in my original post. I ran it through before in GDB and that's where it seemed to be going wrong. I'm not sure I can help out too much since I'm not familiar with the internals but would making that variable thread local be sufficient? (side note, do you want me to keep this issue open? or should I open a new one for the bug, to be clear) |
|
Makes sense. I will work on a patch and I think we can continue to use this issue for tracking. In the meantime, the leak should be benign because it happens once per thread but does not increase for the lifetime of the thread/process. |
The usage guide says the following:
s2n_cleanup cleans up any internal resources used by s2n. This function should be called from each thread or process that is created subsequent to calling s2n_init when that thread or process is done calling other s2n functions.
I'm running a multithreaded web server, which does something a bit like this:
According to the docs, I think this should be correct. However, s2n_cleanup fails on every invocation except the first. I suppose this is due to this code - since it is using a global (i.e non-thread-local) variable, only the first invocation from the process succeeds. If I am correct then the usage guide is wrong.
This may be an XY problem, since I was trying to debug a ~500 byte memory leak (per thread) caused by something in s2n_rand, which led me to this. I was definitely calling all the relevant cleanup functions except I was only calling s2n_cleanup on the main thread until I noticed what it said in the usage guide. It's entirely possible I'm just using the library incorrectly, but if you want to take a look at my code you can find it here.
Thanks for the help.
The text was updated successfully, but these errors were encountered: