Clarify GCM FFI code #970
Merged
Clarify GCM FFI code #970
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
briansmith
force-pushed
the
b/clarify-gcm
branch
from
125c3a8 to
af57522
Compare
This was referenced Apr 29, 2020
briansmith
force-pushed
the
b/clarify-gcm
branch
from
af57522 to
210d70a
Compare
Rust's pointer aliasing rules are tricky. Avoid dealing with them in this code. I remember feeling like I needed the `key_aliasing` pattern when I wrote the code as it was, but it seems like I could/should have done it this cleaner way even back then.
briansmith
force-pushed
the
b/clarify-gcm
branch
from
210d70a to
52cedc2
Compare
BoringSSL has changed such that it uses a different definition of `GCM128_KEY`. Avoid any confusion regarding that by avoiding the name `GCM128_KEY`. Looking at how `GCM128_KEY` is actually used in the code, this actually makes the code clearer anyway, even if there weren't potential for confusion with BoringSSL's changes.
It has always been confusing that *ring* uses a different definition of `GCM128_CONTEXT` than BoringSSL. Clarify the situation by avoiding the `GCM128_CONTEXT` name. Also remove the already-unused C definition of `GCM128_CONTEXT`.
Clarify what the key initialization functions actually do: They calcualte the H table from H. Now the `Key` type exposed from the module isn't `repr(transparent)` which helps with encapsulation as we can now guarantee that nothing outside the module uses the contents of the key by passing it to non-Rust code.
Stop marking `Context` as `#[repr(transparent)]`. Change the remaining declarations of non-Rust functinos to use the `ContextInner` type instead.
briansmith
force-pushed
the
b/clarify-gcm
branch
from
52cedc2 to
b03125a
Compare
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.
Clarify some pointer aliasing in gcm.rs by removing the questionable
key_aliasingpattern. It was much easier to remove it than to figure out if it is guaranteed to work. Clarify how keys and contexts are passed to the non-Rust GCM code. Although these changes are justified on their own, they also make merging upstream BoringSSL changes easier. The individual commit messages each contain more detailed explanation.