Wasmtime GitHub notifications bot (Apr 16 2020 at 18:55): Wasmtime GitHub notifications bot (Apr 16 2020 at 18:55):sunfishcode commented on Issue #1525:
Does this mean that WASI functions like
readandwritewould reject a buffer starting at offset 0?
Wasmtime GitHub notifications bot (Apr 16 2020 at 19:31):pchickey commented on Issue #1525:
Yes, it does. I had to fix up the wat test cases (https://github.com/bytecodealliance/wasmtime/pull/1525/commits/d544abb18bf55fd571d1284b50902f0d93e6320d) for that exact reason.
I'm pretty sure that rejecting those buffers is desirable because it will catch bugs in code written with any LLVM based language. But it may be something we need to bubble up to the standard as well.
Wasmtime GitHub notifications bot (Apr 16 2020 at 19:43):github-actions[bot] commented on Issue #1525:
Subscribe to Label Action
cc @kubkon
<details>
This issue or pull request has been labeled: "wasi"Thus the following users have been cc'd because of the following labels:
- kubkon: wasi
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Apr 16 2020 at 20:37):sunfishcode commented on Issue #1525:
I don't see how we can avoid a spec change if we do this in Wasmtime. If the spec says address 0 is valid, then a program written to the spec might not run under Wasmtime.
Wasmtime GitHub notifications bot (Apr 16 2020 at 21:03):sunfishcode commented on Issue #1525:
One possible alternative would be to change the definition of "null pointer" on wasm to be an all-ones bitpattern instead of all-zeros. It would still convert to/from integer 0, and be falsy, so most code wouldn't notice the difference. This is valid for C/C++, and probably also Rust.
The advantage of doing that would be that, except for one-byte accesses in programs which allocate all 4 GiB of possible address space, all NULL pointer dereferences would trap, not just host calls.
A disadvantage is that it breaks some application assumptions. For example, if code assumes that zeroed-out memory (such as returned by
callocormemory.grow) contains pointers, that they are null pointers, it would break.But one could plausibly argue that due to the nature of wasm's address space, all-ones is a more natural choice.
Wasmtime GitHub notifications bot (Apr 16 2020 at 21:05):sunfishcode edited a comment on Issue #1525:
One possible alternative would be to change the definition of "null pointer" on wasm to be an all-ones bitpattern instead of all-zeros. It would still convert to/from integer 0, and be falsy, so most code wouldn't notice the difference. This is valid for C/C++, and probably also Rust.
The advantage of doing that would be that, except for one-byte accesses in programs which allocate all 4 GiB of possible address space, all NULL pointer dereferences would trap, not just host calls. [Edit: And actually, since C requires arrays have a representable one-past-the-end pointer, we should in theory avoid allocating anything in the last byte of memory anyway.]
A disadvantage is that it breaks some application assumptions. For example, if code assumes that zeroed-out memory (such as returned by
callocormemory.grow) contains pointers, that they are null pointers, it would break.But one could plausibly argue that due to the nature of wasm's address space, all-ones is a more natural choice.
Wasmtime GitHub notifications bot (Apr 16 2020 at 21:19):kubkon commented on Issue #1525:
I’m definitely not versed well enough in the spec, but how come this is surfacing only now? Does that mean that toolchains got away without having the concept of null pointer as a reserved memory region until now? I’m just trying to get some feeling for the magnitude of the problem here :-)
Wasmtime GitHub notifications bot (Apr 16 2020 at 21:26):pchickey commented on Issue #1525:
I agree with @sunfishcode that we should document this restriction (or a corresponding one for all-ones) on pointers in the WASI spec. I don't think I'm up for driving any changes to the existing toolchains to change how null pointers are represented, though. I do think it would be beneficial for C/C++ programs to trap when they try to dereference null, so I wouldn't object to any solution which makes that possible.
I should have stated my motivation for this change up front more clearly - we (Fastly) are transitioning to use Wiggle to handle Wasm programs using all-zeroes for null pointers today, and especially want to avoid writing to pointers which those programs did not intend to be written to. Some of our implementation (unfortunately, closed source for now) predating Wiggle checked for null before using a pointer. I need to preserve that behavior somehow to keep programs working, and this seemed like the most straightforward way. It will change the behavior of wasi-common as well, but I don't believe that wasi-libc depends on this behavior presently.
Wasmtime GitHub notifications bot (Apr 16 2020 at 21:33):kubkon commented on Issue #1525:
Ah, ok, thanks for some context @pchickey! Hmm, so here’s me wondering and making a bit of a blind shot, but would @sunfishcode’s suggestion warrant least changes to
wasi-commonor would it be the same regardless of the route we decided to take?
Wasmtime GitHub notifications bot (Apr 16 2020 at 21:35):pchickey commented on Issue #1525:
In my opinion, wiggle should be responsible for any changes related to this, and the only thing wasi-common should need is the changes in this PR - it needs to know how to report a null ptr using
$errnoand its test suite needs to not use newly-illegal behavior. It wouldn't hurt if it grew tests that null pointers are rejected, too.
Wasmtime GitHub notifications bot (Apr 16 2020 at 22:21):sunfishcode commented on Issue #1525:
Ordinary
storeinstructions would still silently succeed on address 0, as would libc functions likememcpy, so if you have memory you really want to avoid writing to, just having WASI check for it doesn't seem sufficient.There have been a few ideas for solving this problem discussed at the wasm spec level, such as defining an
mprotect-like mechanism which would allow the first page to literally trap, or allowing wasm linear memories to declare a "base offset", which would be added or subtracted from all accesses, however these are just ideas at this point.However, on the topic of checking WASI accesses, another option here would be to add NULL checks in WASI libc, which would avoid the question of whether Wasmtime or WASI themselves should enforce anything.
Wasmtime GitHub notifications bot (Apr 21 2020 at 16:44):kubkon commented on Issue #1525:
I just wanted to get a feel for this PR, i.e., whether we're planning on merging it as-is, or perhaps rethink it following @sunfishcode's ideas?
Wasmtime GitHub notifications bot (Apr 21 2020 at 17:26):pchickey commented on Issue #1525:
I am going to set this aside and re-think it. I still think its valuable to handle a nullable pointer but I don't want to get too far down this rabbit hole right now.
Last updated: Nov 22 2024 at 16:03 UTC