Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify what is UB #149

Merged
merged 21 commits into from
Aug 16, 2019
Merged

clarify what is UB #149

merged 21 commits into from
Aug 16, 2019

Conversation

RalfJung
Copy link
Member

Also add some missing cases

src/what-unsafe-does.md Outdated Show resolved Hide resolved
src/what-unsafe-does.md Outdated Show resolved Hide resolved
src/what-unsafe-does.md Outdated Show resolved Hide resolved
@Lokathor
Copy link
Contributor

I think the str requirement isn't quite correct. As I understand it: the str must be valid bytes any time a string processing function is called on it, but during your own unsafe blocks it can temporarily be invalid the same as a the fields of a Vec can temporarily be invalid as long as you fix it all by the end of the unsafe block.

@Lokathor
Copy link
Contributor

Separate post since it's a separate issue and much less prone to discussion:

  • the NonNull and NonZero types must not be 0

@RalfJung
Copy link
Member Author

RalfJung commented Jul 20, 2019

I think the str requirement isn't quite correct. As I understand it: the str must be valid bytes any time a string processing function is called on it, but during your own unsafe blocks it can temporarily be invalid the same as a the fields of a Vec can temporarily be invalid as long as you fix it all by the end of the unsafe block.

I agree but that seems like a discussion separate from this PR. See rust-lang/unsafe-code-guidelines#78.

the NonNull and NonZero types must not be 0

Good points. We have some more such types inside rustc and nightly code can define their own if they enable feature(rustc_attributes).

@Gankra
Copy link
Contributor

Gankra commented Jul 20, 2019

NonNull and NonZero are again library contracts, and not intrinsic to the language, afaict.

@Lokathor
Copy link
Contributor

So is the str contract. Either it has to get out of the list or they have to be in the list.

@Lokathor
Copy link
Contributor

I take it back:

  • NonZero/NonNull is definitely a Validity Invariant, while "must be utf8" is either a Validity or Safety invariant (probably safety).
  • So it's definitely insta-UB to make a zero NonZeroFoo the same as it is to make a bool that's not 0 or 1.
  • So they definitely need to be in the list even if str is allowed to be taken off the list for being "only" a safety invariant.

Unfortunately the description doesn't currently talk about validity invariant vs safety invariant, which is maybe a bigger change than this PR was setting out to clean up. However, I think that's the best way for this part of the book to move towards.

@Gankra
Copy link
Contributor

Gankra commented Jul 20, 2019

re: str

My understanding is that constructing an invalid str is technically insta-UB, just like bool. Although it's very implausible for the compiler to detect this or use it productively. Has that changed? Is the stdlib or something else actively relying on transiently invalid strs? But more importantly, str is a language primitive, and not something the stdlib cooked up.

re: NonZero

That's a good point on them being insta-UB to mess up. I guess in my mind they're still just library wrappers around some deeper concept that we have simply refused to stabilize. I think I would rather appeal to some more abstract concept like "producing an invalid library type that defines custom invalid values, like NonZero"

@Lokathor
Copy link
Contributor

The fate of str is being discussed here: rust-lang/unsafe-code-guidelines#78

That said, I think this whole page needs to back up a moment, talk about Safety Invariant vs Validity Invariant, and then talk about all major core types in terms of those definitions.

@RalfJung
Copy link
Member Author

That said, I think this whole page needs to back up a moment, talk about Safety Invariant vs Validity Invariant, and then talk about all major core types in terms of those definitions.

To do that we pretty much have to RFC those notions. I think we can do an update that improves the docs without going through all of that.

So, I agree that should happen eventually -- but we are talking about a timescale of months here.

That's a good point on them being insta-UB to mess up. I guess in my mind they're still just library wrappers around some deeper concept that we have simply refused to stabilize.

In some sense that's true, and the deeper concept currently has the name rustc_layout_scalar_valid_range_start. But I did not want to mention that here.

src/what-unsafe-does.md Outdated Show resolved Hide resolved
@Lokathor
Copy link
Contributor

Lokathor commented Jul 20, 2019

So, in that case I think my question becomes... is Rustonomicon itself RFC controlled or informal information? Is the information "how it works today" or is it "stable for all time"?

If it's not RFC controlled stable, just a best effort to document the innards of Rust as it exists today, I don't see the harm in pointing at even draft definitions, assuming that the draft is also its own best effort, if that gives people a much better way to talk about when the UB will happen to them.

The concept of a Validity Invariant and how it's insta-UB to set x: bool = transmute(2); even if you never look at it, compared to a Safety Invariant where you can call Vec::set_len(&v, core::usize::MAX) and it's fine until you use the vec wrong is extremely teachable because it gives names to use to talk about things, so students can ask about what they really mean when they have a question.

Either way, we don't need to add all that in this PR right now, but I'd like to know how official and immutable Rustonomicon is trying to be, and where it lives compared to the Reference and the UCG.

@Gankra
Copy link
Contributor

Gankra commented Jul 20, 2019

The Rustonomicon is non-normative and exists primarily to allow people to write correct programs. The most important thing is that if we say "this is OK to do" then we are in fact correct. It is perfectly fine if we say that you can't do something if you in fact really can. In particular, the world cannot wait in baited breath for the UCG team to formalize the entire language (a hard and time-consuming task), so the nomicon conservatively declares things to be UB which might not be.

In some places we have taken shots on calling things Correct that are ultimately declared Wrong or dubious (e.g. access-based validity). Ideally, this is based on Real Things The Stdlib Does. At least in those cases we're probably not going to misoptimize you, and hopefully the fact that it's wrong will cause enough of a ruckus in the stdlib for it to become A Well-Known Issue.

I am sympathetic to the desire to be maximally accurate so that devs won't dismiss claims as obviously over-broad, and this is definitely a place to be pedantic! But also it might be good in some places to be over-broad just as a matter of clarity.

Talking about Validity vs Safety (or whatever) does indeed sound within scope of the rustonomicon. I'll need to think about it.

@RalfJung
Copy link
Member Author

I think this resolves the open discussions.

How to incorporate the validity/safety invariant thing seems to me to be a topic for a separate PR.

@Havvy
Copy link
Contributor

Havvy commented Jul 21, 2019

Reminder to also copy this PR onto the reference after it's merged here.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 21, 2019

@Havvy Oh, you mean for https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html? I thought that I had seen another "UB list" somewhere but could not remember where.

Is there any good reason to even have two lists?

@Lokathor
Copy link
Contributor

Lokathor commented Jul 21, 2019

Ideally, the Reference will become less generally PR-able and drifts towards being fully normative over time.

At the moment, the reference is just as non-normative as the Nomicon and UCG, so there's not a big deal.

@RalfJung
Copy link
Member Author

In fact the nomicon looks more normative because the reference has all these big fat red warnings everywhere.^^

But I was mostly concerned about having to keep the lists in sync.

@Lokathor
Copy link
Contributor

I still think that the bullet point

  • Dereferencing (using the * operator on) null, dangling, or unaligned references or raw pointers

Needs to remove the word "references". Line 25 already lists null/dangling/unaligned references as UB, so if you have one to de-reference you're already off the edge of the map.

@RalfJung
Copy link
Member Author

My thinking was that it is better to cover a case twice than to forget one.

@Lokathor
Copy link
Contributor

"It wouldn't say that you can't dereference them if you weren't even allowed to have them, it's fine"
"dude what about the list a few lines down?"
"Look it wouldn't talk about unaligned references if they were impossible, so I'm not gonna change it"

when this conversation inevitably happens I'm gonna link them this PR thread ;3

@RalfJung
Copy link
Member Author

RalfJung commented Jul 24, 2019

Okay I think this now covers any UB that primitive operations (as opposed to unsafe fn/intrinsics) can cause -- at least assuming this and this "RFC proposal" ends up being accepted.

src/what-unsafe-does.md Outdated Show resolved Hide resolved
Co-Authored-By: gnzlbg <gnzlbg@users.noreply.github.com>
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have an annoying headache rn, so just briefs comments

src/what-unsafe-does.md Outdated Show resolved Hide resolved
src/what-unsafe-does.md Outdated Show resolved Hide resolved
`Trait` that matches the actual dynamic trait the reference points to
* a non-utf8 `str`
* an uninitialized integer (`i*`/`u*`), floating point value (`f*`), or raw
pointer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to think about this precise wording. I think you can break this out of here and fold it into "reading uninitialized memory". I want to have some wording along the lines of "you can't tell the compiler a value definitely exists where only uninitialized memory resides", but this needs to be reinforced by documentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a type-based property though. The following is fine:

let x: MaybeUninit<u8> = mem::uninitialized();

but the following is not:

let x: u8 = mem::uninitialized();

I would actually remove the "Reading uninitialized memory" clause in favor of this type-based one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a shot, let me know what you think.

Copy link
Contributor

@Gankra Gankra Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i get what you're saying but two things:

A) Pedantic aside that probably doesn't matter: do we actually care about guaranteeing let x: MaybeUninit<u8> = mem::uninitialized(); is valid? I suppose that's important if you want to be able to cast a pointer to a fresh allocation to be &mut MaybeUninit and not do dummy writes of uninit() to be able to call its methods? Hmm... (sub-aside: i'm a bit uncomfortable with technical discussion appealing to mem::uninitialized since it's busted in a bunch of ways, so it doesn't actually have a coherent meaning. Fresh allocations, however, work fine as well-defined uninitialized memory the compiler knows about.)

B) I was using a subtler definition of "a value exists", which agrees with your statement that it's type-based. Basically empty types don't have values; they can't. So by having a union { (), T } you are, with types, telling the compiler that it may always be the case that a value doesn't exist there (but it's not opaque like asm or volatile, so the compiler can clearly observe reads and writes and reason about the memory's bytes).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, my definition of empty things being valueless fails to handle !

Copy link
Contributor

@Gankra Gankra Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's some Proposed Terminology for the sake of this conversation. This is not necessarily useful for a formal model, but it is helping me make sense of the concepts in my head:

  • a value is a property a byte or collection of bytes has. Bytes may have the secret 257th value of "uninitialized".

  • An instance of a type is something you get from constructing that type. Moving or destroying an instance relinquishes a piece of code's access to that instance. Those instances do not necessarily have a unique, tracked, identity, but their ability to reach a point in the program serves as a kind of proof (more on that below). The Copy trait just gives permission to all code that has acquired an instance to mint more instances with the same value.

I am not 100% comfortable with the notion of "construction", as room must be made for transmutes, plain-old-data, and deserializers. I think this basically boils down to a similar situation as double-drops where any type is freely allowed to assume its instances aren't double-dropped with UB at stake, but a type may randomly decide it's fine to be double-dropped. There's no trait/property for this, it's just a thing a type can randomly be fine with. It's very contrived to come up with such a type, so we don't care about this much outside of the very specific niche of standards and trivia.

Similarly, types must be able to assume that there is meaning to the existence of an instance, also with UB at stake. But types may allow instances to be minted without any constructor. Which is to say, for some types it may be fine to have more instances of a type "live" in a program than the number of its constructors that have run. This is a relatively important fact about a type, but just like with doubledropabble it's not something with a standardized trait/property. (perhaps one can just appeal to access to a type's record initialization syntax as permission to mint as many instances as one pleases without even using that syntax?)

Anyway.

Certain types may have forbidden values, indicating that if a piece of code claims to have an instance of a type, the memory that instance occupies must not have certain values.

Zero-sized types do not have values, but they do still have instances. A logical consequence of this is that we cannot have the concept of a ZST with a forbidden value. Nor can we speak of an "uninitialized" ZST. It doesn't make sense.

From this, I would like to assert that ! does not in fact fall under the umbrella of forbidden values: it is simply forbidden to claim that you have an instance of !, because it does not have a constructor. If you claim to have an instance of !, you are lying, and we have made that lie unconditional UB (as is our right as type authors).

Some additional context, from the practical perspective:

In the stdlib we frequently appeal to the idea that, logically speaking, there is always an instance of a zero-sized type everywhere you look. So e.g. (73 as *const ()).read() is not UB ("you can pull instances from the aether").

This logically falls out from needing to be able to implement a type like Vec which doesn't like, reserve an address in memory for its copies of the ZSTs. It just dumps them into the aether and then pulls them from the aether when it needs to.

However it is dangerous to pull values from the aether like this. It is an important property of rust that privacy lets abstraction authors use the existence of a instance of a type they control serve as a proof that certain things have occurred, and this extends to ZSTs. So for instance one might use a ZST as a token giving permission to access a global, and so pulling that token from the aether could cause UB.

The obvious rule of thumb here is that you should only be able to pull as many instances from the aether as you dumped there. Hence Vec<ZST> is fine, because it is basically just a counter that tracks how many instances of the ZST that this Vec has been given and subsequently dumped into the aether. This counter prevents safe code from pulling extra instances from the aether (forging them).

This jives well with ! being illegal to claim to have an instance of: you "can't" pull an instance from the aether that you did not dump there unless you have access to the type's constructor, and no one has access to that constructor.

Copy link
Contributor

@Gankra Gankra Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postscript: claiming to have an instance of union MyUnion { a: T, b: U } is a claim that you have either an instance of T or an instance of U stored in that memory (this similarly follows for enums, but discriminants make it messier to talk about so let's ignore them).

If T is a ZST (as in MaybeUninit), it follows that it is always valid for a MyUnion instance to be uninitialized, as that is always conformant with the claim that "there exists an instance of T here". Similarly if T is fine to instantiate, it's fine for U to be illegal to instantiate (!), as "p or contradiction" is just "p".

An interesting claim that falls out of what I just wrote here is that if T and U have mutually invalid values, the compiler may conclude that those mutually-invalid values are invalid for a MyUnion to have. So a union (or enum) is not actually completely opaque to the compiler. It's just that if one arm is (), the compiler has to let you do whatever you want with the memory/instantiation. I am lightly worried this contradicts something important, but I'm not sure! I do think this is consistent with how we compute "niches" for enum packing optimizations, I'm just having trouble convincing myself that it's ok to peek through someone's bare union if niches happen to line up. If it's not, the union author can use the MaybeUninit trick to make the compiler back off (similar to how people add PhantomData fields all the time), so I don't think it's a big deal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a value is a property a byte or collection of bytes has. Bytes may have the secret 257th value of "uninitialized".

Okay, so that sounds more like what I wold call a "(Rust) Abstract (Machine) Byte".

An instance of a type is something you get from constructing that type.

Is this a list of bytes, of values, or something more mathematically abstract?

Zero-sized types do not have values, but they do still have instances. A logical consequence of this is that we cannot have the concept of a ZST with a forbidden value. Nor can we speak of an "uninitialized" ZST. It doesn't make sense.

From this, I would like to assert that ! does not in fact fall under the umbrella of forbidden values: it is simply forbidden to claim that you have an instance of !, because it does not have a constructor. If you claim to have an instance of !, you are lying, and we have made that lie unconditional UB (as is our right as type authors).

To me this indicates your concepts are flawed. I feel the fact that ! has 0 "inhabitants" and () has 1 and bool has 2 should nicely fall out of the same concept. I have described such a concept in the UCG repo, but it is still work-in-progress.

In particular, "ZSTs have no values" sounds just so wrong to me. The type () obviously has 1 value, and that's ()! (We sadly write them the same in Rust, but you get what I mean.) You even said yourself that "value" has something to do with a "collection of bytes"; if I interpret that freely as "list of bytes" then the type () has one "value", and that's the empty list.

"No value" to me sounds like "empty type", and that raises some bad memories about how many people call void in C "empty". That's not accurate. void is exactly like (), it conveys no information, but it isn't "empty". The way to formally model "no information" is to say that "there is only one possible answer", AKA "the answer was already known so it doesn't even have to be given", AKA a type with exactly 1 possible value.

Information-theoretically this also works perfectly: you need log2(N) many bits to encode which of N answers is given, and log2(1) = 0. So you need 0 bits to encode the () type.

In the stdlib we frequently appeal to the idea that, logically speaking, there is always an instance of a zero-sized type everywhere you look. So e.g. (73 as *const ()).read() is not UB ("you can pull instances from the aether").

The way I would phrase it is that copying/moving a ZST is a NOP. There is nothing to copy, operationally. As you said, Vec won't pull "more instance of a ZST out of this air as exist in the vector", so logically speaking, this is not really out-of-thin air -- it's just that you cannot see the moves that are happening.

We've had cases where our handling of ZST did not follow this model, but I think those were bugs.

However it is dangerous to pull values from the aether like this. It is an important property of rust that privacy lets abstraction authors use the existence of a instance of a type they control serve as a proof that certain things have occurred, and this extends to ZSTs. So for instance one might use a ZST as a token giving permission to access a global, and so pulling that token from the aether could cause UB.

Agreed. However, in my terminology, those privacy-induced extra properties arise from the safety invariant, not the validity invariant.


All of that said, I think much of it is not relevant for a discussion about "what is UB". Even if Vec did "duplicate" a ZST, that would not be instantaneous UB! We cannot make the Abstract Machine understand the safety invariant, it can be arbitrarily complicated after all and is often user-defined.

In other words, I claim that for any slice of ZST, doubling its length does not cause undefined behavior. Doing this may lead to UB later, when some code that assumed its safety invariant is not broken runs, but the act of doubling the slice length itself is perfectly fine, from the perspective of the Abstract Machine.


claiming to have an instance of union MyUnion { a: T, b: U } is a claim that you have either an instance of T or an instance of U stored in that memory

We have some disagreements here. ;)

I think treating unions this way is very, very complicated because when we go about defining the Abstract Machine in a concrete, operational way (like an interpreter), we cannot know which variant the union is in. It is because of that that I am strongly pushing for "unions have no active variant". https://github.com/rust-lang/rfcs/blob/master/text/2514-union-initialization-and-drop.md deliberately did not have such a notion, and these days the Reference states:

Unions have no notion of an "active field".

An interesting claim that falls out of what I just wrote here is that if T and U have mutually invalid values, the compiler may conclude that those mutually-invalid values are invalid for a MyUnion to have. So a union (or enum) is not actually completely opaque to the compiler.

I think you are re-tracing a lot of rust-lang/unsafe-code-guidelines#73 right now. ;) The OP of that thread already contains some code that I (and many in the UCG) think is legal, but that you just ruled out.

Copy link
Contributor

@Gankra Gankra Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't properly consider the claim of unions "having an instance" in the context of field-by-field re-initialization into another type (which is of course extremely important). But my intended claim here is exactly the one gnzlbg is championing in that thread (and I have replied there to that effect) -- only if niches align is a niche actually available. Hence "mutually invalid values" (the intersection of their value domains, in your terminology). This is consistent with the way we make you create a union by providing an entire instance of one of the union's cases. Like, by default to create a union you have to very materially have an instance.

But yeah once you start using the union you can enter a super-position of instances, which certainly makes it awkward to talk about. I am absolutely not intending to suggest there is an "active member" as that is a horribly cursed concept.

Certainly though, if you ever do &mut my_union.a, in my understanding of the current reference semantics, you are making a very material claim of having an instance of that type. And it would also be very difficult for us to make unions robustly opaque, in the way asm!() blocks and volatile accesses are.

Something still really bothers about the rules of ! falling out the rules for value domains. Like yes I understand the notion of 0 vs 1 inhabitants (although thank you for reminding me of that term), but inhabitants don't feel like the right abstraction for talking about packing bits? Like you also appealed to the notion of "a list of bytes", which I agree can reasonably include the empty list, but if I ask for a list, you're not allowed to come back with "well, here's no list at all, that's a list right".

Gonna keep reading the relevant things over and mulling it over. I am under the impression I'm just in a weird headspace and I'll eventually come around to your position on just having ! slot in here naturally.

Question for you: is union { a: !, b: ! } uninstantiable? i.e. is it equivalent to !? In my mind it should be equivalent to !, but it seems like perhaps you would like it to be instantiable?

More broadly: to what extent is union { a: T, b: T} not just T? Your definition of unions seems to want it to be equivalent to MaybeUninit<T> (ignoring Drop as uninteresting here), which is strange, because that isn't the definition it uses!

(assume repr(rust) here, if you find my proposal for unions in rust-lang/unsafe-code-guidelines#73 compelling)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly though, if you ever do &mut my_union.a, in my understanding of the current reference semantics, you are making a very material claim of having an instance of that type.

Fair. At this point we entered rust-lang/unsafe-code-guidelines#77: does a reference &mut T have to point to a "valid" T even when the T is not actually loaded from memory? The document here for now, conservatively, says "yes", but I actually think we should delay this until the pointer is actually "followed" and a T is materialized. But that is a separate discussion.

inhabitants don't feel like the right abstraction for talking about packing bits? Like you also appealed to the notion of "a list of bytes", which I agree can reasonably include the empty list, but if I ask for a list, you're not allowed to come back with "well, here's no list at all, that's a list right".

That's why I also have a relation in the mathematical sense that relates "abstract values" (the view of the world where bool has 2 inhabitants, () has 1 and ! has 0) with the way they are represented on the machine (as lists of bytes). If we assume canonical representaiton for a second (which not all types have, but the three we are considering here do), then that means there are exactly 2 byte lists that represent a valid bool (both lists have length 1: [0x00] and [0x01]), there is exactly 1 byte list that represents a valid () (the empty list []), and there is no byte list that represents a valid !.

So, I think this translates fine even if you switch between the abstract view of a type (the "value" view) and its concrete view (the "byte list" view).

Question for you: is union { a: !, b: ! } uninstantiable? i.e. is it equivalent to !? In my mind it should be equivalent to !, but it seems like perhaps you would like it to be instantiable?

I personally would prefer for a union to be just a "bag of bytes": its abstract values are all byte lists of appropriate lengths, and the types in the fields don't matter at all (except for their size and alignment) until some field is actually used.

My view of a union of size N is basically that is is the same as [MaybeUninit<u8>; N], and that we also offer field access as a convenient way to transmute that type to the type of the field (truncating the data if the field does not cover the entire union).

There are problems with that view, unfortunately. But they are not related to !.

So, with that view, the union you are asking about is indeed "instantiable"; it has size 0 and the empty byte list is valid for that type. But accessing any of its fields is a transmute-to-! and therefore UB.

which is strange, because that isn't the definition it uses!

What do you mean by "the definition it uses"?

src/what-unsafe-does.md Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member Author

RalfJung commented Aug 5, 2019

@gankro any update on this?

@Gankra
Copy link
Contributor

Gankra commented Aug 14, 2019

As discussed in zulip, with the introduction of &raw i believe it becomes necessary to explicitly define that it's UB to write to "read only" memory (immutable statics that don't contain UnsafeCell), as the pointer aliasing rules seemingly no longer cover that issue.

As always I would prefer that this case is actually well-defined (as there is a very reasonable safe behaviour on all major platforms with default configurations -- fault and crash). But I am sympathetic to this being potentially hard to properly define/support/guarantee.

Also it's just unclear to me how to define this in a clear way. I'm not 100% comfortable with just dismissing/ignoring the existence of readonly memmap'd pages, as this feels like a very important thing to elaborate on.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 14, 2019

As discussed in zulip, with the introduction of &raw i believe it becomes necessary to explicitly define that it's UB to write to "read only" memory (immutable statics that don't contain UnsafeCell), as the pointer aliasing rules seemingly no longer cover that issue.

Not only statics:

let x = 3;
unsafe { *(&raw x as *mut i32) = 42 };

does not violate any pointer aliasing rules either AFAICT, yet I feel that the behavior of this code is undefined.

As always I would prefer that this case is actually well-defined (as there is a very reasonable safe behaviour on all major platforms with default configurations -- fault and crash). But I am sympathetic to this being potentially hard to properly define/support/guarantee.

Whether faults, crashes, etc. exist depends on the target. The proper thing to do here would be to abort, which on some targets would just loop forever.

I don't know of any target for which this would be cheap, e.g., most targets just SIGSEV on this, and I don't think it would be ok for the Rust runtime to install a signal handler for these and turn those into aborts, in most cases we can't tell where the SIGSEV is coming from. Even if we did that, the user can replace our signal handler, so I don't think we could guarantee an abort even on "nice" targets.

On targets that do not support read-only memory and just put statics into normal memory, we probably would need to check whether each write access the memory of a static at runtime to be able to abort. Optimizations might be able to, in some cases, conclude that a pointer never points to a static, but it feels like a long shot.

We don't even guarantee that a stack overflow aborts the program if it happens in safe Rust. A write to an immutable variable cannot happen in safe Rust, so all of this would feel like too much trouble for something that's inherently unsafe anyways (pointer dereferences).

This doesn't mean that we shouldn't try to make the program terminate ASAP if a write to an immutable variable happens - undefined behavior allows us to do anything, including emitting a nice error message. I have no idea how we could guarantee that such a things happens without adding overhead.

Also it's just unclear to me how to define this in a clear way. I'm not 100% comfortable with just dismissing/ignoring the existence of readonly memmap'd pages, as this feels like a very important thing to elaborate on.

The functions of GlobalAlloc and Alloc state their safety invariant, which tells you what you are allowed to do with the memory they return.

I don't see how mmap would be any different than, e.g., the following function foo:

// You can only read from foo() if the integer 
// representation of the pointer is `42`. 
int8_t* foo();

If you do an FFI call, you are on your own. Read the docs, hope that they are correct, and do what they say. Even for mmap, dozens of OSes implement it, all of them with custom extensions. I don't see how anything that we could say here would be any more useful to users than "read the docs".

@RalfJung
Copy link
Member Author

as the pointer aliasing rules seemingly no longer cover that issue.

I think they can -- and in fact, Stacked Borrows does. But you may not like the way this happens.

@gnzlbg

does not violate any pointer aliasing rules either AFAICT, yet I feel that the behavior of this code is undefined.

I think if this is UB it must be through pointer aliasing rules. And, see above -- Stacked Borrows makes that happen.

@Gankra
Copy link
Contributor

Gankra commented Aug 14, 2019

(we are going to punt on the read-only memory issue for now since &raw doesn't exist yet)

@Lokathor
Copy link
Contributor

mutating a non-mut binding because you took a shared borrow on it and then use it as a unique borrow must violate some rule somewhere, but I'm also failing to recall exactly where such a thing is already written.

Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more edit notes.

This is a lot of edits, so if you're ok with the contents and don't want to be trapped in this eternal struggle any longer, let me know and I can take this PR over and finish it up.

src/what-unsafe-does.md Show resolved Hide resolved
* Producing invalid primitive values:
* dangling/null references
* null `fn` pointers
* Unwinding into another language
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really strictly defined to unwind between two rust libraries that are directly calling eachother via extern "C"? Otherwise we should have a more precise definition.

Specifically it might be that we just need to enumerate that you can only "exit a function via unwinding" if it has one of these 3 rust-specific calling conventions:

  • extern "rust" (the default of all fns)
  • extern "rust-call" (rust closures)
  • extern "rust-intrinsic" (things in core::intrinsics)

I think "rust-intrinsic" isn't supposed to unwind, but I might be wrong on that (perhaps the builtin operator impls have this ABI, somewhere?). But also "rust-intrinsic" kinda doesn't matter since those implementations are only provided by rustc. Only slightly matters if we care about helping devs assume raw re-exported intrinsics never unwind. But making an intrinsic a "normal" function would hardly be considered a breaking change!

Also I'm guessing incompatible compilation flags like mixing panic=abort is subsumed by the "target features" bullet below?

extern "platform-intrinsic" is also a thing but I think not relevant. I'm guessing it has something to do with libc functions that llvm is allowed to make implementation assumptions about, like malloc?)

Also I thought we had automatic guards against unwinding from an extern fn. Is that not the case? (I only work on panic=abort software so I never worry about this issue...)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is currently de facto undefined to unwind out of a Rust function defined as extern "C" no matter the caller, because our LLVM IR generation adds nounwind to such functions which makes it UB to unwind out of. IIUC lang team is saying this is deliberately and explicitly UB until explicitly stated to be supported in some cases, for which there's an ongoing RFC, rust-lang/rfcs#2699. The safeguard that aborts when you try to unwind out of an extern "C" Rust function did exist but was reverted twice so far because of fallout for software that relied on it despite it being UB, see rust-lang/rust#58794 if you want the messy history and part of the discussion that led to the aforementioned RFC.

src/what-unsafe-does.md Outdated Show resolved Hide resolved
* Executing code compiled with target features that the current thread of execution does
not support (see [`target_feature`][])
* Producing invalid primitive values (either alone or as a field of a compound
type such as `enum`/`struct`/array/tuple):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of our bickering about unions does actually wrap back around to this point:

is union { a: bool; b: bool; } = 3 "producing an invalid value as a field of a compound type"?

(we can probably gloss over this, but it is something to make clearer when we have a better answer)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is "we don't know yet, see rust-lang/unsafe-code-guidelines#73".

So yes, this is a good question, and one that I would prefer we could skip over for now.

src/what-unsafe-does.md Outdated Show resolved Hide resolved
`isize::MAX` bytes in memory
* `dyn Trait` metadata is invalid if it is not a pointer to a vtable for
`Trait` that matches the actual dynamic trait the reference points to
* a non-utf8 `str`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reword for consistency:

  • a str that isn't valid utf8

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we want to skip this entirely because this is just a library invariant?

* an integer (`i*`/`u*`), floating point value (`f*`), or raw pointer read from
[uninitialized memory][]
* an invalid library type with custom invalid values, such as a `NonNull` or
the `NonZero` family of types, that is 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reword:

  • a type with custom invalid values that is one of those values, such as a NonNull that is null. (Requesting custom invalid values is an unstable feature, but some stable stdlib types, like NonNull, make use of it.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say "a type with custom invalid values that is one of those values" sounds rather awkward. I don't have a better proposal either, though.

src/what-unsafe-does.md Outdated Show resolved Hide resolved
src/what-unsafe-does.md Outdated Show resolved Hide resolved
the `NonZero` family of types, that is 0

"Producing" a value happens any time a value is assigned, passed to a
function/primitive operation or returned from a function/primitive operation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested reword and massive clarification:

Many have trouble accepting the consequences of invalid values, so they merit some extra discussion. The claim being made here is a very strong one, so read carefully.

A value is produced whenever it is assigned, passed to something, or returned from something. Keep in mind references get to assume their referents are valid, so you can't even create a reference to an invalid value. Additionally, uninitialized memory is always invalid, so you can't assign it to anything, pass it to anything, return it from anything, or take a reference to it. (Padding bytes are not technically part of a value's memory, and so may be left uninitialized.)

In simple and blunt terms: you cannot ever even suggest the existence of an invalid value. No, it's not ok if you "don't use" or "don't read" the value. Invalid values are instant Undefined Behaviour. The only correct way to manipulate memory that could be invalid is with raw pointers using methods like write and copy. If you want to leave a local variable or struct field uninitialized (or otherwise invalid), you must use a union or enum which clearly indicates at the type level that this memory may contain no values (see MaybeUninit for details).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied most of your suggestions but this one is big enough that it is probably easier to hand the PR off to you. ;) I'd love to do a pass over what you got when you are done, if you don't mind.

I like this new text, as usual in you very pointed style! One comment though:

Additionally, uninitialized memory is always invalid, so you can't assign it to anything

That's not true for MaybeUninit.

Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's ship this and I'll do a second cleanup pass.

@Gankra Gankra merged commit 92b7198 into rust-lang:master Aug 16, 2019
@RalfJung
Copy link
Member Author

@Gankra thanks for all your feedback!

Btw, have you come to a conclusion on the issue of duplicating the list between here and the reference?

@Gankra
Copy link
Contributor

Gankra commented Aug 16, 2019

I think it's fine/good for now, as it fits the structure of the book right now, but in the fullness of time that might change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants