wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

cranelift: ty_smin/smax/umax/mask panic on i128

Open jameysharp opened this issue 3 months ago • 3 comments

OSS-Fuzz reported this as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69209. I've minimized the test case and identified several related panics, all with the message "unimplemented for > 64 bits".

This is not a security issue according to Wasmtime's security policy because it's a panic at compile time. It also shouldn't be reachable from Wasmtime today, only other Cranelift frontends.

Minimized CLIF
test compile
set opt_level=speed
target x86_64

function %slt() system_v {
block0:
    v0 = iconst.i64 0
    v1 = uextend.i128 v0
    v2 = icmp slt v1, v1
    return
}

function %ult() system_v { 
block0:
    v0 = iconst.i64 0 
    v1 = uextend.i128 v0
    v2 = icmp ult v1, v1
    return
}

The original fuzzbug, and my %slt function above, hit this panic in ty_smin: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/isle_prelude.rs#L287-L289

I believe the optimization rule which invoked ty_smin is here: https://github.com/bytecodealliance/wasmtime/blob/f40aaa51a1df835498985acfc91ade33240d02b5/cranelift/codegen/src/opts/icmp.isle#L105-L108

There are similar rules for ult exercised by my %ult function above. They call ty_umax, which calls ty_mask, which has essentially the same panic.

There are also similar rules for slt which would hit an equivalent panic in ty_smax, but I can't exercise them because ISLE is choosing to call ty_smin first which then panics.

I believe these panics are only being exposed now because #8686 allowed these rules to match i128 constants when they couldn't before. The rules should still fail to match because of the fits_in_64 constraint on the other type in the pattern. However, ISLE does not guarantee anything about what order constructors and extractors will be evaluated in except for data dependencies. So that fits_in_64 guard isn't sufficient to ensure that ty_smin won't be called.

One fix that I've verified works for the %slt test case:

  • add the partial keyword to (decl pure ty_smin ...) and similarly for ty_smax,
  • make the corresponding Rust functions return Option,
  • use ? instead of .expect() when .checked_sub() fails.

However I ran out of energy when I looked at doing the same for ty_umax, which requires doing it for ty_mask, which is called from Rust in a lot of places.

cc: @scottmcm

jameysharp avatar May 24 '24 23:05 jameysharp