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

offset_of (and the other macros, too) is unsound #24

Open
RalfJung opened this issue Aug 16, 2019 · 12 comments
Open

offset_of (and the other macros, too) is unsound #24

RalfJung opened this issue Aug 16, 2019 · 12 comments

Comments

@RalfJung
Copy link
Collaborator

@RalfJung RalfJung commented Aug 16, 2019

offset_of is UB because we are creating a reference to a field that has not been initialized yet. (Miri does not currently check references transitively, so tests still pass in Miri.) There is currently no way to avoid this UB when implementing offset_of!. progress is blocked on rust-lang/rfcs#2582.

We are also "more UB" for older versions of rustc, because we are dereferencing a dangling pointer. We only do it to compute a field address, so no memory access happens, but the dereference is enough to cause UB.

See #9 for other ways in which there used to be "more UB".

@RalfJung RalfJung changed the title offset_of (and the other macros, too) is UB offset_of (and the other macros, too) is unsound Aug 16, 2019
@est31 est31 mentioned this issue Aug 17, 2019
4 of 5 tasks complete
@dpc

This comment has been minimized.

Copy link

@dpc dpc commented Aug 25, 2019

I was reviewing this crate as a part of casual cargo-crev review, and I've noticed

// Get the field address. This is UB because we are creating a reference to
. I was going to ask what's the deal with that, and then I've noticed this github issue.

So... what's the end result? Should this crate be reported as unsafe to use right now, and RustSec advisory be filled against it?

@RalfJung

This comment has been minimized.

Copy link
Collaborator Author

@RalfJung RalfJung commented Aug 26, 2019

We are not aware of any case where using this crate from safe code causes actual problems. It is UB wrt. the Rust spec, but it turns out that the LLVM IR we generate is not UB -- and we will take care to update this crate should the compiler ever change in that regard.

Deliberately having unsound code like this is unfortunate, but this crate is by far not the only one doing that. See rust-lang/unsafe-code-guidelines#158 for a non-exhaustive list.

@nico-abram

This comment has been minimized.

Copy link

@nico-abram nico-abram commented Aug 26, 2019

Should a rustc version that doe misscompile or generates UB in the IR exist, would that be enough grounds to file a rustsec advisory, negative crev reviews, and request crate yanking?

@RalfJung

This comment has been minimized.

Copy link
Collaborator Author

@RalfJung RalfJung commented Aug 26, 2019

I suppose so, but that should be evaluated when/if it happens.

@ogoffart

This comment has been minimized.

Copy link

@ogoffart ogoffart commented Sep 27, 2019

@RalfJung How would the RFC 2582 help?

since doing &raw (*uninit).field would still be UB as it creates a reference from an invalid pointer. (If i'm not mistaken)

Also, will we be able to compute the offset in a const function?

(Currently, i'm using this trick to compute the offset in a static variable:

struct Foo {  _d : u32,  field : u8 }
union Transmuter<From : Copy, To : Copy> { from : From, to: To }
static OFFSET : usize = unsafe { Transmuter::<*const _, usize>{ from: (&Transmuter::<usize, &Foo>{ from: 0 }.to.field) as *const _ }.to };

But this would also still be UB even when using &raw because we'd still dereference an invalid pointer to get its field)

@nvzqz

This comment has been minimized.

Copy link

@nvzqz nvzqz commented Oct 6, 2019

I want to add assert_field_offsets! to static_assertions (see nvzqz/static-assertions-rs#22) and I'm dealing with the soundness issue as well. It requires being able to get field offsets in a const context.

@RalfJung

This comment has been minimized.

Copy link
Collaborator Author

@RalfJung RalfJung commented Oct 8, 2019

since doing &raw (*uninit).field would still be UB as it creates a reference from an invalid pointer

Where is there an invalid pointer here? uninit is not a valid reference but we never use it as such.

Currently, i'm using this trick to compute the offset in a static variable

This is very UB as you are creating a NULL reference. You don't even need to dereference that to cause UB.
This also errors on latest nightly, which got better at detecting broken const code like this (that was a side-effect of cleaning up reference and pointer checks in const-eval and fixing a critical soundness bug in Miri):

error[E0080]: could not evaluate static initializer
 --> src/lib.rs:3:71
  |
3 | static OFFSET : usize = unsafe { Transmuter::<*const _, usize>{ from: (&Transmuter::<usize, &Foo>{ from: 0 }.to.field) as *const _ }.to };
  |                                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ invalid use of NULL pointer

It requires being able to get field offsets in a const context.

Right now, that's not possible I am afraid. It's a language feature that Rust does not have yet, comparable to things like const generics or associated type constructors.

However, with rust-lang/rust#63810 we can basically use the variant this crate uses in const-eval. It's still UB though and needs &raw for a proper fix.

@RalfJung

This comment has been minimized.

Copy link
Collaborator Author

@RalfJung RalfJung commented Oct 8, 2019

Notice however that this issue here is about unsoundness of the current offset_of! macro. Any discussion of const-time usage is off-topic here.

#4 is the issue for that.

@ogoffart

This comment has been minimized.

Copy link

@ogoffart ogoffart commented Oct 8, 2019

... `&raw (*uninit).field' ...

Where is there an invalid pointer here?

Is the (*uninit) part not dereferencing the pointer and therefore UB if uninit is not a valid pointer pointing to something?
Or is it fine because we never actually access the pointed memory. (unlike in C++, where (*uninit) is UB even if one does not read or write the value)

@RalfJung

This comment has been minimized.

Copy link
Collaborator Author

@RalfJung RalfJung commented Oct 9, 2019

Is the (*uninit) part not dereferencing the pointer and therefore UB if uninit is not a valid pointer pointing to something?

Quoting from the reference:

Dereferencing (using the * operator on) a dangling or unaligned raw pointer.

The pointer is allocated and aligned. So we do not hit any of the UB clauses.

unlike in C++, where (*uninit) is UB even if one does not read or write the value

In C++, uninit memory contains an "indeterminate value" and I don't think constructing an lvalue (or whatever they call them these days) with an indeterminate value is UB. I would expect UB to only arise when the lvaue is turned into an rvalue. I might be wrong though, and it does not matter much here.

@Nemo157

This comment has been minimized.

Copy link

@Nemo157 Nemo157 commented Jan 15, 2020

Given that raw_ref_op is implemented now, it seems like it should be possible to write a sound unstable offset_of! macro now?

I did a quick copy-and-edit of offset_of! using &raw const to get a pointer to the field and the tests are at least passing: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=12e127ad97313cd137bc27b349a5a55e

@RalfJung

This comment has been minimized.

Copy link
Collaborator Author

@RalfJung RalfJung commented Jan 15, 2020

@Nemo157 yes, that looks great. :)
But this could only be done on nightly right now. I guess we could add yet another cfg-flag in build.rs for this... do you want to try make a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.