Allow s2n_init() to be called multiple times #1999
Conversation
|
I think this is fairly noncontroversial behavior, but let me know if there is something I'm missing! |
| @@ -27,6 +27,8 @@ | |||
|
|
|||
| #include "openssl/opensslv.h" | |||
|
|
|||
There was a problem hiding this comment.
Should this be volatile?
Alternatively (and safer) maybe pthread_once() may be even better?
There was a problem hiding this comment.
My actual use case is not concerned with concurrent calls to s2n_init(). I have one library's code path calling s2n_init() at process start and another lazily calling s2n_init() some time after start.
But I will look into pthread_once() to see if I can cover easily cover the concurrent use case.
|
Also maybe a test where you create a race between multiple threads calling init and cleanup? |
Codecov Report
@@ Coverage Diff @@
## master #1999 +/- ##
==========================================
- Coverage 89.26% 80.65% -8.61%
==========================================
Files 150 232 +82
Lines 11751 17404 +5653
==========================================
+ Hits 10490 14038 +3548
- Misses 1261 3366 +2105 |
|
This is falling down my list of priorities but I think it is important and will get back to it. |
Codecov Report
@@ Coverage Diff @@
## main #1999 +/- ##
==========================================
- Coverage 81.98% 80.24% -1.75%
==========================================
Files 271 271
Lines 18730 18679 -51
==========================================
- Hits 15356 14989 -367
- Misses 3374 3690 +316 |
|
took a shot at using |
| @@ -61,6 +85,9 @@ int s2n_cleanup(void) | |||
| /* s2n_cleanup is supposed to be called from each thread before exiting, | |||
| * so ensure that whatever clean ups we have here are thread safe */ | |||
| GUARD_AS_POSIX(s2n_rand_cleanup_thread()); | |||
| /* This isn't strictly necessary as it isn't currently possible to call s2n_init() again after a call to | |||
There was a problem hiding this comment.
need to purge this comment.
Subsequent calls to s2n_init() without a call to s2n_cleanup() will return success and reuse the previously initialized library state. This change is to accommodate use cases where multiple libraries/threads within a process have a dependency on s2n and attempt to call s2n_init() indepdently. For example, consider a case where a webserver library tries to call s2n_init() every time a thread is spawned. For these use cases I think it is simplest to allow all subsequent calls to s2n_init() to succeed. Alternatively, we could try to keep some global state in the application to guard all code paths that touch s2n_init(), but this can be a large effort that requires coordination between all libraries in a process.
The error handling feedback from pthread_once is wonky. We're relying on s2n_errno being set properly after the function is executed.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Subsequent calls to s2n_init() without a call to s2n_cleanup() will now
return success and reuse the previously initialized library state.
This change is to accommodate use cases where multiple libraries/threads
within a process have a dependency on s2n and attempt to call s2n_init()
indepdently. For example, consider a case where a webserver library
tries to call s2n_init() every time a thread is spawned. For these use
cases I think it is simplest to allow all subsequent calls to s2n_init()
to succeed.
Alternatively, we could try to keep some global state in the application
to guard all code paths that touch s2n_init(), but this can be a large
effort that requires coordination between all libraries in a process.
Testing:
Called s2n_init() twice in a unit test.