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

EFAULT or trap for out of range memory access during a wasi call? #505

Closed
yamt opened this issue Jan 13, 2023 · 6 comments
Closed

EFAULT or trap for out of range memory access during a wasi call? #505

yamt opened this issue Jan 13, 2023 · 6 comments

Comments

@yamt
Copy link
Contributor

yamt commented Jan 13, 2023

it seems that runtimes disagree on how to handle out of range access.
a quick example: https://github.com/yamt/toywasm/blob/master/wat/wasi/efault.wat

is there any authoritative text to require either behaviors or to allow both of them?
i guess EFAULT is a natural choice if you consider wasi an analogous of system calls.

@sunfishcode
Copy link
Member

In the component-model, Canonical ABI, it has traps like trap_if(ptr + length * size(elem_type) > len(cx.opts.memory)) for places where pointers are passed in. There isn't any documentation of this for preview1 yet, but I propose it should also trap.

EFAULT is the conventional Unix syscall thing to do. But Unix does this because it's unable to deliver a SIGSEGV for a trap that happened in the kernel. A signal would require the ability to save and restore the kernel stack, which would add a lot of complexity, given how Unix is designed.

In WASI, there is no fundamental difference between "syscall" and 'library call". Even things which are modeled after Unix syscalls, like wasi-filesystem, could just as well be implemented as library calls to other wasm code. If you pass an out-of-bounds pointer to a library to dereference, it would typically be expected to trap.

This ability to virtualize APIs and treat calls to wasm and calls to the host as interchangeable is one of the unique strengths of Wasm. Passing an out-of-bounds address to most APIs means the application has a bug, and it's the kind of bug that is strongly correlated with memory corruption, and it's better to trap the program than to give it an error code to handle and hope for the best.

@yamt
Copy link
Contributor Author

yamt commented Jan 14, 2023

In the component-model, Canonical ABI, it has traps like trap_if(ptr + length * size(elem_type) > len(cx.opts.memory)) for places where pointers are passed in. There isn't any documentation of this for preview1 yet, but I propose it should also trap.

ok.

EFAULT is the conventional Unix syscall thing to do. But Unix does this because it's unable to deliver a SIGSEGV for a trap that happened in the kernel. A signal would require the ability to save and restore the kernel stack, which would add a lot of complexity, given how Unix is designed.

In WASI, there is no fundamental difference between "syscall" and 'library call". Even things which are modeled after Unix syscalls, like wasi-filesystem, could just as well be implemented as library calls to other wasm code. If you pass an out-of-bounds pointer to a library to dereference, it would typically be expected to trap.

This ability to virtualize APIs and treat calls to wasm and calls to the host as interchangeable is one of the unique strengths of Wasm. Passing an out-of-bounds address to most APIs means the application has a bug, and it's the kind of bug that is strongly correlated with memory corruption, and it's better to trap the program than to give it an error code to handle and hope for the best.

is it really interchangeable? i thought a host implementation was somehow assumed.

my understanding is that, to use wasi, a module needs to:

  • import functions from the wasi implementation
  • export memory to the wasi implementation

can wasm have a way to handle this kind of circular dependencies among modules?

@sunfishcode
Copy link
Member

It is somewhat awkward to do with preview1 binaries, but it is doable. For example, we're taking advantage of this very property in the preview1-to-preview2 polyfill we're building here, which implements the preview1 API with wasm implementations which call preview2 APIs.

@yamt
Copy link
Contributor Author

yamt commented Jan 14, 2023

It is somewhat awkward to do with preview1 binaries, but it is doable. For example, we're taking advantage of this very property in the preview1-to-preview2 polyfill we're building here, which implements the preview1 API with wasm implementations which call preview2 APIs.

interesting. i will probably take a look. thank you.

yamt added a commit to yamt/toywasm that referenced this issue Jan 14, 2023
The previous logic was broken.
It was returning EFAULT (which indicating a trap in memory_getptr)
to the caller without processing the trap properly.

This commit fixes it by distinguishing user-level error and
host-level error. EFAULT for traps is a host-level error.

cf. WebAssembly/WASI#505
yamt added a commit to yamt/toywasm that referenced this issue Jan 14, 2023
The previous logic was broken.
It was returning EFAULT (which indicating a trap in memory_getptr)
to the caller without processing the trap properly.

This commit fixes it by distinguishing user-level error and
host-level error. EFAULT for traps is a host-level error.

cf. WebAssembly/WASI#505
yamt added a commit to yamt/toywasm that referenced this issue Jan 14, 2023
The previous logic was broken.
It was returning EFAULT (which indicating a trap in memory_getptr)
to the caller without processing the trap properly.

This commit fixes it by distinguishing user-level error and
host-level error. EFAULT for traps is a host-level error.

cf. WebAssembly/WASI#505
yamt added a commit to yamt/toywasm that referenced this issue Jan 14, 2023
The previous logic was broken.
It was returning EFAULT (which indicating a trap in memory_getptr)
to the caller without processing the trap properly.

This commit fixes it by distinguishing user-level error and
host-level error. EFAULT for traps is a host-level error.

cf. WebAssembly/WASI#505
@yamt
Copy link
Contributor Author

yamt commented Feb 8, 2023

It is somewhat awkward to do with preview1 binaries, but it is doable. For example, we're taking advantage of this very property in the preview1-to-preview2 polyfill we're building here, which implements the preview1 API with wasm implementations which call preview2 APIs.

interesting. i will probably take a look. thank you.

i took a look and ended up with a simpler demonstration without component-model: https://github.com/yamt/toywasm/tree/master/examples/wasi_hook
i think i now understand what you meant by "somewhat awkward"...

@sunfishcode
Copy link
Member

With #536, this is now documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants