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

"!" is the only type that is unsafe to raw-ptr-deref into a "_" pattern #79735

Closed
RalfJung opened this issue Dec 5, 2020 · 13 comments
Closed
Labels
C-bug Category: This is a bug. F-never_type `#![feature(never_type)]` fixed-by-thir-unsafeck `-Z thir-unsafeck` handles this correctly. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Dec 5, 2020

This code compiles:

fn foo(ptr: *const bool) {
    let _ = *ptr;
}

The MIR is empty, i.e., no ptr deref happens.

But this does not:

fn foo(ptr: *const !) {
    let _ = *ptr;
}

The MIR is unreachable;, i.e., the ptr deref conceptually somehow did actually happen.

That is a very strange inconsistency -- the ! type is just yet another type, why does it behave differently here?

Curiously, this does not yield any unreachable:

fn bar(ptr: *const (i32, !)) { unsafe {
    let (x, _) = *ptr;
} }
@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2020

One possible explanation of this behavior is that "it is UB to create a place of an uninhabited type". That would be a rather surprising special case, I think, but I could get behind this since I am actually proposing to similarly treat uninhabites types specially for &T validity (see rust-lang/unsafe-code-guidelines#77).

However, under that explanation, the following code has UB:

enum Void {}

fn foo(ptr: *const Void) {
    let _ = *ptr;
}

foo(&3 as *const _ as *const _);

And yet this is all accepted as safe code. So I think something is very wrong here.

@jyn514 jyn514 added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Dec 5, 2020
@jonas-schievink jonas-schievink added C-bug Category: This is a bug. F-never_type `#![feature(never_type)]` labels Dec 5, 2020
@camelid
Copy link
Member

camelid commented Dec 7, 2020

Should this be nominated?

@DutchGhost
Copy link
Contributor

Is this what is happening with #77694 ?

@RalfJung
Copy link
Member Author

RalfJung commented Dec 7, 2020

@DutchGhost I don't think so, there is no _ pattern there.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Dec 15, 2020

We discussed this in our @rust-lang/lang meeting today. Clearly the current behavior is unsound. The question is what behavior we expect. There seem to be two related questions:

  • Should let _ = *foo be unsafe when foo is a raw pointer, independent of everything else?
  • Should let _ = *foo be UB if foo is not a valid pointer?
    • Right now it is inconsistent, with that ultimately depending on the type of the referent, seems not great
    • let _ = place is generally a no-op, but it does evaluate place for side-effects
    • so the question perhaps is whether evaluating *foo (where foo is a raw pointer) to a place has the side effect of asserting that foo is a valid pointer which could be loaded, even though we are not currently loading

Does that analysis sound about right to you, @RalfJung? (It doesn't yet say the answers to those questions, especially the second one)

@RalfJung
Copy link
Member Author

RalfJung commented Dec 16, 2020

Yes that sounds about right, modulo the vagueness of the term "valid pointer". ;)

One way to look at this is by being explicit about place-to-value coercions (p2v). The regular let x = *ptr then becomes

let x = p2v(*ptr);

Now there are two axes we can adjust:

  • Does let _ = *ptr perform a place-to-value coercion or not? If yes, certainly the ptr needs to be as valid as in let x = *ptr as the same things are happening.
  • What exactly does the mere construction of the place *ptr, without being followed by a p2v, require in terms of validity? Does the ptr need to be dereferencable (so far I always assumed the answer is yes), does it need to point to data that is valid for the place's type (so far I always assumed the answer is no)? Does this depend on the type of ptr, i.e., do we have "raw places" whose construction is governed by different rules?

One aspect to consider for the second question is expressions like &(*ptr).field and &raw (*ptr).field, where there is no implicit place-to-value coercion (assuming here that ptr is a value expression). Any validity requirement we impose on place construction will also be in force here. &raw (*ptr).field currently compiles to getelementptr inbounds, and if we want to stick to that, some amount of dereferencability needs to be required even when a "raw place" is being constructed. But there's also some demand for relaxing this to e.g. make offset_of! easier to implement -- there are proposals to instead use a currently-hypothetical getelementptr nw for &raw (*ptr).field that ensures no-overflow but does not say anything about allocation bounds. This would correspond to (raw) place construction only requiring that the place (viewed as a base address and a size) does not overflow the high end of memory, so this is another option that's on the table.

@RalfJung
Copy link
Member Author

Cc #80059

@nikomatsakis
Copy link
Contributor

We discussed this in the @rust-lang/lang meeting on 2021-02-09 and we concluded that:

  • We want unsafe to be required here, but that is covered by Unsafe checking skips pointer dereferences in unused places #80059 specifically.
  • We don't want to settle whether let _ = *ptr is UB or not at this time, it doesn't seem urgent. In other words, it is presently considered UB, and we're ok with that, but we would also definitely consider changing that in the future.

I'm going to remove nomination as there isn't anything urgent to solve at this time.

@RalfJung
Copy link
Member Author

We don't want to settle whether let _ = *ptr is UB or not at this time, it doesn't seem urgent. In other words, it is presently considered UB, and we're ok with that,

FWIW, Miri cannot detect this UB since it is not represented in the MIR.

@nikomatsakis
Copy link
Contributor

In truth, my opinion is that let _ = *ptr should not be UB, regardless of the type of ptr, but it should be unsafe if ptr is a raw pointer.

@syvb
Copy link
Contributor

syvb commented Jun 21, 2021

It seems that the THIR unsafety checker now disallows (while the MIR checker allows) dereferencing pointers to wildcards:

fn foo(ptr: *const bool) {
    let _ = *ptr;
}

@RalfJung
Copy link
Member Author

RalfJung commented Oct 30, 2023

This code compiles:

This does not compile any more. I think @cjgillot fixed it? So the issue can be closed if we have a test.

In truth, my opinion is that let _ = *ptr should not be UB, regardless of the type of ptr, but it should be unsafe if ptr is a raw pointer.

That is indeed the semantics we are going for. For bool and enum Void {}, this is already the case. For ! it is not but that's a bug: #117288.

@RalfJung
Copy link
Member Author

#102256 added a test so this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. F-never_type `#![feature(never_type)]` fixed-by-thir-unsafeck `-Z thir-unsafeck` handles this correctly. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants