Stream: git-wasmtime

Topic: wasmtime / issue #4756 cranelift-icache fuzzbug: "interna...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2022 at 15:48):

cfallin opened issue #4756:

thread '<unnamed>' panicked at 'internal error: entered unreachable code: Invalid OperandSize: 16', [wasmtime/cranelift/codegen/src/isa/x64/inst/args.rs:1802](https://github.com/bytecodealliance/wasmtime/blob/d620705a323e3da59bd90473b4e627c8502b1255/cranelift/codegen/src/isa/x64/inst/args.rs#L1802):18

Discovered on oss-fuzz (https://oss-fuzz.com/testcase-detail/6676165556830208)

base64 -d to decode input:

I/0g////AAD/ARQE

cc @bnjbvr

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2022 at 16:18):

jameysharp commented on issue #4756:

According to cargo fuzz fmt cranelift-icache, this is the function being tested:

SingleFunction(
    function u0:1(i128 sext) system_v {
    block0(v0: i128):
        v1 = iconst.i128 0
        v2 = iconst.i64 0
        v3 = iconst.i32 0
        v4 = iconst.i16 0
        v5 = iconst.i8 0
        v6 = udiv v0, v0
        return
    }
    ,
)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2022 at 16:20):

jameysharp edited a comment on issue #4756:

According to cargo fuzz fmt cranelift-icache, this is the function being tested:

SingleFunction(
    function u0:1(i128 sext) system_v {
    block0(v0: i128):
        v1 = iconst.i128 0
        v2 = iconst.i64 0
        v3 = iconst.i32 0
        v4 = iconst.i16 0
        v5 = iconst.i8 0
        v6 = udiv v0, v0
        return
    }
    ,
)

So the x64 backend is just refusing to compile an i128 udiv.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 24 2022 at 07:12):

bnjbvr commented on issue #4756:

Yep, this is an internal panic (not even an error propagated up to callers), and I'm afraid that the icache fuzz target, as it directly generates CLIF code (as opposed to other fuzz targets which generate wasm code translated to CLIF) will keep on hitting those things that haven't been implemented because they weren't useful for wasm. (I guess that's the same situation cg_clif would be facing if it was being fuzzed.)

The overall solution I can think of is to make all those Result and propagate up errors, but that might add lots of error checking in many places in the code base, so I wonder if it's worth doing. Do other people have other ideas that come to mind?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 24 2022 at 07:31):

afonso360 commented on issue #4756:

I guess that's the same situation cg_clif would be facing if it was being fuzzed.

Yeah, I think so too.

Do other people have other ideas that come to mind?

What first came to mind was implementing the i128 div/rem ops, but another solution is to prevent the function generator from building i128 ops. We've disabled ops in the past where the x64 backend isn't ready to deal with them yet.

Although if we do disable those ops, we shouldn't commit that PR just yet, for the same reasons as https://github.com/bytecodealliance/wasmtime/pull/4752#issuecomment-1224326672 It will mark all issues as fixed in OSS Fuzz and then probably create dups in the future.

This would solve this immediate issue, but not the overall solution of not panicking during compilation (which I think is a great idea, but probably not for now!)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 24 2022 at 07:35):

afonso360 edited a comment on issue #4756:

I guess that's the same situation cg_clif would be facing if it was being fuzzed.

Yeah, I think so too.

Do other people have other ideas that come to mind?

What first came to mind was implementing the i128 div/rem ops, but another solution is to prevent the function generator from building i128 div/rem ops. We've disabled ops in the past where the x64 backend isn't ready to deal with them yet.

Although if we do disable those ops, we shouldn't commit that PR just yet, for the same reasons as https://github.com/bytecodealliance/wasmtime/pull/4752#issuecomment-1224326672 It will mark all issues as fixed in OSS Fuzz and then probably create dups in the future.

This would solve this immediate issue, but not the overall solution of not panicking during compilation (which I think is a great idea, but probably not for now!)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 24 2022 at 08:08):

afonso360 edited a comment on issue #4756:

I guess that's the same situation cg_clif would be facing if it was being fuzzed.

cg_clif specifically lowers these ops directly into a libcall, so it never hits this path. But this probably could have also been reported in the fuzzgen differential fuzzer since they share pretty much the same code paths.

Do other people have other ideas that come to mind?

What first came to mind was implementing the i128 div/rem ops, but another solution is to prevent the function generator from building i128 div/rem ops. We've disabled ops in the past where the x64 backend isn't ready to deal with them yet.

Although if we do disable those ops, we shouldn't commit that PR just yet, for the same reasons as https://github.com/bytecodealliance/wasmtime/pull/4752#issuecomment-1224326672 It will mark all issues as fixed in OSS Fuzz and then probably create dups in the future.

This would solve this immediate issue, but not the overall solution of not panicking during compilation (which I think is a great idea, but probably not for now!)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 24 2022 at 20:42):

cfallin commented on issue #4756:

Yes, the ideal answer here is that the verifier is the one gate that determines/defines what is valid CLIF; and if valid, then a correct backend can compile it. Anything less than that is in theory a temporary situation, a place where we either want to eventually fill out the backend or reduce the scope of valid CLIF at the verifier level (by changing the definitions in the meta crate).

I don't necessarily think introducing Results all the way up is the right answer here if the semantics we eventually want are that the verifier is the single source of acceptability; but what we can and should do probably soon is to try to close the gap between the verifier and backends' ideas of "acceptable".

If we don't expect to actually use i128.div or i128.rem anywhere, I'm ok with deleting this case by modifying the instruction definitions accordingly. It feels a little non-orthogonal, but then, maintaining actually-unused code is not a great feeling (or good use of time) either.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 24 2022 at 21:53):

jameysharp commented on issue #4756:

Based on the discussion in #4771, I think it's better to keep the 128-bit versions of division operators: cg_clif needs them and is currently working around their absence by generating function calls to compiler-builtins implementations. I like Afonso's suggestion of having Cranelift implement these instructions as libcalls, although it sounds like there are some potential issues there.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2022 at 21:29):

jameysharp closed issue #4756:

thread '<unnamed>' panicked at 'internal error: entered unreachable code: Invalid OperandSize: 16', [wasmtime/cranelift/codegen/src/isa/x64/inst/args.rs:1802](https://github.com/bytecodealliance/wasmtime/blob/d620705a323e3da59bd90473b4e627c8502b1255/cranelift/codegen/src/isa/x64/inst/args.rs#L1802):18

Discovered on oss-fuzz (https://oss-fuzz.com/testcase-detail/6676165556830208)

base64 -d to decode input:

I/0g////AAD/ARQE

cc @bnjbvr

Last updated: Nov 22 2024 at 17:03 UTC