Potential bug in name section generation/merging (when linking)

[osa1]

Some of the .wasm files' name sections can't be parsed by parity-wasm. Here's the test program:

fn main() {
    let file = std::env::args().nth(1).unwrap();
    test_parity_wasm(&file);
}

fn test_parity_wasm(file: &str) {
    let module = parity_wasm::deserialize_file(file).unwrap();
    match module.parse_names() {
        Err((err, _)) => {
            println!("Error while parsing names: {:?}", err);
        }
        Ok(_) => {}
    }
}

If I run this on .wasm files generated by moc, some of them are parsed fine while others fail:

$ cargo run /home/omer/dfinity/motoko_2/test/run-drun/_out/idl-any.ref.wasm
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/debug/parse_module /home/omer/dfinity/motoko_2/test/run-drun/_out/idl-any.ref.wasm`

$ cargo run /home/omer/dfinity/motoko_2/test/run-drun/_out/utf8.wasm
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/debug/parse_module /home/omer/dfinity/motoko_2/test/run-drun/_out/utf8.wasm`
Error while parsing names: [(11, Other("index is larger than expected"))]

It's also possible that this is a parity-wasm bug. It tried parsing names with wasmparser (the library wasmtime uses), but it's very difficult to use that library and I think it doesn't have as much error checking and it tries to return something useful even in the case of minor problems. In my testing I was unable to get any errors with wasmparser.

Not having names is making debugging difficult. I'm currently having a OOB heap access bug in a branch of mine and I can't get the backtrace because of this issue.

[osa1]

Interestingly, I have a dozen or so failing tests, and we can't parse name sections of any of them. There are other tests where we can parse names, but those pass ...

[osa1]

If I disable upper-bound checking in parity-wasm

diff --git a/src/elements/index_map.rs b/src/elements/index_map.rs
index 151f525..57ea642 100644
--- a/src/elements/index_map.rs
+++ b/src/elements/index_map.rs
@@ -161,9 +161,6 @@ impl<T> IndexMap<T> {
                let mut prev_idx = None;
                for _ in 0..len {
                        let idx: u32 = VarUint32::deserialize(rdr)?.into();
-                       if idx as usize >= max_entry_space {
-                               return Err(Error::Other("index is larger than expected"));
-                       }
                        match prev_idx {
                                Some(prev) if prev >= idx => {
                                        // Supposedly these names must be "sorted by index", so

then I can parse the .wasm files and get useful backtraces like

     Running `target/debug/wasmrun_ic0 /home/omer/dfinity/motoko/test/run-drun/_out/actor-class-cycles.wasm`
canister_self_copy(dest=0x4b604, offset=0x0, size=0xa), mem_addr=Some(MemAddr(0))
Error during execution: Trap(OOBMemoryAccess)
Backtrace:
  0: canister_init
  1: collect
  2: mark_compact
  3: motoko_rts::mark_compact::mark_fields::h5580d8bda5193d28
  4: motoko_rts::mark_compact::push_mark_stack::h515943b49a9e0fd5

Because the backtrace makes sense I think this could be a parity-wasm bug. If the backtrace was nonsensical then I'd expect this to be a moc bug.

[osa1]

The upper bound checking is there to avoid (according to comments) DoS attacks by passing a huge index to IndexMap. IndexMap allocates space proportional to the max index so it needs to be capped at a reasonable size. Currently parity-wasm allocates IndexMap with number of function for the function names, for example, which makes sense. I'm guessing there could be a bug in counting number of functions, or perhaps we generate redundant names or less names than we need to generate.

[nomeata]

I's also possible that this is a parity-wasm bug

I fixed that two weeks ago: https://github.com/paritytech/parity-wasm/pull/298

[osa1]

That's funny.. the bug you fixed is the exact same bug I fixed a while ago in Wasm instrumentation code (counting multiple locals of same type as single local).

Confirmed that your PR fixes this issue. Closing.