Reference s2n_crypto_parameters via pointers #3469
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
Switching conn->secure to a pointer results in many, mostly automated, changes. To avoid hiding substantial changes in this mess, let's first JUST make the type switch without actually changing the memory management.
lrstewart
force-pushed
the
mem_1
branch
2 times, most recently
from
e53ffe4 to
ae85bf2
Compare
camshaft
approved these changes
Aug 26, 2022
goatgoose
approved these changes
Aug 26, 2022
| @@ -111,7 +112,7 @@ int s2n_tls13_default_sig_scheme(struct s2n_connection *conn, struct s2n_signatu | |||
| POSIX_GUARD(s2n_connection_get_signature_preferences(conn, &signature_preferences)); | |||
| POSIX_ENSURE_REF(signature_preferences); | |||
|
|
|||
| struct s2n_cipher_suite *cipher_suite = conn->secure.cipher_suite; | |||
| struct s2n_cipher_suite *cipher_suite = conn->secure->cipher_suite; | |||
There was a problem hiding this comment.
We may want a POSIX_ENSURE_REF here at some point.
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.
Description of changes:
This is a stepping stone towards allocating the
s2n_crypto_parametersseparately froms2n_connectionso that we can freeconn->initialafter the handshake: lrstewart@0754e5cWe reference
conn->securequite a lot in our code, often to access the cipher suite (conn->secure.cipher_suite). That means that if we make it a pointer, we have to update a lot of references toconn->secure.withconn->secure->. Apparently 93 files worth.To avoid drowning the upcoming actually interesting changes, I'm splitting the uninteresting changes of just finding and replacing "." with "->" into this PR. To do that, I just point the new
conn->initialandconn->securepointers at memory still allocated on the s2n_connection structure.conn->secureshould never be null (we'll never free it) but I addedENSURE_REF(conn->secure)everywhere outside of the tests where we dereferenced it just in case. This was manual, and involved a lot of files, so you may need to just take my word for it and accept that if I missed one it's not a huge risk.The only non-find/replace changes:
tls/s2n_connection.c
tests/saw/spec/handshake/handshake_io_lowlevel.saw
Callouts
conn->securea pointer if we never free it? To keep the code simple. If we need to manageconn->initialandconn->securedifferently, it could get complicated and open us up to bugs. Just look at the current logic for managing s2n_crypto_parameters :(Testing:
Existing unit test pass. I didn't make any changes to the tests beyond fixing all the references to conn->initial and conn->secure.
I unfortunately had to fix the SAW test: https://github.com/aws/s2n-tls/pull/3469/files#diff-1fded1e92cdd5ab7f968a9154528b59efbd04a25de8f004cd431abb10477630d
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.