Audit use of unsafe in header/name.rs #428
Closed
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
The use of MaybeUninit to replace the depreciated mem::uninitialized() is supported because the minimum supported version of Rust is 1.39. This change moves most of the uninitialized memory related use of unsafe into the parse_hdr() function.
The general pattern follows the "Initilizing an array element-by-element" example in the documentation of MaybeUninit. This change removes some unnecessary use of unsafe.
This introduces a dev-dependency on the criterion crate. The new benchmark for HeaderName uses different standard header names of increasing lengths.
A previous commit had extracted the comparison of a presumed-initialized MaybeUninit<u8> and a static u8 to its own function. While clearer, this lead to a performance regression so this commit manually inlines this method again. This restores most (but not yet all) of the performance that regressed.
Move the eq! macro into the parsh_hdr() function which locallized all of the unsafe code related to MaybeUninit.
The comments describe the pre-condition and post-conditions on the various different parts of the two parse_hdr() implementations that combine to make the use of MaybeUninit in that function sound. The process of assessing the soundness of the use of MaybeUninit also included manually checking that the number of parameters to each all eq!() invocation matches the number of bytes initilized by the immediatly preceeding invocation of to_lower!() (which is necessary to avoid undefined behavior). To avoid being overly repetative the general pattern that assures soundness is documented in the comments but not each instance of that pattern. Each instance, though, was checked.
The comments document the invariant, preconditions, and post-conditions that together ensure that the use of unsafe related to UTF-8 assumptions (in calls to ByteStr::from_utf8_unchecked()) are sound.
|
@seanmonstar It's been a little while since this PR was opened. Is there anything community members can do to move this PR forward? |
jeddenlea
added a commit
to jeddenlea/http
that referenced
this pull request
Apr 1, 2022
…o constname_merge Merges hyperium#428 into hyperium#499
Closed
jeddenlea
added a commit
to jeddenlea/http
that referenced
this pull request
Apr 6, 2022
…o constname_merge Merges hyperium#428 into hyperium#499
jeddenlea
added a commit
to jeddenlea/http
that referenced
this pull request
Apr 15, 2022
…o constname_merge Merges hyperium#428 into hyperium#499
|
Merged in #555 (thanks so much!) |
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.
This PR documents and improves the use of
unsafein theheader::namemodule. That module has two kinds of usages ofunsafe: the use of uninitialized memory and calls toByteStr::from_utf8_unchecked().The uninitialized memory in
header::nameis used as a temporary buffer when normalizing the bytes that represent a name before matching them against the standard header names. This PR upgrades the uninitialized memory from the deprecatedmem::unititialized()to an array ofMaybeUninit<u8>. This upgrade has the useful side effect of mostly localizing the use ofunsaferelated to uninitialized memory to the two implementations ofparse_hdr().The PR documents (through comments) the preconditions and post-conditions that make the use of uninitialized memory sound. The performance sensitive nature of
parse_hdr()make these comments harder to follow than they might have been had it been possible to refactor parse_hdr() without causing a performance regression (I started such a refactoring but backed off when it caused regressions). The documented preconditions and post-conditions show the reasoning on how eachMaybeUninit<u8>is initialized before it is accessed for reading and before it is used as a part of the return fromparse_hdr().The PR also document the preconditions, post-conditions, and invariant that make the use of
unsaferelated toByteStr::from_utf8_uncheckedsound. These comments are also more extensive than they might have been because of the performance sensitive nature ofparse_hdr().Finally the PR adds a new set of micro-benchmarks related to parsing the standard header names.
The new benchmarks make use of the criterion micro-benchmarking framework. This adds a new dev-dependency which is a major downside of this change. I suggest, though, that this downside is offset by the following upsides:
This is part of #412.