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

[jameysharp]

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:

wasmtime/cranelift/codegen/src/isle_prelude.rs

Lines 287 to 289 in f40aaa5

	let shift = 64_u64
	.checked_sub(ty_bits.into())
	.expect("unimplemented for > 64 bits");

I believe the optimization rule which invoked ty_smin is here:

wasmtime/cranelift/codegen/src/opts/icmp.isle

Lines 105 to 108 in f40aaa5

	;; slt(x, SMIN) == false.
	(rule (simplify (slt (fits_in_64 (ty_int bty)) x smin @ (iconst_u cty y)))
	(if-let $true (u64_eq y (ty_smin cty)))
	(subsume (iconst_u bty 0)))

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

[alexcrichton]

Would it perhaps make sense to revert #8686 while a "true fix" is found in the meantime?

[scottmcm]

From my side, feel free to revert my PR for now. I likely won't be able to look at fixing this in the short term.

[alexcrichton]

I've posted #8707 to revert the original PR and included this issue's test cases as well so a re-landing will be sure to trigger the new test case too.