Private unsafe fields are a poorly motivated feature

[ais523]

I think part of the problem here is that Vec internally uses so much unsafe that it's a bad example – types should be trying to use as much of safe Rust as they can to reduce the amount of unsafe preconditions that they have to check.

Vec could be made much safer than it currently is, using more of the language's existing guard-rails. A Vec conceptually has two fields: a storage field of type Box::<[MaybeUninit<T>]>, and a length field that controls how much of the storage field is initialised. (There's nothing blocking Rust from changing to such a definition of Vec today – although there's a stable guarantee that a Vec consists of a pointer, a capacity, and a length, this layout fits that because &'static mut [MaybeUninit<T>] is a pair of a pointer and a capacity.) Ignore the opsem special case for Box here – it could be overridden if necessary (and will probably be changed anyway).

The invariant on such a Vec is that the storage field is initialised as far as the length, but no further.

When looking at this hypothetical Vec layout, it feels conceptually to me like the length should be unsafe but the storage shouldn't be, although I appreciate that this position isn't entirely consistent, so I'm trying to analyse why I feel that way. I think the reason is that the potentially unsound thing you can do with the storage (assuming it's initialised when it isn't) is an obviously unsafe thing that's already guarded by unsafe requirements (on <[MaybeUninit]>::assume_init_*) – so it's somewhat obvious that "swapping out the storage for a different slice" is an operation that requires care. It's less obvious that changing the length field will break the unsafe assumptions made by <[MaybeUninit]>::assume_init_*, so it's useful to have the language force changes to that field to add a safety comment acknowledging how the invariant is being maintained.

Notably, the pointer and capacity can't possibly get out of sync with each other – in this model, that's being checked by safe Rust's usual type checks and is not something that needs any unsafe assumptions at all. So the capacity is not an unsafe field in this model.

[RalfJung]

@lordan

My question there is: how?

(This was about how misuse of from_utf8_unchecked can cause a memory safety violation.)

See this code.

I leave it as an exercise to craft an input such that misusing from_utf8_unchecked triggers a Miri error (which would conclusively demonstrate that this is truly about UB / memory safety).

[dlight]

I really would like to have robustness in Rust proper. What about a RFC?

This would settle the question on when it's fine for unsafe code to trust third party safe code. And the answer is: unsafe code can trust safe code for UB matters for properties declared as robust (the author of the robust API is really promising that those properties are valid for unsafe code to depend upon, AND that new semver-compatible versions will continue to hold those properties).

This means if some unsafe function triggers UB when depending on a broken robustness guarantee of a safe function, it's the safe function that gets to be blamed for this bug and needs to be fixed. Then, the stdlib could publish its robustness guarantees, and unsafe code can document exactly which guarantees they rely upon, if they wish.

Or rather, the stdlib appears to operate as if their guarantees were robustness guarantees,

@RustyYato

I think the standard library (for types/functions in core or alloc) is a bit special in this regard, most guarantees unless stated otherwise are soundness guarantees. And we can rely on the documentation being correct for soundness. (this is the burden of being std, and a blessing for all of us users of std).

But that's a bit implicit, and other libraries don't do that. Having a way to explicitly indicate a guarantee can be relied upon unsafe code would be useful.

[tczajka]

@dlight

Then, the stdlib could publish its robustness guarantees

To help me understand: what are example guarantees in stdlib that are not robust, which example functions wouldn't be marked robust? And what would adding the keyword to a function signature change in the language? What would be example code that the robust keyword would enable that isn't allowed without it?

[rkjnsn]

I'm not sure I understand how having buf be a Box::<[MaybeUninit<T>]> rather than a RawVec<T> substantively changes things.

@ais523

Notably, the pointer and capacity can't possibly get out of sync with each other

Today, this is handled by RawVec<T>, which handles special cases for zero-size types on top. (No need to keep reallocating for zero-size types, just claim a fixed capacity of usize::MAX.)

@ais523

The invariant on such a Vec is that the storage field is initialised as far as the length, but no further.

I agree with the first part (buf must be initialized as far as the length), but not the second part. It is not a safety invariant that no elements beyond len be initialized. That will merely lead to a leak, not UB. Indeed, the whole purpose of set_len, as I understand it, is to allow initializing values past len, such as via an FFI call, before updating len.

@ais523

it feels conceptually to me like the length should be unsafe but the storage shouldn't be

This is the part I'm still having trouble understanding. A RawVec<T> of any capacity and content (or a Box::<[MaybeUninit<T>]> of any size and content) is valid in isolation. Either can be replaced by an uninitialized allocation of an arbitrary size by safe code. It is only in the pairing of len and buf within Vec that there is an additional invariant that applies to both and relates the two. Thus, it seems to me like both should be unsafe, or neither should be.

@ais523

I think part of the problem here is that Vec internally uses so much unsafe that it's a bad example

My feeling is that it's a bad motivator for unsafe fields for a different reason: it's actually conceptually pretty simple. You have an allocation, buf, a length, len, and an invariant relationship between the two, that the first len elements of buf are initialized. Thus, in the context of the vec module, it is readily apparent, and easy to remember, than any modification of either buf or len needs to be handles with care.

Given a more complex example, where a struct has a lot more going on, and it could be easily forgotten that a specific field has additional invariants that apply to it, I'm of two minds. On the one hand, I could see the appeal of being able to mark a field as unsafe as an added reminder that updating it requires special care, but on the other hand, I can see the argument that it's probably a sign that the struct is trying to do too much, and might benefit from splitting apart the multiple concerns. I think Vec is a good example of the latter, where by having RawVec<T> handle all of the buffer management concerns, it can solely focus on its single added invariant of len elements being valid.

[tczajka]

@RustyYato

TLDR: unsafe is useful because it is rare, and making it less rare by applying it to general invariants/preconditions doesn't actually help ensure correctness.

Using unsafe fields consistently would make unsafe blocks much less rare.

Suppose I implement TwoDigit and, to avoid all the discussion about whether correctness can be assumed by users, I explicitly say that it is "robust" and its correctness can be trusted for memory safety purposes^[1]. And I don't even implement new_unchecked, so it's all safe code:

/// Represents a two digit number robustly.
/// Certified by professionals. It can be trusted for memory safety.
pub struct TwoDigit(u8);

impl TwoDigit {
    pub fn new(n: u8) -> Option<Self> {
        if n < 100 { Some(Self(n)) } else { None }
    }

    /// Returns the top digit in 0..10.
    /// This is a robust guarantee.
    pub fn top_digit(self) -> u8 {
        self.0 / 10
    }

    /// Returns the bottom digit in 0..10.
    /// This is a robust guarantee.
    pub fn bottom_digit(self) -> u8 {
        self.0 % 10
    }
}

But it has to be careful not to set self.0 to something outside of the range 0..100. That could cause UB for users!

According to my understanding of the logic behind unsafe fields, the field here should be an unsafe field: setting it incorrectly would cause UB. But now the above code would need a bunch of unsafe blocks, making such blocks much less rare:

#![feature(unsafe_fields)]

/// Represents a two digit number robustly.
/// Certified by professionals. It can be trusted for memory safety.
pub struct TwoDigit {
    /// We must ensure for memory safety that this is in the range 0..100.
    unsafe val: u8,
}

impl TwoDigit {
    pub fn new(n: u8) -> Option<Self> {
        if n < 100 {
            // Safety: `n` is in the range 0..100.
            let res = unsafe { Self { val: n } };
            Some(res)
        } else {
            None
        }
    }

    /// Returns the top digit in 0..10.
    /// This is a robust guarantee.
    pub fn top_digit(self) -> u8 {
        // Safety: just reading.
        let val = unsafe { self.val };
        val / 10
    }

    /// Returns the bottom digit in 0..10.
    /// This is a robust guarantee.
    pub fn bottom_digit(self) -> u8 {
        // Safety: just reading.
        let val = unsafe { self.val };
        val % 10
    }
}

Also notice how the calculations whose correctness users rely on (val / 10, val % 10) are still outside of unsafe blocks. I could make mistakes there, outside of unsafe blocks, and users will get UB. So even having to add all these unsafe blocks doesn't really point me to where the dangerous code actually is:

    pub fn top_digit(self) -> u8 {
        // Safety: just reading.
        let val = unsafe { self.val };
        val / 9  // OOPS a bug, should be val / 10
    }

I think this feature would be forcing yourself to use unsafe blocks for something they aren't well suited for. unsafe blocks aren't well suited for marking "delicate code that has to be correct". unsafe blocks can and usually do rely on the correctness of safe code in the same module. All that safe code has to be correct too.

The only thing unsafe blocks can consistently mark are requirements between abstractions, at the entry points of an abstraction encapsulated and separated by a privacy boundary.

---

1. I think it should be people using my library that decide whether it's trustworthy enough to use for risky purposes, not the author, but I'm avoiding that discussion here. ↩︎

[ais523]

I think I now understand the problem that unsafe fields are trying to solve (and although I have an idea of a solution, I'm not sure whether it's actually a good one).

There are two possible scenarios:

1. A field of a struct (or of an enum variant) has some values that (for whatever safety-related reason) it can never be set to, so changing it arbitrarily is unsound.
2. A struct or enum has some invariants that (for whatever safety-related reason) need to be preserved, so changing it in ways that break those invariants is unsound.

For scenario 1, unsafe fields (private or otherwise) work pretty well. But this scenario already has an existing solution: you can just use a wrapper type that enforces the restriction. (As a simple example, if a numerical field can have any value other than 0, instead of using a primitive integer type, you can use NonZero which acts as a proof that the value is in the expected range.) In this case, the field wouldn't need to be unsafe because the wrapper type is already enforcing the restriction you need.

For cases of scenario 2 that don't reduce to scenario 1, you want operations that could break the invariant to require an unsafe – but that's usually a small subset of the operations you're actually performing. Additionally, the invariants aren't really restricted to a particular field; they cross multiple fields, and may on some occasions depend on things that aren't part of the struct/enum itself. For example, Vec has a restriction that the storage is initialised as far as the length (which are restrictions on the Vec fields) but also that the storage is allocated by a particular allocator (which is an invariant that could be violated by, e.g., calling the safe method as_raw_ptr and then directly deallocating the memory using the resulting pointer – this method of violating the invariant and thus breaking memory safety doesn't require changing the Vec's fields at all).

There are some tools available in current Rust for helping to enforce invariants. For cross-field invariants, you can mark the struct/enum fields as non-public, and place it into a small module whose only purpose is to ensure that all accesses to the fields go through accessors that maintain the invariants. For invariants that apply to the world outside the struct/enum, you can use types like mutable references and OwnedFd for which values of that type represent promises that some particular part of the machine state won't be changed except via the value: owning an &mut T represents a promise that nobody else will change the target of the reference, and owning an OwnedFd represents a promise that nobody else will close the referenced file.

This line of thought makes me think that maybe the correct abstraction is an "internally unsafe module" – the idea is that a type defined in such a module might have invariants that are required for soundness, and everything within the module would need unsafe to access fields of the type and safety comments to explain how those properties are upheld. Most of the type's implementation would be done outside the module, calling into a safe API offered by the module in question, and so wouldn't need the safety comments. (The idea is that the module would be as small as possible and encapsulate a safe interface for using the object without violating its invariants.) I'm not sure that this is actually the right solution but it seems to be pointing in the right sort of direction.

[RalfJung]

@tczajka

According to my understanding of the logic behind unsafe fields, the field here should be an unsafe field: setting it incorrectly would cause UB. But now the above code would need a bunch of unsafe blocks, making such blocks much less rare:

I am curious, would you object less to unsafe fields if reading them was fine but writing required an unsafe block? That would reduce the number of unsafe blocks quite a lot overall.

(This was discussed a lot and there are good reasons why the RFC made all accesses unsafe, but it's certainly one of the less clear-cut parts of the RFC.)

[tczajka]

@RalfJung

I am curious, would you object less to unsafe fields if reading them was fine but writing required an unsafe block? That would reduce the number of unsafe blocks quite a lot overall.

I don't see myself using this feature either way. It might make sense for pub fields. I much prefer my simple types such as TwoDigit not to use unsafe. I use such types a lot and they rarely require any unsafe, and if they do then it's only for some special "advanced" optimizations not for basic field access.

[josh]

Personally, I think "unsafe to do anything with" is the right default, but I'd be a fan of some way to spell "unsafe to write" too.

[HjVT]

It be to me that maybe unsafe fields could make more sense with the following framing:

Unsafe field is one such that validity of it relies on the state of a different field.

This is does cover Vec example, and it would explain why it does not make sense for any of your numeric newtype examples, since they only have one field.

[tczajka]

@HjVT

It be to me that maybe unsafe fields could make more sense with the following framing:

Unsafe field is one such that validity of it relies on the state of a different field.

I don't see any difference. I wouldn't want Rational to use unsafe blocks for all its math, even though I may want to later trust Rational's correctness for memory allocation and optimization:

pub struct Rational {
    // Numerator and denominator are always kept mutually prime.
    // Denominator is always positive.
    numerator: i32,
    denominator: u32,
}

@HjVT

Vec example

I wouldn't want to use unsafe fields when implementing Vec either. The relationship between len, cap and buf is a typical struct invariant, just like in Rational.

If Vec does a lot more other than just dealing with the memory buffer, that's a sign that the memory buffer logic should be split into a separate type in a separate module that only deals with the memory buffer. That would be a much better abstraction than sticking unsafe on all field accesses (or writes).

[tczajka]

@dlight

This would settle the question on when it's fine for unsafe code to trust third party safe code. And the answer is: unsafe code can trust safe code for UB matters for properties declared as robust (the author of the robust API is really promising that those properties are valid for unsafe code to depend upon, AND that new semver-compatible versions will continue to hold those properties).

Maybe I'm misunderstanding something, but this whole idea seems to be suggesting is that the author of each crate should declare how confident they are the code is correct, on a scale 0-2:

• 0 = not correct
• 1 = probably correct, but don't trust me when the risk is high
• 2 (robust) = I'm very sure it's correct, feel free to use it for memory purposes

It doesn't seem sensible for the author of a crate to judge this. Users should be judging the quality, not the author.

Also, how would a robust keyword prevent anybody from using level 1 crates ("not robust") in unsafe blocks? Would it be a compile error to call functions not marked robust in unsafe blocks? What about in the code just before unsafe blocks?

What makes even less sense to me is to write the internal implementation of a crate differently depending on which level you're at. Once you do a lot of testing and upgrade confidence level from 1 to 2, now you go and change the internal implementation to use unsafe fields and unsafe blocks for modifications of those fields?

[ia0]

@tczajka

Maybe I'm misunderstanding something

I guess there's quite some misunderstanding in this thread more generally than just this. Not sure what's the best vocabulary, but here's an attempt to clarify.

There's a few somehow orthogonal notions:

• Trust: The final user wants to know something (correctness, safety, no I/O, etc) about the program. There's no choice but to check it, possibly delegating those checks (like using an open source compiler and hoping that it gets enough attention to not have to look at it yourself). It doesn't matter what each crate of the dependency tree says (unless you trust them).
• API: This is a contract between a crate author (or its many authors) and its users. It documents many things like correctness, safety, stability (some properties may break at minor bumps only, while others at major bumps), etc. The contract is not about trust, it's about documentation.
• Properties: Those are things about the program that matter for trust and API. Most commons are correctness (absence of bugs) and safety (absence of UBs). Different people care about different properties at different levels.
• Style: This is how crate authors organize their code. Single authors may just write code and have all properties and proofs in mind. Crates with many collaborators may want to document the inner of the crate as if it were many internal crates, because no one sees the big picture (and should not need to, to contribute).

Robustness is an API concept regarding the safety property. It is not about trust and not about style. Its just a standard way to document safety guarantees.

However proper API obviously helps with trust, because the final user does not need to reverse the code to figure out if their property of interest holds. There are breadcrumbs at API level.

API also helps with style, because you can document your modules as if they were crates. You can even document internal functions and fields. That's why it makes no sense to criticize unsafe fields by looking at Vec::len, because unsafe fields are an API concept, not a style concept. In the context of a big crate like the standard library, it makes sense to use a narrow style for the many contributors and audits (trust). But in some random crate that only provides Vec then this is obviously overkill.

The "read/write" versus "write only" question is yet something else that I commented on before (and it's related to the lack of the robust keyword).

[tczajka]

@ia0

safety (absence of UBs)

@ia0

Robustness is an API concept regarding the safety property.

This seems to contradict your example:

@ia0

impl NonZero {
    /// Returns the underlying non-zero value.
    ///
    /// # Robustness
    ///
    /// The returned value is non-zero.
    pub robust fn get(self) -> u32 {
        self.val
    }
}

The "Robustness" section says something about what the return value is going to be, not just about whether the function has undefined behavior.

@ia0

Its just a standard way to document safety guarantees.

So you have a "Robustness" section in the doc comment. What's the point of the keyword in addition to that? What does the keyword do?

[ia0]

Safety is a program property. The safety contract of a function is just how you can compose a larger proof about the program. It's not about the behavior of that function in isolation.

[ia0]

@tczajka

So you have a "Robustness" section in the doc comment. What's the point of the keyword in addition to that? What does the keyword do?

The keyword is just here for tooling. You don't need it per se. At least for free functions. For trait methods, it makes implementing the trait unsafe. For fields, it make writing them unsafe.

[tczajka]

@ia0

The safety contract of a function is just how you can compose a larger proof about the program.

In other words, sounds like it's a weaker but more confident correctness guarantee for use in proofs, like I said:

With level 1 confidence: get will return the actual correct non-zero number that NonZero was set to (but don't trust this for safety proofs)

With level 2 confidence: get will return some non-zero number, not necessarily the correct one (you can trust this for sure, use it for your proofs)

@ia0

The keyword is just here for tooling.

What tooling?

I don't see much value in having the crate author judging this. After all, there could be bugs in his level 2 guarantees too, and then any proofs are working on wrong assumptions. A confident author will say all the guarantees are at level 2. No tooling is going to verify the claims made at either level. Users just have to decide whether they trust a crate or not, regardless of any such markings.

[ia0]

@tczajka

In other words, sounds like it's a weaker but more confident correctness guarantee for use in proofs

Not "confident", but otherwise yes. It's what the author wants to guarantee for safety (the selling point of Rust versus say C). Not sure what's the best word is.

@tczajka

What tooling?

The type system and also custom tooling (like the one triggering particular review on unsafe code). But for free functions, it's not so relevant because it's the same person who writes the specification and the implementation. For traits and fields it's different.

@tczajka

I don't see much value in having the crate author judging this

I agree with this. That's one of the point I documented in the unsafe mental model. But I believe this applies to correctness too. If you provide a sort function, how do you know if your users want a stable one? It's hard to judge too. That's an iterative process based on user feedback.

[tczajka]

@ia0

For trait methods, it makes implementing the trait unsafe.

There already exists a notation for this: unsafe trait.