When auditing crates containing unsafe code the general rule is to eliminate it and verify its correctness if the performance hit is too great after removing. I'm suggesting a third step, when the changes aren't massive, to branch with #[cfg(reduce_unsafe)]; such that the user or binary crate author (see link for more information) can decide to reduce the unsafe code. (Not library crates)
Or without debug assertions validating equivalence reduce_unsafe::unchecked; should only be used when the returned type doesn't impl PartialEq or the branches have side effects.
After reconsidering; libraries could raise hints, like binary crates, but shouldn't have the final say. The user should. The idea of the richer tooling is to add --cfg reduce_unsafe and/or -Funsafe-code on a global or per-crate basis.
# Cargo.toml
# if this crate is not forbidden, then
[package.metadata.unsafe_policy]
# default is forbid (when the meta entry is present)
# allow foo to use unsafe code
foo = "allow"
# ask bar to reduce its unsafe code
bar = "reduce"
# forbid bar's dependency 'baz' from using unsafe code
bar.baz = "forbid"
[package.metadata.unsafe_policy.'cfg(feature = "something_which_needs_unsafe")']
quz = "allow"
@jonh This is a good start but my key motivation here is to allow crates to forbid or reduce unsafe code, selectively and globally in all of their transitive dependencies.
Adding a cfg switch adds more code paths, thus increasing complexity. Yes, it allows people to reduce the amount of unsafe code they depend on, but with several caveats:
Sometimes safe alternative implementations are not (easily) available, thus this cannot remove anywhere near all unsafe usages.
Memory-safe code can still exhibit bugs, even security issues. Increasing complexity even increases the chance of bugs.
I don't want to get into the whole Actix thing, but clearly some authors/maintainers care more about safety issues than others, so don't expect this to magically fix your favorite dependencies.
I do think that this proposal by itself doesn't really make any sense. Either the crate author simply doesn't use unsafe, or they objectively need unsafe in an irreplaceable way, or they want unsafe and did all the due diligence to validate that it's sound so there's no benefit to disabling it, or they want unsafe and won't do the due diligence... in which case they probably won't add this cargo feature.
Basically, the same argument as [Pre-RFC] Cargo Safety Rails and Crate capability lists. It's a pretty fundamanetal and general problem with all of the "less unsafe everywhere" proposals we've seen so far.
For this specific suggestion, the only scenario where it would even make sense to me is:
the soundness of some code is fundamentally unclear even if you've done your due diligence, and
there's a safe alternative for whatever the code's doing, and
for whatever reason, the safe alternative is not simply another crate.
Offhand, I'm not aware of a single example of all three. arrayvec is probably worth getting out of your dep tree if you can, but IIUC the separate crate tinyvec already covers the subset of use cases that don't fundamentally require unsafe. Pointer offsetting is probably the single biggest example of "soundness unclear" in the ecosystem today, but it's also famously impossible to do in current Rust without UB anyway, so we're stuck with waiting on a language change there. And so on.
The goal of the --cfg reduce_unsafe is only to signal a preference (which may allow to forbid unsafe in a crate). Nothing more. The cargo wrapper I want extends above this to enable per-crate or global policy enforcement.
An example where this branch makes sense is str::from_utf8_unchecked vs str::from_utf8-then-unwrap. The former relies on unsafe code which may be sound today; but may become unsound tomorrow with some change in safe code. The latter is always sound and may be optimized away entirely; but will panic if it is ever hit. I'd also advise users of _unchecked to use debug assertions on the unsafe branch to panic on debug builds - where the performance penalty of checking isn't a concern.
So yes, the cargo wrapper does go in the style of crate capability lists; which I want; thanks for those links BTW. However, at least for now I'm coming from an auditing and code review direction; not forced tooling.
As for the increased complexity; I'm not advising complex branches. This hint should be used only when the branching is simple enough. Debug builds could even compare the branches for equivalences in their outputs if there aren't other side effects preventing this. This helps reason with the unsafe code when it is used.
@josh Without the --cfg reduce_unsafe present this makes absolutely no difference to the branching or resulting compiled code. Even if you use debug assertions in the unsafe code (please do) you won't hit them in release builds. With the hint we're only asking to prefer safety/panics over unsafety/performance. This doesn't mean less testing; the reverse is true when using the debug branch verifies they compute the same result (likely in different time).
I'm not talking about runtime branches. I'm talking about compile-time configuration options using cfg: having two different configurations adds complexity, and drastically reduces how much testing the non-default configuration gets.
The default here is to allow the unsafe code, exactly unmodified. My suggestion is to allow users to diverge from the default, in favor of a safe implementation which must produce the same result. Running cargo test would cover both branches; not one (unless the unchecked variation of the macro is used).
Edit: Re-read your message. Yes. This is additional code which hits a less tested code path; but so is any minority function or feature. If this is a problem, fewer branches doesn't help, you need coverage tests.
As a library author I'd be wondering how much slower can the non-unsafe code be. Sometimes it's not a simple question, e.g. should lock-free data structure start using locks? Is it OK to change O(n) algorithm to O(n^2)?
Looking at usage of unsafe in my libraries, I struggle to find cases where I could transparently provide alternatives:
FFI
transmuting of pointers and creation of slices in order to offer zero-copy API. To make it safe I'd have to return Vec/Cow instead of &[], but that's a visible API change.
unsafe impl Send for a struct that contains Rc (which I can't remove), but which I know won't be used from another thread.
UnsafeCell to implement lazy initialization. It's again API change to avoid it.
@Kornel The first step of unsafe code reduction is elimination. Try to write the safe code.
If the safe code is too complicated or too slow, then it might not be worth using. But don't forget about your attempt, put it in a branch and let it bitrot until someone (maybe you) wants to try again.
If the safe code is, as in my example, not too complicated but is measurably slower; then try to verify the unsafe code is sound and finally consider using the cfg-flag or macros. When using the checked variant you're verifying the unsafe code matches the safe code for all inputs you or your consumers throw at it during tests.
I wouldn't use this to decide between aes-soft and aesni or vastly different data structures. At this point just use the right crate.
This proposal is for crate authors and auditors to consider instead of discarding the safer variants when the safer variants are suitable, just slower.
What I'm trying to say is that safe and unsafe code may end up needing a different API, e.g. zero-copy unsafe code vs copying safe code. In Rust owned and borrowed types are not interchangeable.
Plus many of my uses are not because of performance, but to make the code compile at all. Slower-but-safer option doesn't exist for them. "Safe Rust" is not like Rust, but slower. Turing equivalence aside, "Safe Rust" is for all practical purposes a smaller, restricted, incomplete language.
Given a choice between unsafe code that's the default and thus well-tested, and safe code that's not the default and potentially less well tested, I'm likely to choose the former. I'd prefer safe code in general, but not at the expense of maintenance burden and scrutiny and testing.
I have a hard time imagining a case where making that feature flag user-visible allows the consumers of the library to make a meaningful, informed choice.
I do like the idea of a formalized way to write safe code "specifications" for unsafe code blocks. Aside from the main use case mentioned here, this would be very useful for fuzzing and property-based testing.