search

hyperium / http

Rust HTTP types
Apache License 2.0
1.15k stars 285 forks source link

Audit the use of unsafe in header/value.rs #419

Open sbosnick opened 4 years ago

sbosnick commented 4 years ago

Reorganized the Debug impl for HeaderValue to make the invariants that its use of unsafe rely upon for soundness more obvious. This also adds a few more test cases for edges cases of the Debug impl. This also adds comments to all of the uses of unsafe in header/value.rs to explain and document their soundness.

One of the uses of unsafe in this file is in the function from_maybe_shared_unchecked() which is part of the public API of this crate. Currently this function is an unsafe function, but if unsafe functions are intended to signify caller responsibility for preventing undefined behaviour then this function is safe. There are no parameters you can pass to from_maybe_shared_unchecked() that will cause undefined behaviour.

There are, however, parameters you can pass to from_maybe_shared_unchecked() that would create a HeaderValue that did not comply with RFC 7230 (section 3.2). That is, a HeaderValue that contained (for example) a NUL byte (0x00). If unsafe functions are intended to signify caller responsibility for maintaining some precondition (not just avoiding undefined behaviour) then it may make sense to keep from_maybe_shared_unchecked() as an unsafe function.

Removing unsafe from from_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 unsafe functions to use and ~may~ make further changes to from_maybe_shared_unchecked() (I can at least improve the doc comments if the unsafe should remain).

This is part of #412.

sbosnick commented 4 years ago

The latest commits (specifically https://github.com/hyperium/http/pull/419/commits/6364f795fb860e959d7133688f2e797be148ae57) make an opinionated change to marking from_maybe_shared_unchecked as an unsafe function. Specifically it removes the unsafe from the function definition.

As described in more detail in in the commit message for https://github.com/hyperium/http/pull/419/commits/6364f795fb860e959d7133688f2e797be148ae57, this is both sound and is not a breaking change.

This change is taking the design view that unsafe function in the public API should be restricted to functions where the caller bears responsibility for preventing undefined behaviour and not as a general means of indicating caller responsibility for ensuring that API constraints unrelated to undefined behaviour are met.

I am making this change to have a concrete proposal for consideration. If there are reasons to prefer the other approach to unsafe functions in the public API then I can change this back and explain the constraints (and their relationship to avoiding undefined behaviour) through comments.