Audit use of unsafe in uri/mod.rs #417
Conversation
The tests include an example of invalid UTF-8 which is an error.
This test targets and invalid UTF-8 byte after valid bytes for a uri scheme.
The refactoring simplifies the handling of the authority component of a Uri by eliminating code duplication.
The two uses of unsafe rely on postconditions in other functions which are also documented to show this reliance.
The nightly build was failing because of a know regression concerning the use of the new "unused_braces" lint in doc tests.
This reverts commit c7bc09b.
| return Ok(Uri { | ||
| scheme: scheme.into(), | ||
| authority: authority, | ||
| path_and_query: PathAndQuery::empty(), |
There was a problem hiding this comment.
This used to just assume empty(), and now its calling parse(s)?, right?
There was a problem hiding this comment.
Not exactly. It used to assume PathAndQuery::empty() and now it is calling PathAndQuery::from_shared(s) where s is an empty Bytes. We know that s is an empty Bytes because we test that authority_end equals the whole length of s at line 821 (of the changed file) and then call s.split_to(authority_end) at line 831. This latter call becomes (in this case) s.split_to(s.len()) which results in s being left empty.
PathAndQuery::empty() and PathAndQuery::from_shared(s) where s is empty both produce a PathAndQuery with data the equivalent of ByteStr::new() and query as NONE (the sentinel value declared in uri::path).
Logically this change still produces the same result in the scheme.is_none() case.
Performance wise, this particular case isn't covered by any of the existing benchmarks in benches/uri.rs. Although PathAndQuery::from_shared(s) in general is linear in the size of s, when s is empty this a constant operation, though this doesn't answer the performance question without having benchmarks.
If there are any concerns about the performance implications of this change I would be happy to add a benchmark to benches/uri.rs to cover this case.
Added tests for attempts to parse various types of invalid
Uri's including ones with invalid UTF-8 bytes in them. Added a test for parsing&[u8]as a Uri where it has invalid UTF-8 bytes in the fragment. This test accepts theUrias valid becauseUri(currently) does not expose the fragment so those bytes are never interpreted as a&str.Refactored the
parse_full()function to eliminate some code duplication and thereby simplify the function. Finally, added comments toparse_full()and the functions it calls to document the postconditions thatparse_full()relies on to make its use ofunsafesound.This PR has a weak discrepancy on #414 and #416 in the sense that some of the comments added in this PR make more sense in light of the comments added in the earlier two PR's. There is no dependency in this PR on the earlier PR's to build or to run tests.
This is a part of #412.