Fix issue#2445 Null Pointer Deref

[haruki3hhh]

Fix issue 2445:
#2445

[sbc100]

Is it possible for ParseWatModule above to succeed without *m being set?

Would this work instead:

if (errors.length()) { 
  return Result::Error;
}
assert(*m);

[SoniEx2]

doesn't ParseWatModule also return a Result? probably a good idea to check that instead.

[sbc100]

Yes that sounds good. You can do something like this

if (Failed(ParseWatModule(lexer.get(), &m, &errors, options_)) { 
   for (const auto& error : errors) {
      ...
   }
   return Result::Error;
}
assert(errors.length == 0);

[haruki3hhh]

Hi,
After adding this patch, execute the poc, it shows:

# ./wat2wasm ../@POC@
../@POC@:1:2: error: error in quoted module: @0x100000001: unexpected token "|", expected a module field or a module.
(module quote "\7c")
 ^^^^^^

And ASAN is not triggered.

Do you think we should make some further improvements about it?
I'm glad to do it, if needed.

[sbc100]

It would be good to add that as a test case I think. Seems like the kind of test that could also be upstreamed to the wasm spec test suite too (once it lands here).

[haruki3hhh]

If I want to add it into test case, should I directly add a .txt file to https://github.com/WebAssembly/wabt/tree/main/test/parse?

[haruki3hhh]

@sbc100 @SoniEx2 Hi folks, I added a testcase, could you also please take a look at it?

https://github.com/WebAssembly/wabt/pull/2461/files#diff-198b0c14be90b6dbd98ff0d8d3289a686f4daa9733c748db3dadfaa5c55635fd

[SoniEx2]

the test looks great. wonder if it would make sense to upstream it...

[haruki3hhh]

Hi folks, @SoniEx2 @sbc100

Do you think I need to do some further improvements for this patch? Or we can merge it?

I'd like to help, if further improvements are needed!

[SoniEx2]

ParseWatModule returns a Result, it would be a good idea to check that Result.