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 it UB for a uninhibited type, like the never type, value to exist? #337

Open
erickt opened this issue May 10, 2022 · 7 comments
Open

Comments

@erickt
Copy link

erickt commented May 10, 2022

Is it safe to create a value that contains an uninhibited type, like the never type, if that value is never read from? For example:

enum Impossible {}

#[repr(transparent)]
struct Foo {
    value: u8,
    impossible: Impossible,
}

fn main() {
    let x = 5u8;
    let foo: Foo = unsafe { std::mem::transmute(x) };
    println!("{:?}", foo.value);
    match foo.impossible {}
}

This thread suggests that this is UB, because we require the transmute to execute. Running this will warn that the type is unreachable and that the value is uninhabited, but the program will compile, and fail with a SIGILL if the uninhibited value is accessed:

warning: unreachable expression
  --> src/main.rs:12:34
   |
11 |             let foo: Foo = unsafe { std::mem::transmute(x) };
   |                                     ---------------------- any code following this expression is unreachable
12 |                 println!("{:?}", foo.value);
   |                                  ^^^ unreachable expression
   |
   = note: `#[warn(unreachable_code)]` on by default
note: this expression has type `Foo`, which is uninhabited
  --> src/main.rs:11:37
   |
11 |             let foo: Foo = unsafe { std::mem::transmute(x) };
   |                                     ^^^^^^^^^^^^^^^^^^^^^^

warning: unused variable: `foo`
  --> src/main.rs:11:17
   |
11 |             let foo: Foo = unsafe { std::mem::transmute(x) };
   |                 ^^^ help: if this is intentional, prefix it with an underscore: `_foo`
   |
   = note: `#[warn(unused_variables)]` on by default


running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

This came up from a review of hyper's proto::h2 module. It feels a little suspicious to me, but cargo miri didn't complain about it.

(Incidentally it appears this code is unnecessary, so we're trying to remove it).

@RalfJung
Copy link
Member

This is definitely UB, and Miri complains about your example.

@erickt
Copy link
Author

erickt commented May 10, 2022

Whoops, I was running cargo miri test, not cargo miri run, so it didn't trigger miri.

@erickt
Copy link
Author

erickt commented May 10, 2022

After talking with @djkoloski, I didn't accurately represent what hyper is doing. Instead, what they're doing is ultimately transmuting a Vec<T>, where T is some regular type, to Vec<U>, where U contains an uninhabited type. They then transmute it back before using the value. I think this better captures their pattern:

enum Impossible {}

#[repr(transparent)]
struct Item<T> {
    _value: T,
    _impossible: Impossible,
}

struct Container<T>(Vec<Item<T>>);

impl<T: std::fmt::Debug> Container<T> {
    fn new(values: Vec<T>) -> Self {
        assert_eq!(std::mem::size_of::<T>(), std::mem::size_of::<Item<T>>());
        let values = unsafe { std::mem::transmute(values) };
        Container(values)
    }

    fn print(&self) {
        let values: &Vec<T> = unsafe { &*(&self.0 as *const _ as *const _) };
        println!("{:?}", values);
    }
}

fn main() {
    let items = vec![5u8];
    let container = Container::new(items);
    container.print();
}

This passes a miri check:

% cargo +nightly miri run
[5]

@digama0
Copy link

digama0 commented May 10, 2022

I think you meant struct Container<T>(Vec<Item<T>>);, else the transmute is trivial...

I don't think the layout compatibility is guaranteed here: Vec<T> and Vec<Item<T>> don't have to have the same layout. There is some documentation which more or less promises that the layout won't change gratuitously, though, so you are probably fine.

Other than layout issues, there should not be any problems with this code. Validity does not traverse the data pointer in Vec, so you can cast it to another type as long as you cast it back before calling any Vec functions on it.

@erickt
Copy link
Author

erickt commented May 10, 2022

I think you meant struct Container(Vec<Item>);, else the transmute is trivial...

Oops, thanks, I've corrected my example.

Other than layout issues, there should not be any problems with this code. Validity does not traverse the data pointer in Vec, so you can cast it to another type as long as you cast it back before calling any Vec functions on it.

Thanks for this. Is that a property of pointer types? I swapped Box in for Vec, and it also passed a miri run. Does that mean it's sound to have a pointer to an impossible type, as long as we never try to access any values from it?

@digama0
Copy link

digama0 commented May 10, 2022

Thanks for this. Is that a property of pointer types? I swapped Box in for Vec, and it also passed a miri run. Does that mean it's sound to have a pointer to an impossible type, as long as we never try to access any values from it?

This is #77 . It is still open (not many issues actually get closed around here...) but the popular opinion seems to be that validity should not be recursive through pointer types, so Box<T> can be cast to Box<U> as long as you don't drop the box or call any functions on it while it has the "wrong" type.

@RalfJung
Copy link
Member

Note however that Box<!> and &! are likely indeed invalid types (though Miri does not currently check that yet) -- we don't need to actually look up anything in memory to conclude that these cannot point to anything valid.

Vec<!> is fine though of course, it can even be created in safe code.

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

No branches or pull requests

3 participants