offset_of is unsound

[Centril]

With the implementation in:

macro_rules! offset_of {
    ($parent:ty, $($field:tt)+) => (unsafe {
        let x: &'static $parent = $crate::Transmuter::<$parent> { int: 0 }.ptr;
        $crate::Transmuter { ptr: &x.$($field)+ }.int
    });
}

you are constructing a &'static T at address 0. This is undefined behavior because Rust may assume that you have a valid &'static T but you do not.

Moreover, in the old pre-1.33.0 implementation you have:

macro_rules! offset_of {
    ($father:ty, $($field:tt)+) => ({
        #[allow(unused_unsafe)]
        let root: $father = unsafe { $crate::mem::uninitialized() };

        let base = &root as *const _ as usize;

        // Future error: borrow of packed field requires unsafe function or block (error E0133)
        #[allow(unused_unsafe)]
        let member =  unsafe { &root.$($field)* as *const _ as usize };

        $crate::mem::forget(root);

        member - base
    });
}

Note that when you say &root you have already triggered undefined behavior because again, you assert with &root that you have a valid reference but you do not in fact.

See rust-lang/rfcs#2582 and rust-lang/unsafe-code-guidelines#77 for a discussion.

As for your comment about // Future error, do note that unsafe { ... } here does not make what you are doing any less undefined behavior. Instead, while you might not get an error from the compiler, you might get miscompilation should LLVM or rustc's behavior change.

cc @RalfJung

[RalfJung]

Note that when you say &root you have already triggered undefined behavior

Actually, mem::uninitialized already triggers UB (for most $father types). But that cannot be avoided until MaybeUninit hits stable -- which should happen in less than 2 weeks!

Also see https://gankro.github.io/blah/initialize-me-maybe/

[de-vri-es]

This already goes wrong in practice on aarch64. I compiled the example below with --target=aarch64-unknown-linux-gnu --release and ran the result on a Raspberry Pi 3B.

use memoffset::offset_of;

struct Foo {
	foo: i32,
}

fn main() {
	println!("foo: {}", offset_of!(Foo, foo));
}

The result of running the compiled binary on a Raspberry Pi 3B is that it hits a brk instruction which zsh reports as a trace trap:

./memoffset-minimum-example 
zsh: trace trap (core dumped)

A stacktrace with lldb:

lldb ./memoffset-minimum-example 
(lldb) target create "./memoffset-minimum-example"
Current executable set to './memoffset-minimum-example' (aarch64).
(lldb) r
Process 20353 launched: '/home/dronebox/memoffset-minimum-example' (aarch64)
Process 20353 stopped
* thread #1, name = 'memoffset-minim', stop reason = signal SIGTRAP
    frame #0: 0x0000aaaaaaaad944 memoffset-minimum-example`memoffset_minimum_example::main::h4461b5e7e8206c6c
memoffset-minimum-example`memoffset_minimum_example::main::h4461b5e7e8206c6c:
->  0xaaaaaaaad944 <+0>: brk    #0x1

memoffset-minimum-example`main:
    0xaaaaaaaad948 <+0>: str    x30, [sp, #-0x10]!
    0xaaaaaaaad94c <+4>: mov    x3, x1
    0xaaaaaaaad950 <+8>: adrp   x8, 0
(lldb) bt
* thread #1, name = 'memoffset-minim', stop reason = signal SIGTRAP
  * frame #0: 0x0000aaaaaaaad944 memoffset-minimum-example`memoffset_minimum_example::main::h4461b5e7e8206c6c
    frame #1: 0x0000aaaaaaaad984 memoffset-minimum-example`std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h87f1335357fe260f + 12
    frame #2: 0x0000aaaaaaab40d8 memoffset-minimum-example`std::panicking::try::do_call::he785b49af8496835 [inlined] std::rt::lang_start_internal::_$u7b$$u7b$closure$u7d$$u7d$::h6c6ea72b9d5d834b at rt.rs:49:12
    frame #3: 0x0000aaaaaaab40d0 memoffset-minimum-example`std::panicking::try::do_call::he785b49af8496835 at panicking.rs:294
    frame #4: 0x0000aaaaaaab58b4 memoffset-minimum-example`__rust_maybe_catch_panic at lib.rs:85:7
    frame #5: 0x0000aaaaaaab4998 memoffset-minimum-example`std::rt::lang_start_internal::h8c1220cbac1ca21e [inlined] std::panicking::try::h6bc78e31cf4d4a00 at panicking.rs:273:12
    frame #6: 0x0000aaaaaaab4970 memoffset-minimum-example`std::rt::lang_start_internal::h8c1220cbac1ca21e [inlined] std::panic::catch_unwind::h3e6ede07014eedd5 at panic.rs:388
    frame #7: 0x0000aaaaaaab4970 memoffset-minimum-example`std::rt::lang_start_internal::h8c1220cbac1ca21e at rt.rs:48
    frame #8: 0x0000aaaaaaaad970 memoffset-minimum-example`main + 40
    frame #9: 0x0000ffffbf5146e4 libc.so.6`__libc_start_main + 228
    frame #10: 0x0000aaaaaaaad86c memoffset-minimum-example`_start + 52
(lldb)

Compiled using rustc --version:

rustc 1.37.0-nightly (02564de47 2019-06-10)

[RalfJung]

That probably has the memoffset_constant_expression feature enabled because you used Rust 1.33 or newer? In that case it's "just" creating a NULL reference, which has been causing traps in the past as well. Still, nice catch!

[de-vri-es]

Indeed, it's compiled with the memoffset_constant_expression feature enabled.

[RalfJung]

Btw, another way in which this is unsound is that it does not protect against auto-deref: Something like offset_of!(Box<Foo>, foo) will return utter garbage, and also cause UB.

[RalfJung]

@Gilnaa btw, see this thread on IRLO for a version of the macro that fixes most of these problems. The comment about references to uninitialized data being okay is incorrect though, but they currently cannot be avoided until the lang team makes progress on rust-lang/rfcs#2582.

This macro doesn't work in const, but I am afraid that's just not a possibility right now. Also note that your macro does not work in const fn either, it only works in const items.

[Gilnaa]

I'll try to take care of it over the weekend.
The version they use doesn't allow for all kinds of use cases the macro uses now, so I'll have to see if I can modify it.

I apologise for the delay.

[Gilnaa]

AFAICT, there's no way to currently implement this correctly.
Using the pointer math is obviously not working.
I guess I should probably revert it to the older version by default.

Note that when you say &root you have already triggered undefined behavior because again, you assert with &root that you have a valid reference but you do not in fact.
cc @RalfJung

Why is that? Isn't it a valid reference to an invalid T?
Ignoring Deref, why is that UB?

[Centril]

@Gilnaa By "valid reference" here I mean that the answer to both questions in rust-lang/unsafe-code-guidelines#77:

does &T have to point to allocated memory (with at least size_of::() bytes being allocated)? If yes, does the memory have to contain data that satisfies the validity invariant of T?

is "yes".

(Answering both "yes" renders "valid reference to an invalid T" a contradiction in terms)

[Gilnaa]

I see.
So how does the MaybeUninit version solves this UB?

[RalfJung]

It doesn't. That's why I said above that implementing offsetof currently is impossible.

However, with MaybeUninit you can at least make a version that works, without causing SIGILL on ARM and other "fun" things.

[RalfJung]

Ignoring Deref, why is that UB?

One of the reasons we want it to be UB is that some people ask that a function like fn foo(x: &!) should be removed as dead code. We can only do that if &! is a type that cannot be inhabited, not even by safe code, without causing UB.

But that means &*MaybeUninit::<!>::zeroed().as_ptr() must be UB. We cannot allow a reference to an invalid !.

[Gilnaa]

I've removed the unsound impl and yanked the affected version from crates.io

[RalfJung]

Thanks!

The remaining impl is also still unsound. If you wait until Thursday, you can make it use a version with MaybeUninit at least on newer rustc that support that? I can help review this if you want.

And probably, on old rustc the following is safer than mem::uninitialized:

let base_ptr: &$father = NonNull::dangling().as_ref();
let field_ptr = &base_ptr.$field;
let offset = (field_ptr as *const _ as usize) - (base_ptr as *const _ as usize)

This way at least we only materialize bad references and perform an incorrect offset operations, instead of materializing bad values of any type.

Do you have any ideas for fixing the problem with auto-deref? TBH I am not sure if the API you want (that supports any number of nested fields) can be implemented correctly. And I am not talking about UB here, I am talking about:

struct S { x: u32 };
type Father = Box<S>;

let offset = offsetof!(Father, x); // *oops*

[Gilnaa]

I don't think anyone is actually using the nested field syntax, but it's probably pretty easy to check.
If it's as I suspect, I'd rather drop this feature completely in favour of soundness.

[Gilnaa]

Humm, it seems as if discord_game_sdk by @ldesgoui uses the nested version:
https://github.com/ldesgoui/discord_game_sdk/blob/master/mock/src/macros.rs#L20

$ rg from_ptr!
discord_game_sdk/mock/src/instance.rs
46:    from_ptr!(Instance, from_core, sys::IDiscordCore, interfaces.core);
47:    from_ptr!(Instance, from_application, sys::IDiscordApplicationManager, interfaces.applications);
48:    from_ptr!(Instance, from_user, sys::IDiscordUserManager, interfaces.users);
49:    from_ptr!(Instance, from_image, sys::IDiscordImageManager, interfaces.images);
50:    from_ptr!(Instance, from_activity, sys::IDiscordActivityManager, interfaces.activities);
51:    from_ptr!(Instance, from_relationship, sys::IDiscordRelationshipManager, interfaces.relationships);
52:    from_ptr!(Instance, from_lobby, sys::IDiscordLobbyManager, interfaces.lobbies);
53:    from_ptr!(Instance, from_network, sys::IDiscordNetworkManager, interfaces.networking);
54:    from_ptr!(Instance, from_overlay, sys::IDiscordOverlayManager, interfaces.overlay);
55:    from_ptr!(Instance, from_storage, sys::IDiscordStorageManager, interfaces.storage);
56:    from_ptr!(Instance, from_store, sys::IDiscordStoreManager, interfaces.store);
57:    from_ptr!(Instance, from_voice, sys::IDiscordVoiceManager, interfaces.voice);
58:    from_ptr!(Instance, from_achievement, sys::IDiscordAchievementManager, interfaces.achievements);

[ldesgoui]

Hello!

I can probably update to not use the nested function anymore, I might have the time to do that tomorrow.
Please don't wait on _mock to get updated, I don't think anyone is using it aside from me as it's extremely incomplete, I think it's fine broken for the time being.

Thank you for checking which libraries would be affected though!

[ldesgoui]

Oh, this is probably the reason why offset_of! was throwing SIGILLs in very particular cases (IIRC it only happened in threads), I had fixed the issue by binding the result to a const rather than to a let

[Gilnaa]

Wait. Wouldn't forcing the constness inside the macro fix this? @RalfJung

[RalfJung]

What is the concrete proposal?

[Gilnaa]

Declare a const inside of the macro to force compile time evaluation, and return it.

[RalfJung]

But which implementation do you want to use in the const? The union-based one?

Not sure how I feel about that... UB during CTFE can't really propagate to run-time, at least currently. However, we reserve the right to make CTFE complain loudly and fail if your const-code causes UB. I consider the fact that such code does not error a bug. So, your code would basically not be subject to Rust's stability guarantee. (That's my interpretation, anyway.)

[Centril]

Should this not be kept open until the soundness issue is resolved entirely (and not just the exploitable UB)?

[RalfJung]

Also, I have not looked closely at span_of but it's use of a Transmuter makes me suspect it has similar problems.

[Gilnaa]

I think spanof originally used offsetof and it probably still can

[RalfJung]

I'll close this issue and open a new one discussing the remaining soundness problems. Most of the discussion here is not relevant any more.

[RalfJung]

New issue is at #24.