wasmtime / issue #3714 Invalid clock ID leads to fatal error · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #3714 Invalid clock ID leads to fatal error

Wasmtime GitHub notifications bot (Jan 22 2022 at 00:15):

kripken labeled issue #3714:

This happens when sending an invalid clock id to wasi_snapshot_preview1::clock_time_get or clock_res_get. In emscripten we have a test that sends 42, if the specific value matters, which is how we noticed this. (The test is checking for error handling.)

History:

In wasmtime 0.8.0 an error is returned.

In 0.18.0 a warning is printed out. (This extra logging makes the emscripten test fail, which is how we noticed this.)

In 0.33.0 the VM hits a fatal error:
Caused by:
    0: failed to invoke command default
    1: In func wasi_snapshot_preview1::clock_time_get at convert Clockid: Invalid enum value Clockid
       wasm backtrace:
           0:  0x917 - <unknown>!__clock_gettime
           1:  0x6cb - <unknown>!__original_main
           2:  0x8e5 - <unknown>!_start
I would assume that an invalid clock id should lead to an error and not extra logging or a fatal VM error, but reading the docs I can't actually find an answer to that... Apologies if this is not a bug!

Emscripten issue where this was noticed: https://github.com/emscripten-core/emscripten/pull/16076

Wasmtime GitHub notifications bot (Jan 22 2022 at 00:15):

kripken opened issue #3714:

This happens when sending an invalid clock id to wasi_snapshot_preview1::clock_time_get or clock_res_get. In emscripten we have a test that sends 42, if the specific value matters, which is how we noticed this. (The test is checking for error handling.)

History:

In wasmtime 0.8.0 an error is returned.

In 0.18.0 a warning is printed out. (This extra logging makes the emscripten test fail, which is how we noticed this.)

In 0.33.0 the VM hits a fatal error:
Caused by:
    0: failed to invoke command default
    1: In func wasi_snapshot_preview1::clock_time_get at convert Clockid: Invalid enum value Clockid
       wasm backtrace:
           0:  0x917 - <unknown>!__clock_gettime
           1:  0x6cb - <unknown>!__original_main
           2:  0x8e5 - <unknown>!_start
I would assume that an invalid clock id should lead to an error and not extra logging or a fatal VM error, but reading the docs I can't actually find an answer to that... Apologies if this is not a bug!

Emscripten issue where this was noticed: https://github.com/emscripten-core/emscripten/pull/16076

Wasmtime GitHub notifications bot (Jan 22 2022 at 01:08):

pchickey commented on issue #3714:

Clockid is defined in the witx as an enum (as opposed to as a handle, like an fd is), its an ABI error to pass an out-of-bounds value for it, and ABI errors trap execution, rather than return an error value.

This behavior has been in wasmtime for a while, since https://github.com/bytecodealliance/wasmtime/pull/2487.

Wasmtime GitHub notifications bot (Jan 24 2022 at 13:54):

sunfishcode commented on issue #3714:

I can also add that this is something we'll revisit with the switch to wit tooling. The Unix/Linux clock_gettime operates on several different kinds of clocks, so I'm imagining wasi libc will take over the initial clock ID dispatch, and it can report an EINVAL, allowing users to dynamically detect supported clocks.

Wasmtime GitHub notifications bot (Jan 24 2022 at 16:25):

kripken commented on issue #3714:

I see, thanks for the info @pchickey and @sunfishcode !

Is this documented somewhere? Just curious where I should have looked. Also, this is not consistent between wasm VMs - I see that latest wasmer returns an error code.

I wonder if this is a settled issue? If not, I'd actually support the older behavior. That is, if the VM is going to check the code anyhow at runtime, in order to abort if it is incorrect, then it could as easily return an error code in that case, and doing so would allow libc code to be smaller (if it has chosen the constants properly to allow that). It's just a few bytes, of course, but the same principle would apply elsewhere, and maybe all those extra unnecessary bytes add up. Seems nicer to do such things once in the VM when possible rather than constantly in userspace?

Wasmtime GitHub notifications bot (Jan 24 2022 at 17:33):

sunfishcode commented on issue #3714:

It's not specified or documented yet. WASI has been a complex landscape while interface-types has gone through several rounds of evolution underneath it.

If the only goal is minimal code size in C programs in the short term, we could go even further, and bake clock_gettime directly into WASI, along with errno for reporting errors. However, while we do care about code size, we care about it along with other things too.

Outside of C and its descendents, there often isn't a single unified clock index space. For example, in Rust's standard library, there's a different function for querying the real-time clock versus querying the monotonic clock. These two clocks have mostly disjoint use cases. It's also the case that the real-time clock may want to return a wider value (often secs + nsecs) to be able to talk about dates at points in the far (but not impossibly far) future, while a single u64 nsecs is likely to be sufficient for the monotonic clock. Having separate imports for the two reduces code size in programs that just want to use one or the other. For example, users of the monotonic clock don't need all that extra logic for subtracting or adding (secs+nsecs) pairs. And, in terms of static analyzability, having separate imports for real-time and monotonic clocks means we can statically know which clocks a program might care about.

And even in C programs, LLVM and/or binaryen could pattern-match certain library call idioms, so in the fullness of time, we should be able to pattern-match clock_gettime(CLOCK_MONOTONIC, ...), and translate it into a simple call to the dedicated monotonic-clock import. If we do that, it would get us lower per-call codesize than we could get with a dynamic clock index. Since it's per-call, this will save us much more than just saving a few bytes in one place in libc. So in the long run, this will be even better for code size, and have other advantages.

Wasmtime GitHub notifications bot (Jan 24 2022 at 18:19):

kripken commented on issue #3714:

I see, thanks @sunfishcode Very good points about code size in the long run.

For now we'll add the code to handle this for wasmtime in emscripten, and I guess the larger issues here will wait for the stabilization of WASI in the long term, as you mentioned. And I'll close this issue.

Wasmtime GitHub notifications bot (Jan 24 2022 at 18:19):

kripken closed issue #3714:

This happens when sending an invalid clock id to wasi_snapshot_preview1::clock_time_get or clock_res_get. In emscripten we have a test that sends 42, if the specific value matters, which is how we noticed this. (The test is checking for error handling.)

History:

In wasmtime 0.8.0 an error is returned.

In 0.18.0 a warning is printed out. (This extra logging makes the emscripten test fail, which is how we noticed this.)

In 0.33.0 the VM hits a fatal error:
Caused by:
    0: failed to invoke command default
    1: In func wasi_snapshot_preview1::clock_time_get at convert Clockid: Invalid enum value Clockid
       wasm backtrace:
           0:  0x917 - <unknown>!__clock_gettime
           1:  0x6cb - <unknown>!__original_main
           2:  0x8e5 - <unknown>!_start
I would assume that an invalid clock id should lead to an error and not extra logging or a fatal VM error, but reading the docs I can't actually find an answer to that... Apologies if this is not a bug!

Emscripten issue where this was noticed: https://github.com/emscripten-core/emscripten/pull/16076

Last updated: Nov 22 2024 at 16:03 UTC