Skip to content

[EH] Add validation for new instructions #6185

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

Merged
aheejin merged 4 commits into from
Dec 20, 2023
Merged

[EH] Add validation for new instructions #6185

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d1feab8
[EH] Add validation for new instructions
aheejin Dec 14, 2023
9b67a70
Update src/wasm/wasm-validator.cpp
aheejin Dec 20, 2023
0bfa896
Address comments
aheejin Dec 20, 2023
f9c493e
Update wasm-validator.cpp
aheejin Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 33 additions & 36 deletions src/wasm/wasm-validator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2455,11 +2455,6 @@ void FunctionValidator::visitTryTable(TryTable* curr) {
curr->type,
curr->body,
"try_table's type does not match try_table body's type");
} else {
shouldBeEqual(curr->body->type,
Type(Type::unreachable),
curr,
"unreachable try_table must have unreachable try_table body");
}

shouldBeEqual(curr->catchTags.size(),
Expand All @@ -2475,45 +2470,47 @@ void FunctionValidator::visitTryTable(TryTable* curr) {
curr,
"the number of catch tags and sent types do not match");

const char* invalidSentTypeMsg = "invalid catch sent type information";
Type exnref = Type(HeapType::exn, Nullable);
for (Index i = 0; i < curr->catchTags.size(); i++) {
Name tagName = curr->catchTags[i];
if (!tagName) {
continue;
}
auto sentType = curr->sentTypes[i];
size_t tagTypeSize;

// Check tag validity
auto* tag = getModule()->getTagOrNull(tagName);
if (!shouldBeTrue(tag != nullptr, curr, "")) {
getStream() << "catch's tag name is invalid: " << tagName << "\n";
} else if (!shouldBeEqual(tag->sig.results, Type(Type::none), curr, "")) {
getStream()
<< "catch's tag (" << tagName
<< ") has result values, which is not allowed for exception handling";
}
Name tagName = curr->catchTags[i];
if (!tagName) { // catch_all or catch_all_ref
tagTypeSize = 0;

Copy link
Member

@tlively tlively Dec 20, 2023

Choose a reason for hiding this comment

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

nit: I think this extra newline looks a little odd.

Copy link
Member Author

@aheejin aheejin Dec 20, 2023

Choose a reason for hiding this comment

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

Removed it

} else { // catch or catch_ref
// Check tag validity
auto* tag = getModule()->getTagOrNull(tagName);
if (!shouldBeTrue(tag != nullptr, curr, "")) {
getStream() << "catch's tag name is invalid: " << tagName << "\n";
} else if (!shouldBeEqual(tag->sig.results, Type(Type::none), curr, "")) {
getStream()
<< "catch's tag (" << tagName
<< ") has result values, which is not allowed for exception handling";
}

// Check types in sentTypes is valid
auto tagType = tag->sig.params;
auto sentType = curr->sentTypes[i];
if (tagType.size() == 0 && !curr->catchRefs[i]) {
if (shouldBeTrue(sentType.size() == 1, curr, "")) {
shouldBeEqual(sentType,
Type(Type::none),
curr,
"invalid catch sent type information");
// tagType and sentType should be the same (except for the possible exnref
// at the end of sentType)
auto tagType = tag->sig.params;
tagTypeSize = tagType.size();
for (Index j = 0; j < tagType.size(); j++) {
shouldBeEqual(tagType[j], sentType[j], curr, invalidSentTypeMsg);
}
}
for (Index j = 0; j < tagType.size(); j++) {
shouldBeEqual(
tagType[j], sentType[j], curr, "invalid catch sent type information");
}

// If this is catch_ref or catch_all_ref, sentType.size() should be
// tagType.size() + 1 because there is an exrnef tacked at the end. If
// this is catch/catch_all, the two sizes should be the same.
if (curr->catchRefs[i]) {
Copy link
Member

@tlively tlively Dec 19, 2023

Choose a reason for hiding this comment

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

It would be good to check that the sizes match when this is false as well. Alternatively, when there is no extra exnref, you can just check tagType == sentType.

Copy link
Member Author

@aheejin aheejin Dec 20, 2023

Choose a reason for hiding this comment

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

This was actually incorrect because even if there is no values to send, sentType will contain Type::none, which is size 1... And this wasn't crashing because I was doing the early exit (https://github.com/WebAssembly/binaryen/pull/6185/files#diff-2149be365b66a82038f1d3fa8f9fb1b4dcd40ab535c986f4bf0a4c37e669a1ceR2481-R2483). Will fix that and check both cases.

Copy link
Member Author

@aheejin aheejin Dec 20, 2023

Choose a reason for hiding this comment

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

Nevermind the comment. Didn't know (Type::none).size() returns 0...

Copy link
Member Author

@aheejin aheejin Dec 20, 2023

Choose a reason for hiding this comment

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

Ended up rewriting much of the loop, but I think I an now checking both cases. PTAL

if (shouldBeTrue(sentType.size() == tagType.size() + 1, curr, "")) {
shouldBeEqual(sentType[sentType.size() - 1],
exnref,
curr,
"invalid catch sent type information");
if (shouldBeTrue(
sentType.size() == tagTypeSize + 1, curr, invalidSentTypeMsg)) {
shouldBeEqual(
sentType[sentType.size() - 1], exnref, curr, invalidSentTypeMsg);
}
} else {
shouldBeTrue(sentType.size() == tagTypeSize, curr, invalidSentTypeMsg);
}

// Note catch destinations with sent types
Expand Down