Enforce init and cleanup calling rules (#3446) #3512
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lrstewart
reviewed
Sep 27, 2022
jeking3
force-pushed
the
issue-3446/init-idempotency
branch
2 times, most recently
from
805b8f9 to
c02e0d7
Compare
lrstewart
reviewed
Sep 28, 2022
utils/s2n_init.c
Outdated
Comment on lines
98
to
106
| bool cleaned_up = \ | ||
| s2n_result_is_ok(s2n_cipher_suites_cleanup()) && | ||
| s2n_result_is_ok(s2n_rand_cleanup_thread()) && | ||
| s2n_result_is_ok(s2n_rand_cleanup()) && | ||
| s2n_result_is_ok(s2n_libcrypto_cleanup()) && | ||
| s2n_result_is_ok(s2n_locking_cleanup()) && | ||
| (s2n_mem_cleanup() == S2N_SUCCESS); | ||
|
|
||
| initialized = !cleaned_up; |
There was a problem hiding this comment.
Why leave initialized==true if cleanup fails?
If it's so that we can retry cleanup, isn't that a problem if some of the cleanup methods aren't idempotent? Or is the idea that the next call to s2n_init should fail?
There was a problem hiding this comment.
Correct; we did not fully clean up, so we are still initialized. Calling s2n_init in this state may leak memory or have other side-effects.
There was a problem hiding this comment.
Got it. And being able to call s2n_cleanup again isn't as big of a problem, because worst case the call fails.
jeking3
force-pushed
the
issue-3446/init-idempotency
branch
from
c02e0d7 to
9814470
Compare
lrstewart
approved these changes
Sep 28, 2022
jeking3
force-pushed
the
issue-3446/init-idempotency
branch
from
9814470 to
d9ac456
Compare
|
Rebased on main, no changes made. |
jeking3
force-pushed
the
issue-3446/init-idempotency
branch
from
d9ac456 to
788aae2
Compare
|
Rebased on main, no changes made. |
jeking3
force-pushed
the
issue-3446/init-idempotency
branch
from
788aae2 to
e1fb7b7
Compare
|
Rebased on main to pick up fix (#3529) for npn test error found in last run. No code changes in PR. |
camshaft
approved these changes
Oct 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved issues:
This fixes #3446
Description of changes:
The USAGE-GUIDE states that "... s2n_init() MUST NOT be called more than once ...".
The public documentation in s2n.h for s2n_init states that s2n_init "should only be called once".
Issue #3446 is a result of not enforcing the documented restrictions.
This pull request enforces the init-once rule, and also properly sets the internal flag
initializedto allow the library to be re-initialized, for projects that fully control the initialization scope and re-initialize the library, for example within unit tests and fixtures.Call-outs:
Any code that has been misbehaving will start getting errors instead of undefined behavior. Previously the code did not protect against calling s2n_init midway through a program, which could have unexpected consequences.
Testing:
The s2n_init unit test was modified to ensure the calling rules are enforced and that appropriate errors are present when violated.
Licensing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.