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

Cannot use 0 as an address in wasm in with opt-level >= 1 #57897

Closed
surma opened this issue Jan 25, 2019 · 15 comments
Closed

Cannot use 0 as an address in wasm in with opt-level >= 1 #57897

surma opened this issue Jan 25, 2019 · 15 comments
Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/

Comments

@surma
Copy link

surma commented Jan 25, 2019

I want to use Rust to work on the raw chunk of memory that WebAssembly creates for me. Consider the following rust code:

#![no_std]
#![no_main]

use core::panic::PanicInfo;
use core::slice::from_raw_parts_mut;

#[no_mangle]
fn test() {
  let in_b: &mut [u32];

  unsafe {
    in_b = from_raw_parts_mut::<u32>(0 as *mut u32, 3);
  }

  in_b[0] = 123;
}


#[panic_handler]
fn panic(_info: &PanicInfo) -> ! {
  loop {}
}

Compile with

rustc --target=wasm32-unknown-unknown -o rust.wasm rotate.rs

And load with this HTML file:

<!DOCTYPE html>
<script>
  async function init() {
    const { instance } = await WebAssembly.instantiate(
      await fetch("/rust.wasm").then(r => r.arrayBuffer())
    );
    instance.exports.test();
    console.log(new Uint32Array(instance.exports.memory.buffer, 0).slice(0, 3));
  }
  init();
</script>

This will work fine (i.e. no errors).

If you add -C opt-level=1 (or 2 or 3) to the rustc invocation, the wasm module will fail with unreachable.

If you change the rust code as following

-    in_b = from_raw_parts_mut::<u32>(0 as *mut u32, 3);
+    in_b = from_raw_parts_mut::<u32>(4 as *mut u32, 3);

the code will work again.

I think if I can use 0 as an address in a debug build I should also be able to do it in release mode 😄

cc @ashleygwilliams

@Centril Centril added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Jan 25, 2019
@nagisa
Copy link
Member

nagisa commented Jan 25, 2019

Oh my, 0 pointer not being a null strikes!

@Centril
Copy link
Contributor

Centril commented Jan 25, 2019

from_raw_parts_mut explicitly states that the data pointer (you've passed ptr::null_mut() to it) must not be null. Therefore the behavior of your program above is undefined.

@Centril Centril changed the title Canntot use 0 as an address in wasm in with opt-level >= 1 Cannot use 0 as an address in wasm in with opt-level >= 1 Jan 25, 2019
@surma
Copy link
Author

surma commented Jan 25, 2019

Ha, I really should read the documentation when using unsafe stuff. Thanks for pointing that out.

It’s completely fair to not allow address 0, but can we then make sure that the program will panic in debug mode? Having it coincidentally do the right in debug while it crashes when doing a release build is very unfortunate.

@Centril
Copy link
Contributor

Centril commented Jan 26, 2019

@surma hmm... the function has debug_assert!(data as usize % mem::align_of::<T>() == 0, ...);; maybe it didn't panic due to the #[panic_handler]? -- ah no; that's because std's debug_assert doesn't show up in your code; only when building std itself... iirc.

@surma
Copy link
Author

surma commented Jan 26, 2019

Even if it was, it would only be checking for alignment, not for a non-zero value, wouldn’t it?

@Centril
Copy link
Contributor

Centril commented Jan 26, 2019

@surma yeah my bad; we should perhaps add debug_assert!(data != ptr::null_mut(), ..); -- make a PR for that and see what folks think?

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Jan 26, 2019

The pre-built binaries distributed through rustup are built without debug assertions, so neither the existing alignment check nor a new (debug-assertion-only) null check would actually reach most users.

@surma
Copy link
Author

surma commented Jan 26, 2019

neither the existing alignment check nor a new (debug-assertion-only) null check would actually reach most users.

So what changes would need to be made to make this program crash in debug mode, and not only in release mode?

@nagisa
Copy link
Member

nagisa commented Jan 26, 2019

Invocation of undefined behaviour yields undefined results, so we cannot and will not guarantee crash, crash-free or matching behaviour at any of the optimisation levels.

I’m sympathetic to the observation that 0 is an absolutely valid pointer in WASM and perhaps std::mem::nullptr should be something else, however this is and will increasingly be extremely difficult to change. I believe, doing so would involve making changes to the whole WASM ecosystem (including runtimes, C code, etc.)

I do not see how this issue is actionable in any way as is.

@surma
Copy link
Author

surma commented Jan 26, 2019

I am not expecting 0 to work as an address just for WebAssembly. That is a massive change and not reasonable or proportional. Agreed.

I do feel like it would be reasonable to add some debug-only code that guarantees a crash in debug mode, but that code would get stripped in release mode. Or is that not possible?

In release mode I am of course 100% okay with undefined behavior being exactly that, undefined.

@Centril
Copy link
Contributor

Centril commented Jan 26, 2019

@surma Another option is to try out the cargo miri tool and see if that catches any UB (also available on playground).

@nikomatsakis
Copy link
Contributor

@surma

I do feel like it would be reasonable to add some debug-only code that guarantees a crash in debug mode, but that code would get stripped in release mode. Or is that not possible?

It's not impossible, but it is pretty hard. The problem is that the stdlib -- at present -- is always optimized. We've talked for a long time about having the ability to rebuild the stdlib with different options, or at least shipping a "debug version", in part so that we can add assertions like this, but it's not something we have the capacity to do right now. So, that would imply that we'd have to add the assertion also in optimized mode, which we don't really want to do.

@surma
Copy link
Author

surma commented Jan 28, 2019

Ah understood. Fair enough. Thanks for clarifying.

@RalfJung
Copy link
Member

Also see #53871

@alexcrichton
Copy link
Member

I'm going to close this because this is something we're pretty unlikely to ever support in wasm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/
Projects
None yet
Development

No branches or pull requests

7 participants