Audit the use of unsafe in header/value.rs #419
Conversation
This file contains hidden or 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
Document the sound use of unsafe in HeaderValue::to_str().
The tests inlcude variation on the placement of unicode codepoints that encode to multi-byte UTF-8 and an instance of a [u8] with invalid UTF-8 that is still a valid HeaderValue.
The new code uses Iterator::try_fold() instead of external iteration to review the bytes in the HeaderValue and separate the printing of visible ascii bytes printing other bytes (and special casing b'"'). This new implementation may behave slightly different for a HeaderValue of length usize::MAX, but such a HeaderValue is likely to exhaust memory before the Debug impl comes into play (which also makes it hard to test this hypothetical edge case).
The comments describe the invariants that make the use of unsafe sound.
The simplification removes the need for one of the invariants that described how the use of unsafe is sound.
HeaderValue::from_maybe_shared_unchecked() is currently an unsafe function but it could be made a safe function without intoducing a soundness hole because it does not do anything directly that depends on `unsafe` and it isn't protecting any invariants of HeaderValue that are relied upon later when using `unsafe`. from_maybe_shared_unchecked() can accept bytes as a HeaderValue that are invalid bytes for what is described in RFC 7230 (section 3.2) but all such bytes would still be valid to convert directly to a str since they are all valid single-byte UTF-8.
9 tasks
HeaderValue::from_maybe_shared_unchecked had been marked unsafe but this was to indicate caller responsibility for api constraints and not caller responsibility for avoiding undefined behaviour. This change removes the use of unsafe for this purpose and instead uses a more explict doc comment to draw the caller's attention to the restrictions (as well as the continued use of the "_unchecked" suffix. This is not a breaking change (despite affecting the public API) because removing "unsafe" from a function still allows existing uses to compile. This use of "unsafe" was not protecting an invariant on the internal structre of HeaderValue that was relined on in unsafe blocks elsewhere in the module to ensure the absence of undefined behaviour. All (other) unsafe blocks in this module rely on local checking to ensure they avoid undefined behaviour.
Some of the comments describing the invariants necessary to ensure the sound use of "unsafe" refered to an older version of the reasoning that had used 2 invariants. This change makes them refer to "the invariant" instead of "invariant 1".
|
The latest commits (specifically 6364f79) make an opinionated change to marking As described in more detail in in the commit message for 6364f79, this is both sound and is not a breaking change. This change is taking the design view that I am making this change to have a concrete proposal for consideration. If there are reasons to prefer the other approach to |
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.
Reorganized the
Debugimpl forHeaderValueto make the invariants that its use ofunsaferely upon for soundness more obvious. This also adds a few more test cases for edges cases of theDebugimpl. This also adds comments to all of the uses ofunsafein header/value.rs to explain and document their soundness.One of the uses of
unsafein this file is in the functionfrom_maybe_shared_unchecked()which is part of the public API of this crate. Currently this function is anunsafefunction, but ifunsafefunctions are intended to signify caller responsibility for preventing undefined behaviour then this function is safe. There are no parameters you can pass tofrom_maybe_shared_unchecked()that will cause undefined behaviour.There are, however, parameters you can pass to
from_maybe_shared_unchecked()that would create aHeaderValuethat did not comply with RFC 7230 (section 3.2). That is, aHeaderValuethat contained (for example) a NUL byte (0x00). Ifunsafefunctions are intended to signify caller responsibility for maintaining some precondition (not just avoiding undefined behaviour) then it may make sense to keepfrom_maybe_shared_unchecked()as anunsafefunction.Removing
unsafefromfrom_maybe_shared_unchecked()is an API change but it does not appear to be a breaking change.Before this is merged we have to make a decision on the approach to
unsafefunctions to use andmaymake further changes tofrom_maybe_shared_unchecked()(I can at least improve the doc comments if theunsafeshould remain).This is part of #412.