How (not) to use unsafe code


#1

This reddit discussion is picking up steam. I’m sure there are folks here that read the Rust subreddit but don’t participate in discussions there, like myself (I don’t like the UX of it, personally). So, I figured having a (parallel but separate) discussion here can’t hurt.

So, it’s a bit unfortunate that the crate in question is actix-web, but this can happen (perhaps all too easily, depending on one’s background) in any crate. It is fortunate in that a web framework facing the open internet cannot cut corners when it comes to safety, and so better to discuss (and hopefully fix) these issues now rather than when a publicized exploit occurs.

I’m speculating, but I think a lot of this has to do with designing code with the borrow checker in mind, which takes quite a bit getting used to (and still occasionally hard/unnatural/unergonomic once layers of abstraction pile up and design “mistakes” become more apparent). It’s easy to just punt to unsafe and hope things line up properly, but that’s a shortcut that will very likely come back to bite - hard - once even more code is layered on and the safety invariants get lost in the maze of code.

The upside is this topic will now get (more) attention because a popular and well-received by the community crate is fielding criticism.


TWiR quote of the week
#3

I guess it’s better that it has come up now, rather than after finding some vulnerability :slight_smile:


#4

I’m aware this is the users forum, not internals, but a few topics on the thread are talking about auditing unsafe code. I wonder if adding an attribute to mark why uses of unsafe are actually safe could force authors to think a bit more about it by documenting their thought process, and reassure future maintainers and users that the writer knew what they were doing. E.g.

#[safe(reason = "Bit layout is the same, so safe to transmute")]
unsafe {
    let my_number = mem::transmute::<[u8; 4]; u32>(my_array);
}

This could be optional for normal use, but could be turned on to deny not marking every use of unsafe with one as a lint (or whatever the correct terminology is) for crates that care about safety.


#5

I’m 99.9% sure there’s been an RFC (or maybe a pre-RFC) for exactly this before - let me see if I can find it…

EDIT: Aha! https://github.com/rust-lang/rfcs/pull/1910


#6

That doesn’t seem like a bad idea although:

  1. A lot of cases will require more than just a sentence or two to explain - that may get visually unwieldy over simply having code comments.
  2. As far as it being an extra speedbump to avoid needless unsafe, I suspect someone will just plow through this anyway. The actix-web code appears to have some instances where existing lints were turned off, both in rustc and clippy, so the use was very much going to happen regardless :slight_smile:.

#7

(Trying not to repeat others.)
“How (not) to use safe code” is also perfectly appropriate. Safe only goes so far and programmers should not get fooled into thinking only the unsafe code needs review. (Failing to parse input main thing on mind but not the only case that can cause code state to become invalid.)

I view it good to use features that default to safe but when enabled switch to more performant code.

RefCell is safe way for runtime borrow but in the case of breaking a borrow with unsafe; the code in question could be paired with some custom eg Arc<Atomic> structure that can add extra runtime check. Comments in code can only state intent rather than enforce.


#8

An alternative could be to mandate a doc-comment above unsafe blocks, though that would require defining what putting a doc-comment on a code block means.


#9

Some canonicalization around explaining unsafety internal to a crate wouldn’t hurt, but that’s somewhat orthogonal. It won’t stop anyone, of course.

I’m personally most interested in why actix-web chose to use some much unsafety in the first place, seemingly recklessly in places, including disabling existing lints along the way.

It would be nice for crates.io (and maybe cargo integration) to somehow show an “unsafety barometer” for a crate so someone considering using it has an inkling as to what’s in there. That unsafety may be documented (or not), but that’s a secondary issue. If I, e.g., see an http lib rife with unsafe use, it’s going to raise questions. If, on the other hand, I see something that sounds like it should have unsafe code, then it’ll make more sense (no guarantees it’s not reckless, but that’s a separate issue).


#10

Keep in mind that unsafety of unsafe blocks is not equal, and not even proportional to their length.

I use unsafe mainly to to call FFI functions. It would be painful if every FFI call needed even more ceremony.

Actix seems to have intentionally used unsafe to shut the compiler up, even disabled clippy warnings for some code. So I think just more compiler warnings wouldn’t help, they’d just work around/silence more roadblocks.


#11

There has been previous discussion about marking crates as “unsafe”. I think this is a delicate issue that can’t be solved with something simple like counting lines or occurrences. It would need at least a static analyzer to distinguish between good and reckless uses of unsafe.

It could backfire if people felt they had to write pure safe Rust code or have their crate marked as bad.


#12

I don’t think we can ever really know, but one of the things that makes UB so insidious is that even if the behavior of your program is undefined, it can still actually appear to work exactly like you expect it to. From my own personal experience, I get the feeling that for some people, this is, in and of itself, enough, regardless of how many roadblocks we put up.

In some cases (maybe for actix-web, but I don’t know), I suspect the only effective road block would be adoption itself. If abuse of unsafe is noticed immediately, then it’s possible that adoption/attention might have never materialized because of that. While abuse of unsafe is unfortunate, what makes it truly bad is when it actually starts getting heavily used by others.

With respect to surfacing the visibility of unsafe, I don’t know what the right answer there is. It could be anything from automated tools that report metrics to just developing a stronger culture around auditing code manually, or perhaps something in-between.


#13

We can probably get fairly close because, AFAIK, all the actix projects are essentially one author, and he frequents this forum as well :slight_smile:. I’m hesitating in cc’ing him because I’d rather this be an educational conversation, and not a unsafety witchhunt.

This instance is particularly interesting because actix is a non trivial project, and has had success in gaining users (with overwhelmingly raving reviews from their standpoint, from what I’ve seen). It’s done well in performance benchmarks. The person(s) behind it clearly know what they’re doing (leaving the unsafe stuff aside). And yet there’s rampant unsafety.

Why was it difficult (or impossible? or what?) to write the same functionality in safe Rust? Is it a performance issue? Is it too difficult to design a complex app with constantly keeping borrowing in mind? Were these just initial shortcuts that were never removed later? Something else?

I think (a) figuring out why that is and (b) possibly addressing it is just as important for moving Rust forward as catering to Rust newcomers and other initiatives that, to one degree or another, have been questioned recently.

I don’t know either, although I think we can all agree with some elements of what it would look like (implementation difficulties aside).


TWiR quote of the week
#14

This seems like it could naturally pair with #[safe(...)] (or some approximation) and audit tools. That could go some ways to addressing @kornel’s concerns:


#15

What if author created unsafe!() macro that auto inserts #[safe(shut up)]? Or just put a message like “because I need a mutable pointer here”.

In actix case the author intentionally silenced clippy warnings, so probably the author strongly believes they’re not correct, not relevant or there’s no other way. So it’d expect them to do the same for “unhelpful” #safe() annotation.


#16

Could a proposal similar to the following (previously posted to another thread in internals) help in this regard: https://internals.rust-lang.org/t/pre-rfc-another-take-at-clarifying-unsafe-semantics/7041/31?u=gbutler


#17

I think if I were to read/see such in the #[safe(...)] I would be reasonable in assuming that the author cared not about the safety or correctness of their code and that I could categorically ignore using the Crate on the grounds that the author has demonstrated a lack of care regarding safety.


#18

The purpose of #[safe(...)], in such a regime, would be to key into a formal audit trail. Alone, it wouldn’t do anything beyond document the author’s intent. Its power is in combination with a formal review.

Extending @IsaacWoods’ example:

#[safe(reason = "Bit layout is the same, so safe to transmute", review="5550")]
unsafe {
    let my_number = mem::transmute::<[u8; 4]; u32>(my_array);
}

review="5550" would key the unsafe block to a review system. The specific semantics are debatable–is this a GitHub issue, a code-reviewed pull request, a signed commit, a crates.io report, etc–but the idea is to pair unsafe statistics with an audit trail.


#19

All of you make great points. While a safe marker certainly wouldn’t prevent reckless unsafe usage, it would help auditors figure if it was warranted.

I also think #[safe(..)] - while succinct, would encourage end users to write incredibly short reasons (which could backfire and make unsafe usage rather confusing). Perhaps a special marker or keyword can be introduced into the markdown documentation? Or perhaps marking something that is #[safe] would require some form of documentation above it?

BatmanAoD also makes some great points against this below.


#20

I don’t really see the value of this, and I think that if the community ever comes up with a “good” solution, legacy #[safe(.... lines would just be noise.

This is important, I think, because I do believe that it is possible to find a good solution. In particular, I think that any robust solution would have some kind of precondition/postcondition logic. This is quite complicated; in particular, it would require:

  • Some kind of formal language for preconditions and postconditions
  • Runtime and/or compile-time checking of conditions
  • Primitives for interfacing with code and/or compiler internals (e.g. IsaacWoods’ “Bit layout is the same” condition can’t be checked within the language itself)

I expect that the RustBelt project will necessarily develop something like this as a compiler extension, at which point the Rust team can start to consider adopting parts of their language into the core language.

In short, I believe that there is a path forward on unsafe annotations that we could eventually use for linting purposes, but it is a long and arduous path, and if we take a shortcut like #[safe(reason = "some arbitrary block of text")], I’m not sure what we would be gaining, and I think we would actually be adding noise.


#21

The review= syntax might actually be okay. I would actually eliminate the reason= bit; I don’t think there’s any reason to make it “appear” that the compiler is actually going to be doing something interesting with the annotation when in fact there’s no realistic way for it to do so.