[panic] assert failed in cranelift_entity (only using compile fuzzer)

[pventuzelo]

Issue description

During fuzzing of lightbeam, i found this crash that seems to be related to cranelift.

$ cargo +nightly fuzz run compile assert_failed_cranelift_entity.wasm
Running: assert_failed_cranelift_entity.wasm
thread '<unnamed>' panicked at 'assertion failed: x < ::cranelift_entity::__core::u32::MAX', /XXX/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/cranelift-wasm-0.50.0/src/translation_utils.rs:18:1

Note: crash will not happen if you are calling wasmtime binary but only using compile API.
Also, the crash happen using both fuzzing strategy (i.e. cranelift or ligthbeam)

Reproduction

Download assert_failed_cranelift_entity.zip

Run the fuzzer:

$ cargo +nightly fuzz run compile assert_failed_cranelift_entity.wasm
Running: assert_failed_cranelift_entity.wasm
thread '<unnamed>' panicked at 'assertion failed: x < ::cranelift_entity::__core::u32::MAX', /XXX/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/cranelift-wasm-0.50.0/src/translation_utils.rs:18:1

wasmtime commit: 086ff63

Crash details

Running: assert_failed_cranelift_entity.wasm
thread '<unnamed>' panicked at 'assertion failed: x < ::cranelift_entity::__core::u32::MAX', /XXX/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/cranelift-wasm-0.50.0/src/translation_utils.rs:18:1
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:61
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1030
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:65
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:50
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:188
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:205
  10: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
             at /rustc/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/liballoc/boxed.rs:956
  11: libfuzzer_sys::initialize::{{closure}}
             at /XXX/.cargo/git/checkouts/libfuzzer-sys-e07fde05820d7bc6/0c45075/src/lib.rs:50
  12: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:468
  13: std::panicking::begin_panic
             at /rustc/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/libstd/panicking.rs:400
  14: cranelift_wasm::translation_utils::FuncIndex::from_u32
             at /XXX/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/cranelift-wasm-0.50.0/<::cranelift_entity::entity_impl macros>:20
  15: cranelift_wasm::sections_translator::parse_function_name_subsection
             at /XXX/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/cranelift-wasm-0.50.0/src/sections_translator.rs:419
  16: core::ops::function::FnOnce::call_once
             at /rustc/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/libcore/ops/function.rs:227
  17: core::option::Option<T>::and_then
             at /rustc/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/libcore/option.rs:657
  18: cranelift_wasm::sections_translator::parse_name_section
             at /XXX/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/cranelift-wasm-0.50.0/src/sections_translator.rs:395
  19: cranelift_wasm::module_translator::translate_module
             at /XXX/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/cranelift-wasm-0.50.0/src/module_translator.rs:83
  20: wasmtime_environ::module_environ::ModuleEnvironment::translate
             at crates/environ/src/module_environ.rs:83
  21: wasmtime_jit::instantiate::RawCompiledModule::new
             at crates/jit/src/instantiate.rs:68
  22: wasmtime_jit::instantiate::CompiledModule::new
             at crates/jit/src/instantiate.rs:159
  23: wasmtime_fuzzing::oracles::compile
             at crates/fuzzing/src/oracles.rs:84
  24: rust_fuzzer_test_input
             at fuzz/fuzz_targets/compile.rs:8
  25: libfuzzer_sys::test_input_wrap::{{closure}}
             at /XXX/.cargo/git/checkouts/libfuzzer-sys-e07fde05820d7bc6/0c45075/src/lib.rs:27
  26: std::panicking::try::do_call
             at /rustc/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/libstd/panicking.rs:287
  27: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:79
  28: std::panicking::try
             at /rustc/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/libstd/panicking.rs:265
  29: std::panic::catch_unwind
             at /rustc/4f03f4a989d1c8346c19dfb417a77c09b34408b8/src/libstd/panic.rs:396
  30: LLVMFuzzerTestOneInput
             at /XXX/.cargo/git/checkouts/libfuzzer-sys-e07fde05820d7bc6/0c45075/src/lib.rs:25
  31: _ZN6fuzzer6Fuzzer15ExecuteCallbackEPKhm
             at libfuzzer/FuzzerLoop.cpp:553
  32: _ZN6fuzzer10RunOneTestEPNS_6FuzzerEPKcm
             at libfuzzer/FuzzerDriver.cpp:292
  33: _ZN6fuzzer12FuzzerDriverEPiPPPcPFiPKhmE
  34: main
             at libfuzzer/FuzzerMain.cpp:19
  35: __libc_start_main
  36: _start

[abrown]

I looked at the wasm file posted in zip:

$ wasm-objdump -s assert_failed_cranelift_entity.wasm

assert_failed_cranelift_entity.wasm:	file format wasm 0x1
000010f: warning: invalid linking metadata version: 3
0000153: warning: invalid function index: 4294967295
...

That function index is equal to u32::MAX so perhaps we could change the failing assertion to less-than-or-equals. That might get rid of this failure, but another option would be to throw an error at the validation level since it seems that the tooling already thinks this function index is warning-worthy.

[fitzgen]

u32::MAX is technically valid and within range of the index space, even if we won't ever have that many functions in practice. Since we want to use a single u32 for the entities and reserve u32::MAX for ourselves, and therefore our implementation only supports the index space up to 2^32 - 2. Since this is an implementation-specific limit, and not a spec limit, I don't think it makes sense to enforce this in the parser's validator (unless we give it the ability to have custom impl-specific limits). Instead, I think cranelift-wasm should check this during translation.

[fitzgen]

Actually maybe I'm wrong here, if wasm-objdump also thinks this is an invalid index. Let me re-read the spec more carefully...

[fitzgen]

Hmmm... the following seems to suggest that u32::MAX aka 2^32 - 1 should be a valid index:

https://webassembly.github.io/spec/core/syntax/modules.html#syntax-index

funcidx ::= u32

https://webassembly.github.io/spec/core/syntax/values.html#syntax-int

uN ::= 0 | 1 | ... | 2^N - 1

---

I suppose the real issue is that wherever we are constructing the entity, we aren't first checking that the index is within the number of functions that this module actually has (and also somewhere ensuring taht we don't allow u32::MAX numbers of functions in a module as an impl limit).

[fitzgen]

reduced test case:
min-issue-1306.zip