Docs driveby fix for Memory::data

[pepyakin]

Reshape the safety warning around mutable aliases instead of threads.

[yurydelendik]

The removed doc is true as well. Can we keep it as well?

[peterhuene]

Perhaps I'm missing something. Isn't Memory currently !Send and !Sync? How would another thread invalidate the underlying pointer by growing the memory?

[peterhuene]

Also, should data be &mut self since it returns a mutating slice reference?

[peterhuene]

This appears to conflict with #812 from @alexcrichton which might answer some of my questions above.

[yurydelendik]

How would another thread invalidate the underlying pointer by growing the memory?

The exact problem is here https://www.reddit.com/r/rust/comments/e18jcq/how_to_read_and_write_to_memory_in_wasmtime/ , notice discussion around getting mem.data_ptr() after the call.

[peterhuene]

Agreed that storing the slice reference returned by data, invoking a WebAssembly function, and then proceeding to use previously stored reference might result in invalidation from the memory growing during the execution of the WebAssembly.

I was failing to see, however, how data wasn't thread-safe given the current implementation limitations. We can't send instances between threads (#793), so how could another thread grow the memory?

At any rate, I think this PR might now be superseded by #812.

[alexcrichton]

Yes to clarify there's a large number of reasons that this is an unsafe API, and it doesn't require threads for this to be unsafe. Also sorry yeah I didn't look into this before writing up #812 which expands on the unsafety reasons here as well.

To answer your questions though @peterhuene:

Perhaps I'm missing something. Isn't Memory currently !Send and !Sync?

Correct!

How would another thread invalidate the underlying pointer by growing the memory?

While true that other threads can't update this, you could still have something like:

let ptr = memory.data();
instance.call_export("foo"); // invalidates `ptr` by calling `memory.grow`
println!("{:?}", ptr); // reads stale data

(basically this is just a "bland use-after-free")

Also, should data be &mut self since it returns a mutating slice reference?

Due to the aliasing of so many internals here that actually doesn't end up buying us anything. I recently added internal reference counting so you could actually do:

let mut memory = ...;
let mut memory2 = memory.clone();

assert_eq!(memory.data(), memory2.data()); // aliasing mutable pointers!

Even if we didn't have reference counting you still have the above problem:

let mut view = memory.data();
instance.call_export("foo"); // technically referred to by instance, so `view` is still aliased
println!("{:?}", view); // use-after-free

The only way Rust's type system would help here is if we would have something like Instance using no reference counting, Memory using no reference counting, and a fn (&mut Instance) -> &mut Memory method. That way we can guarantee that when you're looking at &mut Memory no one else in the world can possibly be looking at it, so you can safely get &mut.

Even if we did that, however, then you couldn't share memories across threads which is something we'd like to support eventually!

Overall I think we'll need to stick to "all memory access are unsafe" and we'll need to build up safe abstractions (like interface types) which encapsulate the unsafety. Otherwise we can be very very careful to document the various unsafeties.

[peterhuene]

@pepyakin thanks for putting up this PR! Given @alexcrichton's incoming changes to the API and subsequent updates to the comments to this method, I'm closing this as superseded by #812.

Please comment on that PR if the updated doc comments are insufficient to convey the changes made in this PR.

Thanks!

[peterhuene]

@alexcrichton agreed that there is a general aliasing problem, which is why I thought the change here was a more "general purpose" warning than the previous message.

I appreciate the additional detail you've added in #812!