Type construction safety: unsafe struct / unsafe drop

1

It seems there is currently no way to express that construction of a value of some type is unsafe due to unsafe operations perfomed in its Drop implementation.

An example of such type is MutexGuard - it performs an unsafe operation on drop (mutex unlock), ie. creating a value of this type (referencing some mutex) seems to be inherently unsafe. But there's no way to express that and it can't be enforced that a value should only be constructed in unsafe code.

In practice this is kind of solved by having private fields in the guard so the type can't be instantiated outside of its module. But that feels more like a workaround and also the code inside the module isn't protected. Edit: As resolved in the thread, this is in fact the right approach.

I suppose to express this one would need syntax along the lines of

unsafe struct MutexGuard {
    ...
}

Has this been discussed in past? (I couldn't find anything...)
What are your thoughts?
Thanks :slight_smile:

2

To me "public, but fields are private" seems like a reasonable and already canonical solution to this sort of thing. I'll grant that it's not obviously the best or most natural solution to this specific example, but I'm not aware of anyone ever proposing an alternative that'd be any better.

Any approach to dealing with unsafe-to-construct types is going to involve publicizing some special functions/methods that construct it safely, which means having the type somewhere in the signature (as return types or "out parameters" or whatever), so the type itself has to be public to appear in their signatures (assuming we agree the private-in-public lints are at least conceptually a good thing). Field privacy is something we need for plenty of other reasons. Those seem like inevitable consequences of any language wanting to have both a module/privacy system and unsafe code, and together they already almost automatically provide us with the "public with private fields" solution.

This part sounds confused to me. If the unsafe code is inside the MutexGuard module, and other modules can only run it via the safe public functions, isn't it already being enforced that "construction" happens only inside the unsafe code? (assuming the public API is sound)

Or are you suggesting that every let x: MutexGuard<i32> = ...; statement should be inside an unsafe block, no matter how safe the wrappers invoked in the ... are? (if so, you can obviously just not provide any safe public constructors, but why would you want to create a type like this?)

This aspect is not unique to construction/destruction. It's well-known that if a module contains any unsafe code at all, then all of the safe code in that module is also potentially important for safety. The classic example is some safe code in std::vec randomly setting a Vec's len or cap fields to bad values.

AFAICT, this is a specific example of much more general issues around the basic meaning of unsafety and its interaction with modules that have been widely discussed and currently aren't sources of controversy, but admittedly that's pretty abstract so it's worth checking that I haven't missed something obvious here.

3

Better answer: It finally hit me that the stuff in my post I probably first learned from the Rustonomicon. Specifically, the page that's now at Working with Unsafe - The Rustonomicon spells out "the canonical Vec example" I was thinking of and then sums it up with:

Because it relies on invariants of a struct field, this unsafe code does more than pollute a whole function: it pollutes a whole module . Generally, the only bullet-proof way to limit the scope of unsafe code is at the module boundary with privacy.

However this works perfectly . The existence of make_room is not a problem for the soundness of Vec because we didn't mark it as public. Only the module that defines this function can call it. Also, make_room directly accesses the private fields of Vec, so it can only be written in the same module as Vec.

It is therefore possible for us to write a completely safe abstraction that relies on complex invariants. This is critical to the relationship between Safe Rust and Unsafe Rust.

1 Like
4

I'm a little confused by what you are asking for? The construction of MutexGuard itself is unsafe.

https://doc.rust-lang.org/src/std/sync/mutex.rs.html#417

The boundary of unsafe is indeed the module, otherwise, there couldn't be safe methods relying on other unsafe components. As an example, all safe Vec methods must rely on all unsafe methods manipulating the capacity and the length right, as well as reallocating properly.

5

No, what I meant was that the MutexGuard { ... } syntax that creates a value would require unsafe.

I mean sure, the constructor function for MutexGuard, ie. MutexGuard::new() is unsafe (and mutex unlocking is unsafe too) but it's not that clear why they are unsafe since you can just create the MutexGuard value directly without unsafe and by dropping it you can also call the unsafe mutex unlock without unsafe.

I guess the role of unsafe in this case is more documentary, code-readability type of thing.

Well maybe you could also have struct fields marked unsafe whose modification would require an unsafe block :slight_smile: Sorry, that's probably a little bit over the top. Still, I find it interesting to explore the definition / scope of unsafe a little bit.

In any case, that Rustonomicon link puts it quite clearly that the boundary is the module API. I guess that answers my qustions. Thanks for the link.

6

Is that a big problem? I think it's fair to say that inside a module all bets are off, and that's OK as long as the type exposes a safe interface outside of the module.

For example, Vec.len field is totally unsafe to modify. Should it be unsafe struct Vec or have something like struct Vec { unsafe len: usize }?

7

This sounds confused again. You can't construct it directly because it has a private field. If it was possible to directly construct a MutexGuard in safe code, without going through any of the safe wrappers, that would be a serious soundness bug in the standard library, not a false negative in a lint or outdated documentation.

Also, this is why Drop::drop() is a safe function. It would be just as serious a soundness bug if the MutexGuard destructor was not safe to call (that its implementation includes an unsafe method doesn't change this; the public API still has to be safe).

Incidentally, MutexGuard has had at least one major soundness bug before, and it got fixed.

If you're trying to ask "why is Type { ... } not unsafe when Type has invalid UB-causing values?" then the answer is "because the { ... } is not where the UB is caused." It's essentially the same reason you can move and copy raw pointers in safe code, while only dereferencing is unsafe: Copying and moving alone can never cause UB, and there may be modules that use only those operations so it'd be unreasonable to force them to write unsafe.

FWIW, "unsafe fields" are not crazy at all, and have been discussed extensively in the past: Unsafe fields · Issue #381 · rust-lang/rfcs · GitHub

My impression is that they never made it into the language simply because everything you could do with them is already possible today, and it's not yet clear whether adding a different way of doing those things would be a net win. Maybe after we have clearer rules for unsafe code it'll be easier to evaluate the arguments in those old threads.

3 Likes
8

I meant inside the module.

Yes, but then again the same is true of MutexGuard::new() and the the raw mutex unlock method, and yet both of those are unsafe.

Interesting...

9

The key point here is that the unsafety barrier is the module. As much as it would be convenient to scope all potentially unsafe code within an unsafe { block }, the reality is that unsafe code necessarily ends up relying on the correctness of some safe code.

This means that now and forevermore, any unsafe invariants in a module need to be upheld by every bit of code in the module, whether safe or unsafe. If you want to scope it smaller, create a micro module for just the struct definition and unsafe accessors, then re-export it from the parent module.

2 Likes
10

Hm? It's the other way around, the safe code relies on the correctness of the unsafe code in the same block.

11

That's what I'd thought too when I started the thread, but as it turns out, it goes both ways. In the cases mentioned above the correctness of unsafe functions / blocks relies on some other safe code in that module not messing up invariants.

12

Not always. Take for example Vec. Safe code inside the module could write to the len field, which could cause for example indexing to perform out of bounds access of memory.

1 Like
13

That's the whole idea about unsafety in Rust: we don't (because we can't) remove unsafety, instead we encapsulate it. Encapsulation is an even more general technique that is useful for enforcing and upholding invariants other than those concerned with memory safety. That is in fact the point of visibility modifiers.

We don't need a "better" solution to a problem that does not exist.

2 Likes
14

I have acknowledged in the thread above the module being the boundary of unsafety. I have now also edited the original post to reflect this.

The "problem" (quotes because it's questionable whether it's actually a problem) is from my point of view now that the unsafe keyword is a lot more vague than I'd thought it was, since code in an unsafe function/block isn't necessarily actualy unsafe and conversely safe code isn't necessarily actualy safe if it is inside a module where unsafe is also used (such as with the Vec's len field).

I'm not sure if this "problem" should be fixed and how it would be done, but ideally it would be more clear that editing a code inside modules using unsafe is in fact potentially unsafe even outside of unsafe functions/blocks, ie. without having to search through all the module's code to see if unsafe is used.

I'm considering adding some clear comment at the top of my modules which use unsafe.

15

The trick I always use is

Documentation, documentation, documentation!

If there are module-wide assumptions, it makes sense to document it at the top of the file. If there are type-encoded invariants, the definition and its fields should have # Safety sections to their docs listing out what conditions they need to fulfill.

You can add extra typed safety on top of this by using safe getters and unsafe setters (and mutable getters) instead of direct field access where such is possible (i.e. no pattern matching over fields). If you put the definition in a submodule and re-export it, then you cannot access the fields except through the accessors, which brings the safety to the type level.

3 Likes
16

Yeah, I've done this with a custom lock guard recently. I'm still not sure whether the extra code is worth the extra safety :slight_smile: I guess I'll sleep on it and see... But in any case, thanks for the hints!

17

Yes, I would go a bit further and say unsafe wasn't the best way to show what is happening. unsafe plays two roles, when applied to function and trait definitions, it states that they are unsafe to call/implement. When applied to a block, it says trust me, I've checked this code. Very different meanings. I would have opted for two different words for this. unsafe is fine for function and trait definitions, but something like trust_me, checked, or verified would have been nicer for blocks.

This way unsafe means that the function or trait has some additional invariants that must be ensured before calling/implementing. Just one meaning. Having a verified block means that you have done the necessary checks to maintain said invariants, as to ensure soundness.

Now, this is exactly what unsafe means right now, just that the two different meanings are conflated.

The implication here is that having an entirely safe crate, that has 0 unsafe blocks and only has unsafe functions/traits (which do not call any unsafe functions or do anything intrinsically unsafe like raw pointer deref) means that none of those unsafe functions will actually induce unsoundess. This is because all of those functions are technically sound* no matter what inputs you give them. And there are no implementations of unsafe traits, so there is no way to go wrong there either.

But, as soon as you introduce a single verified block, anything that can influence this verified block is suspect. This could be as limited as a single function, or as wide as a whole crate, usually no more than that.

All this being said, it is far too late to introduce verified or anything like it. We will just have to document the double roles that unsafe plays.

* note: even if it is technically sound to call these functions, you shouldn't before reading the documentation and following the safety guidelines in the docs. If there are no safety guidelines, then you aren't allowed to call the function, because there is no way of knowing what inputs it is safe on, (as that could change in the future, introducing unsoundness in a patch update)

18

Do remember that unsafe also tells the compiler that the programmer has assumed some of the responsibility for avoiding UB, causing the compiler to accept code that it would otherwise reject with (usually) detailed error messages. In that sense unsafe is appropriate for functions, traits and other blocks.

19

Well, as it turns out, this isn't actually the case (see above). An unsafe block can't (generally) be verified on its own, since it depends on other code (both safe and unsafe) not messing up invariants.

If no additional type-system level measure is taken (such as unsafe fields as discussed previously etc.), then maybe it would make sense to perhaps make the /// # Safety documentation more explicit (ie. maybe have dedicated syntax for it?) and make it mandatory for

  1. unsafe functions
  2. modules in which unsafe is used

Perhaps a measure as simple as making doc comments mandatory for those cases would be enough :slight_smile: Although gathering community support for that would surely be challenging since it would be a breaking change...

20

The /// # Safety documentation section is for publicly unsafe things, to document what the caller's obligations are. It's not about trying to prove to callers that your implementation is sound.

But within the module it does seem reasonable for the safe functions to get comments like "doesn't touch the len or cap fields that unsafe code relies on". That probably depends on the module though, and I don't see much point in a dedicated syntax for comments that lints or rustdoc won't be looking at. Nor can I think of a more precise way of "making them mandatory".

Though at this point it just sounds like we're going back and forth on the same point. An unsafe { } block of code cannot be proven correct without looking at the rest of the module. That's been widely known and understood since well before Rust 1.0 and even proven formally sound. AFAIK the only real questions that have been raised, including proposals like unsafe fields, are essentially different syntaxes atop the same fundamental semantics and mechanisms. It's pretty clear that any workable system of safe vs unsafe code is going to have to rely on some sort of encapsulation boundary.

I feel like your remaining concerns are not so much about technical correctness but more the social aspects of whether this encourages writing unsafe code well. Social engineering ideas have also been discussed in the past so I'll just link to the most relevant posts I have in past threads: