Likely undefined behavior in Wasm blog post

[MarkMcCaskey]

Hello! I'm still in the process of reading your wasm as a platform for abstraction blogpost and I love it! I actually work on Wasmer and we're super impressed with the depth of your post!

The use of Ctx::data is a bit subtle; it doesn't have to be reset on every host call if you don't want it to be, and the code:

    let mut state = State { env };
    let instance = &mut self.instance;

    // point the data pointer at our temporary state.
    instance.context_mut().data = &mut state as *mut State<'_> as *mut _;
    // we can't use the old state variable any more (we'd have aliased
    // pointers) so deliberately shadow it
    #[allow(unused_variables)]
    let state = ();

Is a bit concerning because with non-lexcial lifetimes I believe it's valid for Rust to call drop on your state here and then any access to Ctx::data would be accessing freed memory.

The way I've been avoiding this when I do this is to use Box::into_raw(Box::new(...)) and then using Box::from_raw to destroy it when I want to.

I don't have a full mental model of your system yet, I'll keep reading and then edit this post if I realize I've made a mistake!

[MarkMcCaskey]

If you know it will only be accessed in this scope, you can use std::mem::forget and then manually call drop (if you need to); I think Rust will automatically clean up the stack even if you mem::forget on the stack. Though now that I think of it, I actually have no idea what Rust's guarantees are in regards to the integrity of the stack, given NLL, it seems like it could be mutating the stack 🤔 to drop things.

[Michael-F-Bryan]

Ooh, that's quite subtle! I'm guessing NLL won't provide any guarantees around when things are dropped because that would let people become coupled to the implementation.

I guess I could remove the let state = () statement and put a drop(state) right at the end, that explicitly would extend state's lifetime until after it's no longer needed.

I've made a thread on the internal forums. Hopefully people smarter than I will be able to shed some light on the situation.

The use of Ctx::data is a bit subtle; it doesn't have to be reset on every host call if you don't want it to be

This same point was brought up when proofreading the article. I've opted to clear the context.data pointer because if (somehow, maybe through callbacks or future michael forgetting invariants while adding more code) something called WASM code which invokes a host function without first passing through with_environment_context() then it's possible to observe a dangling pointer. Nulling out the context.data pointer is a belts and braces thing to make doubly sure things don't go wrong.

[Michael-F-Bryan]

This Comment seems pretty relevant:

The person who filed the issue is incorrect. NLL is only a change in compile-time borrow checking. It did not change the run-time behavior of any programs, and in particular it did not change when drop gets called. A value is still dropped deterministically, when its owner goes out of scope or is reassigned.

[Michael-F-Bryan]

In light of mbrubeck's comment I don't think this is actually UB. NLL is purely a compile-time check and won't have any affect on runtime code (the "with non-lexcial lifetimes I believe it's valid for Rust to call drop on your state here" comment).

[CAD97]

Specifically, if a type implements Drop, that type is dropped at the end of scope no matter what. NLL's shorter variable liveness only applies to types with no drop glue: to a first order of approximation, Copy types.

So long as your type has drop glue (it implements Drop or contains a structure that recursively has drop glue), it will deterministically be dropped at the end of scope.

[MarkMcCaskey]

Thanks for asking about it! It's always nice to update my mental model of these things!

I'm glad to hear that Rust isn't doing anything too fancy with NLL, like eagerly dropping values after last use!

@CAD97

So long as your type has drop glue (it implements Drop or contains a structure that recursively has drop glue), it will deterministically be dropped at the end of scope.

Does shadowing the variable here count as reassignment or by reassignment do they mean overwriting the value at that location?

A value is still dropped deterministically, when its owner goes out of scope or is reassigned.

I could read the above quote to mean either, though I'm guessing it probably doesn't include shadowing.

If that is the case, then the original quoted code is using a dropped value, but it's not because of NLL, because the reassignment (shadowing).

---

This same point was brought up when proofreading the article. I've opted to clear the context.data pointer because if (somehow, maybe through callbacks or future michael forgetting invariants while adding more code) something called WASM code which invokes a host function without first passing through with_environment_context() then it's possible to observe a dangling pointer. Nulling out the context.data pointer is a belts and braces thing to make doubly sure things don't go wrong.

If I understand it correctly, it should be possible to use safe Rust types for this if you wanted to. You'd have to create a wrapper type that you store a pointer to your Ctx::data and inside your always valid pointer, you could store any data you want using enums, structs, or whatever else. So your with_environment_context could interact with safer Rust data structures allowing you to write your code without checking for null pointers.

That's more of a personal preference thing though!

[CAD97]

@MarkMcCaskey shadowing just makes the value inaccessible*. Reassignment is actually rebinding the same mut binding to a new value.

Here's a playground that shows off some of the behavior: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c04ffccccd5a844d6ef147a14202d60d

*actually, macros mean that's not the case 100% of the time! See the playground link.

[MarkMcCaskey]

@CAD97 Wow, that's super useful! I shared that with my team -- thanks for the explanation and example!

[danielhenrymantilla]

By the way, regarding the initial pattern, I am more fond of doing it like this:

    let ref mut state = State { env };

    // point the data pointer at our temporary state.
    instance.context_mut().data = { state } as *mut State<'_> as *mut _;

As you can see in https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f2083bd6bcd9a37e053fc7833887158c, the state binding is indeed unusable afterwards, thanks to { state } having been moved rather than reborrowed.

[Michael-F-Bryan]

By the way, regarding the initial pattern, I am more fond of doing it like this:

That's a really nice way to use patterns! I might leave it as-is though seeing as the let ref foo = ... syntax isn't something most people would recognise at a glance, and I'm trying not to use too many "smart" tricks to make my unsafe code easier to verify.

[danielhenrymantilla]

Note that using ref mut is just my personal style, a &mut on the rhs works just as fine: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d19d8c9d1333fc45d69ddd3422778046