Name section parsing fails for valid wasm

[jwasinger]

This works with 0.35.0 and panics on 0.38.0:

        let wasm: Vec<u8> = FromHex::from_hex("0061736d0100000001080260017e0060000002170103656e760f657468657265756d5f757365476173000003030201010503010001071102046d61696e0001066d656d6f727902000801020a070202000b02000b0020046e616d65010e0201046d61696e02056d61696e320209030001000001000200").unwrap();

        let mut module: Module = parity_wasm::deserialize_buffer::<Module>(&wasm).unwrap();
        module = module.parse_names().unwrap(); // panic here

Error snippet:

panicked at 'called `Result::unwrap()` on an `Err` value: ([(7, Other("index is larger than expected"))], Module { magic: 1836278016, version: 1, sections: [Type(TypeSe
ction([Function(FunctionType { form: 96, params: [I64], return_type: None }), Function(FunctionType { form: 96, params: [], return_type: None })])), Import(ImportSection([ImportEntry { module_str: "env", field_str
: "ethereum_useGas", external: Function(0) }])), Function(FunctionSection([Func(1), Func(1)])), Memory(MemorySection([MemoryType(ResizableLimits { initial: 1, maximum: None, shared: false })])), Export(ExportSecti
on([ExportEntry { field_str: "main", internal: Function(1) }, ExportEntry { field_str: "memory", internal: Memory(0) }])), Start(2), Code(CodeSection([FuncBody { locals: [], instructions: Instructions([End]) }, Fu
ncBody { locals: [], instructions: Instructions([End]) }])), Custom(CustomSection { name: "name", payload: [1, 14, 2, 1, 4, 109, 97, 105, 110, 2, 5, 109, 97, 105, 110, 50, 2, 9, 3, 0, 1, 0, 0, 1, 0, 2, 0] })] })',
 src/libcore/result.rs:997:5

I'm using Ubuntu 18.0.4.

[pepyakin]

I think it could be related to #270 . cc @michaelvoronov / @NikVolf

[mikevoronov]

I have dug a little bit into this example and found that the name section looks like this:

00 - name section type
20 - the size of overall name section
04 - size of the following string
6e616d65 - "name", the id of a name section

01 - type of name subsection (1 refers to the function name subsection type)
0e - size of the name subsection (function)
02 - count of names in the functions section
01 - id of a string
04 - size of the following string
6d61696e - "name", the string that should have id 1
02 - id of a string
05 - size of the following string
6d61696e32 - "name2", the string that should have id 2

02 - type of name subsection (2 refers to the locals name subsection type)
09 - size of the name subsection (locals names)

all next fields are in terms of the IndexMap::deserialize_with function
03 - len

00 - idx
value:
01 - len
00 - idx
00 - val

01 - idx
value:
00 - len

02 - idx <====== panics here (there are only two functions in this module)
value
00 - len

So, smth might be wrong with locals names subsection.

How was this code obtained?

P.S. It works on 0.35.0 because it doesn't parse this locals names subsection :).

[jwasinger]

This was the original text:

(module
    (import "env" "ethereum_useGas" (func (param i64)))
    (memory 1)
    (export "main" (func $main))
    (export "memory" (memory 0))
    (func $main2)
    (func $main)
    (start $main2)
)

And translated to wasm (with name section) using wabt's wat2wasm tool: wat2wasm --debug-names text.wat

[mikevoronov]

@NikVolf It seems that the problem here is that the LocalNameSubsection::deserialize doesn't count the imported functions in max_entry_space. To compare in wabt code which parses the same subsection compares a function id with NumTotalFuncs() result, and in its turn the NumTotalFuncs implemented here takes into account imports as well.

P.S. Shall I make a PR to fix it? Also, it seems reasonable to look through the code to find the similar cases with imports.

[axic]

Was this fixed by #277? If so, can the above example included in the tests?

[mikevoronov]

Actually, this snippet is the same as @benediktwerner provided. Because it depends only on the presence of imports.

Btw, It was really fixed and can be closed.

[nomeata]

Hmm, I still observe Other("index is larger than expected") (with latest git)… will dig into it

[nomeata]

--- a/src/elements/name_section.rs
+++ b/src/elements/name_section.rs
@@ -268,7 +268,7 @@ impl LocalNameSubsection {
 
                let max_space = max_signature_args + max_locals;
 
-               let deserialize_locals = |_: u32, rdr: &mut R| IndexMap::deserialize(max_space, rdr);
+               let deserialize_locals = |_: u32, rdr: &mut R| IndexMap::deserialize(usize::MAX, rdr);
 
                let local_names = IndexMap::deserialize_with(
                        max_entry_space,

avoids it, so either my wasm has wrong local names, or something is wrong with max_space (although it looks reasonable)

[nomeata]

I think I found it: You are counting Locals objects, but these can have a count > 1. This seems to work better:

diff --git a/src/elements/name_section.rs b/src/elements/name_section.rs
index 7001085..a385f4c 100644
--- a/src/elements/name_section.rs
+++ b/src/elements/name_section.rs
@@ -263,7 +263,7 @@ impl LocalNameSubsection {
 
                let max_locals = module
                        .code_section()
-                       .map(|cs| cs.bodies().iter().map(|f| f.locals().len()).max().unwrap_or(0))
+                       .map(|cs| cs.bodies().iter().map(|f| f.locals().iter().map(|l| l.count() as usize).max().unwrap_or(0)).max().unwrap_or(0))
                        .unwrap_or(0);
 
                let max_space = max_signature_args + max_locals;

[mangas]

Has this fix been released? I've had the same issue on v0.42.2 (latest tagged)

[hunjixin]

got the same issue, the wasm was generate from golang. i have try master branch,still have problem.

[hunjixin]

maybe better keep use Unparsed part, after all, this bug has existed for two years. If all think it OK, I can have a pr, @nukemandan

[mangas]

I've open a PR that I think solves this issue, at least what I'm able to replicate. The formatting is a bit messy, if that's important I can try to fix it. Could I get a review? @nukemandan not sure if you're able to review, if not, could you please point me in the right direction?

[mangas]

@pepyakin could you please tag a new version release with this change?