Improved support for using s2n-tls from within an unloadable shared lib #3011
Conversation
justinboswell
force-pushed
the
share-libcrypto
branch
from
81c83df to
91881f9
Compare
| if (pthread_self() == main_thread && !atexit_cleanup) { | ||
| POSIX_ENSURE(s2n_cleanup_atexit_impl(), S2N_ERR_ATEXIT); | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
| return 0; | |
| return S2N_SUCCESS; |
I think it is, because you'd most likely only use these 2 calls together, and I think making developers have to divine the magic pairing is a worse experience than a single API call that just makes it work, and which we can tie any future lifetime behavior into. |
…2n is already initialized
| @@ -83,7 +83,14 @@ struct s2n_connection; | |||
| * with S2N. | |||
| */ | |||
| S2N_API | |||
| extern void s2n_crypto_disable_init(void); | |||
| extern int s2n_crypto_disable_init(void); | |||
There was a problem hiding this comment.
Technically this is a breaking change but hopefully no one has consumed it yet 😄
There was a problem hiding this comment.
Yeah, I'm counting on that. The good news is, even if they consumed it, this cannot cause a compile failure.
There was a problem hiding this comment.
It still could, but unlikely: https://godbolt.org/z/zPeh99793
I actually meant kind of the opposite :) Should we create this API (a separate API that we will have to continue supporting) if it provides a bad experience and the combined API is the end goal, or should we just create that combined API. |
| int s2n_disable_atexit(void) { | ||
| const bool already_initialized = (main_thread != 0); |
There was a problem hiding this comment.
Can you make this comparison? The man page suggests it might not work: https://man7.org/linux/man-pages/man3/pthread_self.3.html You might need pthread_equal. And even then, can we guarantee 0 isn't a valid pthread_t?
Maybe use a separate variable for this.
There was a problem hiding this comment.
Good catch on the comparison! I had no idea
There was a problem hiding this comment.
I'm going to do a followup PR on this.
| * perform final cleanup now */ | ||
| if (pthread_self() == main_thread && !atexit_cleanup) { |
There was a problem hiding this comment.
#3011 (comment) could be an issue for this comparison too
Resolved issues:
Resolves crash on exit in Java and PHP when using s2n-tls via the AWS CRT.
Description of changes:
Currently, s2n uses an
atexit()handler to clean itself up. This is a guaranteed crash when s2n-tls is embedded in a shared library that can be unloaded.A call to
s2n_disable_atexit()prevents s2n from installing theatexit()handler, and adjustss2n_cleanup()so that it will perform final cleanup when called from the main thread (defined as the threads2n_init()was called from).Additionally, after cleaning up its ENGINE/rand implementation, s2n now sets the libcrypto RAND engine and method to NULL, preventing a crash on shutdown as libcrypto tries to free the already freed (and in the case of a shared library, unloaded) ENGINE implementation.
Call-outs:
It may be advantageous to create a single API call to prepare s2n for use in an embedded/modular/plugin environment. That is something we could address in a follow-up, once this is working in target environments. The original draft of the prior change proposed an
s2n_share_libcrypto()API, something singular feels like a good move for users.Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.