[wasmer] multiple panics caused by unimplemented! macro

[pventuzelo]

Describe the bug

The macro unimplemented! is defined as following in: https://doc.rust-lang.org/1.6.0/std/macro.unimplemented!.html

A standardized placeholder for marking unfinished code. It panics with the message "not yet implemented" when executed.

This macro is used multiple time in wasmer and make it panic:

thread 'main' panicked at 'not yet implemented', lib/clif-backend/src/resolver.rs:261:67
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Vulnerable code

Based on github search in this repository, 31 files are using unimplemented! macro (https://github.com/wasmerio/wasmer/search?q=unimplemented%21&type=Code)

$ rg unimplemented!

src/webassembly.rs
56:    unimplemented!();

wapm-cli/src/config.rs
163:            unimplemented!()

src/bin/wasmer.rs
55:        unimplemented!()

lib/clif-backend/src/module.rs
111:            _ => unimplemented!("unsupported wasm type"),

lib/clif-backend/src/resolver.rs
260:                            VmCallKind::SharedStaticMemoryGrow => unimplemented!(),
261:                            VmCallKind::SharedStaticMemorySize => unimplemented!(),
278:                            VmCallKind::SharedStaticMemoryGrow => unimplemented!(),
279:                            VmCallKind::SharedStaticMemorySize => unimplemented!(),

lib/clif-backend/src/relocation.rs
108:        unimplemented!();
121:            _ => unimplemented!("unimplented reloc type: {}", reloc),
149:                        _ => unimplemented!(),
160:                        _ => unimplemented!(),
163:                    _ => unimplemented!(),
193:                    _ => unimplemented!("unimplemented libcall: {}", libcall),
211:        unimplemented!();

lib/clif-backend/src/code.rs
282:                    _ => unimplemented!(),

lib/runtime-core/src/vmcalls.rs
153:    unimplemented!()
159:    unimplemented!()

lib/runtime-core/src/vm.rs
211:                    MemoryType::SharedStatic => unimplemented!(),
219:                    MemoryType::SharedStatic => unimplemented!(),
868:                unimplemented!()
871:                unimplemented!()
876:                unimplemented!()

lib/runtime-core/src/instance.rs
495:        unimplemented!("Use the exports method instead");
512:        unimplemented!("Use the exports method instead");

lib/runtime-core/src/loader.rs
25:        unimplemented!()
29:        unimplemented!()
122:        unimplemented!();
126:        unimplemented!();

lib/spectests/tests/semantics.rs
38:                _ => unimplemented!(),

lib/emscripten/src/exception.rs
13:    unimplemented!()
18:    unimplemented!()
23:    unimplemented!()
28:    unimplemented!()

lib/llvm-backend/src/trampolines.rs
67:        _ => unimplemented!(),
113:        _ => unimplemented!("multi-value returns"),

lib/llvm-backend/src/code.rs
959:                            _ => unimplemented!(),
1383:                    _ => unimplemented!("multi-value returns"),
4464:                unimplemented!("{:?}", op);
4481:            _ => unimplemented!("multi-value returns not yet implemented"),

lib/singlepass-backend/src/codegen_x64.rs
1845:                    } //_ => unimplemented!(),

lib/runtime-c-api/src/value.rs
79:            Value::V128(_) => unimplemented!("V128 not supported in C API"),
117:            Type::V128 => unimplemented!("V128 not supported in C API"),

lib/runtime-c-api/src/export.rs
405:                    Value::V128(_) => unimplemented!("returning V128 type"),

lib/runtime-c-api/src/instance.rs
219:                    Value::V128(_) => unimplemented!("calling function with V128 parameter"),

lib/clif-backend/src/signal/unix.rs
101:                        _ => unimplemented!(),

lib/clif-backend/src/signal/windows.rs
113:    unimplemented!();

lib/runtime-abi/src/vfs/device_file.rs
11:        unimplemented!()
21:        unimplemented!()
27:        unimplemented!()
31:        unimplemented!()
37:        unimplemented!()
43:        unimplemented!()
53:        unimplemented!()
73:        unimplemented!()
79:        unimplemented!()
89:        unimplemented!()
109:        unimplemented!()

lib/runtime-core/src/memory/mod.rs
163:            MemoryVariant::Shared(_) => unimplemented!(),
298:        unimplemented!()
302:        unimplemented!()
308:        unimplemented!()

lib/emscripten/src/syscalls/windows.rs
69:    unimplemented!()
75:    unimplemented!()
89:    unimplemented!()
95:    unimplemented!()
116:    unimplemented!()
122:    unimplemented!()
128:    unimplemented!()
146:    unimplemented!()
152:    unimplemented!();
158:    unimplemented!();
165:    unimplemented!();
171:    unimplemented!()
177:    unimplemented!()
197:    unimplemented!()
212:    unimplemented!()
250:    unimplemented!();
303:    unimplemented!()
309:    unimplemented!()

lib/emscripten/src/syscalls/unix.rs
107:            unimplemented!("The ioctl {} is not yet implemented", wasm_ioctl);
262:    unimplemented!()
534:                unimplemented!("non blocking sockets");
1114:        unimplemented!()

lib/emscripten/src/io/mod.rs
18:    unimplemented!()
24:    unimplemented!()
56:    //unimplemented!()
62:    unimplemented!()

lib/emscripten/src/io/windows.rs
39:    unimplemented!()
45:    unimplemented!()

lib/wasi/src/syscalls/windows.rs
47:            unimplemented!("wasi::platform_clock_time_get(__WASI_CLOCK_PROCESS_CPUTIME_ID, ..)")
50:            unimplemented!("wasi::platform_clock_time_get(__WASI_CLOCK_THREAD_CPUTIME_ID, ..)")

lib/wasi/src/state/mod.rs
408:                    Kind::Buffer { .. } => unimplemented!("state::get_inode_at_path for buffers"),
467:                                    unimplemented!("Absolute symlinks are not yet supported");
477:                                unimplemented!("state::get_inode_at_path unknown file type: not file, directory, or symlink");
763:                    Kind::Symlink { .. } => unimplemented!(),

lib/wasi/src/syscalls/mod.rs
708:                Kind::Symlink { .. } => unimplemented!("Symlinks in wasi::fd_pread"),
848:                Kind::Symlink { .. } => unimplemented!("Symlinks in wasi::fd_pwrite"),
918:                Kind::Symlink { .. } => unimplemented!("Symlinks in wasi::fd_read"),
1115:                    unimplemented!("wasi::fd_seek not implemented for symlinks")
1257:                Kind::Symlink { .. } => unimplemented!("Symlinks in wasi::fd_write"),
1710:            Kind::Buffer { .. } => unimplemented!("wasi::path_open for Buffer type files"),
1726:                unimplemented!("SYMLINKS IN PATH_OPEN");
2032:        Kind::Dir { path, .. } => unimplemented!("wasi::path_rename on Directories"),
2198:            _ => unimplemented!("wasi::path_unlink_file for Buffer"),
2245:    unimplemented!("wasi::poll_oneoff")
2254:    unimplemented!("wasi::proc_raise")
2297:    unimplemented!("wasi::sock_recv")
2308:    unimplemented!("wasi::sock_send")
2312:    unimplemented!("wasi::sock_shutdown")

Expected behavior

unimplemented! should be replaced by an Error to prevent wasmer to panic.
This will require a bit of code rewritting :s

Steps to reproduce

This file is given as an example:
Download unimplemented.zip

$ unzip unimplemented.zip
$ wasmer run unimplemented.wasm
thread 'main' panicked at 'not yet implemented', lib/clif-backend/src/resolver.rs:261:67
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Status of my environment

wasmer version: commit 5239cdb

$ echo "`./target/release/wasmer -V` | `rustc -V` | `uname -m`"
wasmer 0.6.0 | rustc 1.38.0-nightly (ad7c55e1f 2019-08-07) | x86_64

[pventuzelo]

TODO: tag with fuzzer-trophy

[MarkMcCaskey]

unimplemented!() is intentionally used in WASI, for example, to mark code that isn't done yet. Code that terminates the program with a clear message is preferably to code that silently does the wrong thing, especially when dealing with security.

I think we shouldn't accept unimplemented!() without a string explaining why it's there, but that's a different discussion.

unimplemented is a blocker for a 1.0 release, but otherwise it's the correct behavior for unfinished code. Put another way, these panics are not bugs, they're features.

[pventuzelo]

We could have a dedicated Error like we have for CompileError, LinkError inside error.rs

In term of security, unimplemented!() trigger a panic that make wasmer to crash. In short, that means that if wasmer is used on server side (as a library for example), this panic lead to a DoS. If UnimplementedError was triggered instead, it can be easily catch and handle properly instead.

In case of under development feature/code, yes it make sense to have unimplemented code but i'm not sure about the value of calling this macro inside master branch.

[MarkMcCaskey]

@pventuzelo That's a good point; when used as an embedded runtime, having an error that the user can handle is definitely better -- I'm not sure what that would look like right now from a system call (though I'm sure we can make something work).

The intention in the code I've been working on is to give people a specific message that they can file an issue with to let us know that a feature that they want or need isn't ready, so that I can prioritize it. The idea is to get rid of them as fast as is possible, but as a very small company we have a lot more potential work than resources right now. We can do that with a proper error type, too, though.

In the case of Emscripten and to a much lesser extent WASI, if people are using it in production to run untrusted code right now, then their program terminating is the least of their concerns. We will be making them secure soon but resource constraints and all that. I do agree that for more stable and core parts, we should move toward removing unimplemented! as soon as we can.

[syrusakbary]

I think WASI and Emscripten are fine for now, but we should for sure solve it on the runtime-core side of things on the short term :)

[pventuzelo]

I will work on those fixes next week ;)

[pventuzelo]

Fix for unimplemented! in clif-backend/src/resolver.rs : issue #771, pull request #770

[pventuzelo]

I think we can close this issue.
A lot of unimplemented have been removed during the last 2 month ;)