Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue#2445 Null Pointer Deref #2461

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix issue#2445 Null Pointer Deref #2461

wants to merge 3 commits into from

Conversation

haruki3hhh
Copy link

Fix issue 2445:
#2445

sbc100
sbc100 reviewed Sep 11, 2024
View reviewed changes
src/wast-parser.cc
@@ -3439,7 +3439,11 @@ Result WastParser::ParseModuleCommand(Script* script, CommandPtr* out_command) {
error.loc.offset, error.message.c_str());
}
}
*module = std::move(*m.get());
if (m) {
*module = std::move(*m.get());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Would this work instead:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@haruki3hhh
Copy link
Author

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
Copy link
Member

sbc100 commented Sep 11, 2024

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Contributor

SoniEx2 commented Sep 15, 2024 •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbc100 sbc100 sbc100 left review comments

@SoniEx2 SoniEx2 SoniEx2 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haruki3hhh @sbc100 @SoniEx2