-
Notifications
You must be signed in to change notification settings - Fork 758
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.
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
[EH] Fuzz catch+rethrow on the JS side #7287
Changes from 72 commits
2a07050
53d123f
93b68ae
acbee3e
10323cf
c9df9c0
219e885
f93e130
7be44d8
15d90a4
d3b5402
a333277
bf6b02c
3215f00
fe1dc4a
d1e84c8
982dd16
8bd72b1
663194e
060d3ac
a4da2c4
fd3ae28
24f45bb
2ab135e
0e1b1ec
d191f3d
9dfbeab
470f267
cb053d5
9718b5f
424cd2e
d63bfad
796108d
454fc8f
a551bd3
77d4422
921e441
1907e8a
9f4c2b4
282ee57
260ba72
317204f
de6d3a9
86cb4ad
834a150
c06dc07
0362ef8
9084879
5c8a082
be665df
8dfbc5b
ff6b555
cf41934
70ae546
1926f78
4444485
b97216a
a4216b0
89e3ae4
ece9449
2440f6a
cbbcb59
797a6b5
ad988d7
4267acd
1730e96
60a8747
c325697
5533771
fcaa713
1f680a2
c83115a
340f456
fcaf2fd
bf621f5
0126238
2015983
e8312bf
f9fd6c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -170,10 +170,10 @@ function callFunc(func) { | |||||||||
return func.apply(null, args); | ||||||||||
} | ||||||||||
|
||||||||||
// Calls a given function in a try-catch, swallowing JS exceptions, and return 1 | ||||||||||
// if we did in fact swallow an exception. Wasm traps are not swallowed (see | ||||||||||
// details below). | ||||||||||
/* async */ function tryCall(func) { | ||||||||||
// Calls a given function in a try-catch. Return 1 if an exception was thrown. | ||||||||||
// If |rethrow| is set, and an exception is thrown, it is caught and rethrown. | ||||||||||
// Wasm traps are not swallowed (see details below). | ||||||||||
/* async */ function tryCall(func, rethrow) { | ||||||||||
try { | ||||||||||
/* await */ func(); | ||||||||||
return 0; | ||||||||||
|
@@ -208,7 +208,11 @@ function callFunc(func) { | |||||||||
} | ||||||||||
} | ||||||||||
// Otherwise, this is a normal exception we want to catch (a wasm | ||||||||||
// exception, or a conversion error on the wasm/JS boundary, etc.). | ||||||||||
// exception, or a conversion error on the wasm/JS boundary, etc.). Rethrow | ||||||||||
// if we were asked to. | ||||||||||
if (rethrow) { | ||||||||||
throw e; | ||||||||||
} | ||||||||||
return 1; | ||||||||||
} | ||||||||||
} | ||||||||||
|
@@ -271,7 +275,7 @@ var imports = { | |||||||||
'throw': (which) => { | ||||||||||
if (!which) { | ||||||||||
// Throw a JS exception. | ||||||||||
throw 0; | ||||||||||
throw new Error('js exception'); | ||||||||||
} else { | ||||||||||
// Throw a wasm exception. | ||||||||||
throw new WebAssembly.Exception(wasmTag, [which]); | ||||||||||
|
@@ -287,19 +291,39 @@ var imports = { | |||||||||
}, | ||||||||||
|
||||||||||
// Export operations. | ||||||||||
'call-export': /* async */ (index) => { | ||||||||||
/* await */ callFunc(exportList[index].value); | ||||||||||
'call-export': /* async */ (index, flags) => { | ||||||||||
var rethrow = flags & 1; | ||||||||||
if (JSPI) { | ||||||||||
// TODO: Figure out why JSPI fails here. | ||||||||||
rethrow = 0; | ||||||||||
} | ||||||||||
if (!rethrow) { | ||||||||||
/* await */ callFunc(exportList[index].value); | ||||||||||
} else { | ||||||||||
tryCall(/* async */ () => /* await */ callFunc(exportList[index].value), | ||||||||||
rethrow); | ||||||||||
} | ||||||||||
Comment on lines
+304
to
+309
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are these two modes different? What happens we call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first arm of the if calls the function normally. If it throws, the exception keeps propagating out normally. The second arm does the call inside a JS try-catch. If the call throws, we catch and rethrow it from JS. VMs may implement those paths very different, I am told. This is just aiming to fuzz more things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Then why do we pass a function directly to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the second case we want (Another option would be to write the try-catch here, that is, inline And, because of how async-await works in JavaScript, every function on the call stack must be declared async and called with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I'm kind of confused why somewhere we print the contents and somewhere we don't, and when it matters and when it doesn't at this point 😵💫 But anyway it may not matter much. Also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general we don't print exceptions, because the error text (and stack trace) may vary between VMs. Everything else, including the externref inside an exception - which would be a JS value - we do print, if we extracted it from the exception. I might be forgetting something though. The comments are documented earlier in the file: binaryen/scripts/fuzz_shell.js Lines 10 to 13 in c83115a
We need async/await in JSPI mode, but don't want them to change anything in normal mode. So we have them in comments, and un-comment them in JSPI mode. |
||||||||||
}, | ||||||||||
'call-export-catch': /* async */ (index) => { | ||||||||||
return tryCall(/* async */ () => /* await */ callFunc(exportList[index].value)); | ||||||||||
}, | ||||||||||
|
||||||||||
// Funcref operations. | ||||||||||
'call-ref': /* async */ (ref) => { | ||||||||||
'call-ref': /* async */ (ref, flags) => { | ||||||||||
// This is a direct function reference, and just like an export, it must | ||||||||||
// be wrapped for JSPI. | ||||||||||
ref = wrapExportForJSPI(ref); | ||||||||||
/* await */ callFunc(ref); | ||||||||||
var rethrow = flags & 1; | ||||||||||
if (JSPI) { | ||||||||||
// TODO: Figure out why JSPI fails here. | ||||||||||
rethrow = 0; | ||||||||||
} | ||||||||||
if (!rethrow) { | ||||||||||
/* await */ callFunc(ref); | ||||||||||
} else { | ||||||||||
tryCall(/* async */ () => /* await */ callFunc(ref), | ||||||||||
rethrow); | ||||||||||
} | ||||||||||
}, | ||||||||||
'call-ref-catch': /* async */ (ref) => { | ||||||||||
ref = wrapExportForJSPI(ref); | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -132,6 +132,10 @@ struct LoggingExternalInterface : public ShellExternalInterface { | |
return {}; | ||
} else if (import->base == "call-export") { | ||
callExportAsJS(arguments[0].geti32()); | ||
// The second argument determines if we should catch and rethrow | ||
// exceptions. There is no observable difference in those two modes in | ||
// the binaryen interpreter, so we don't need to do anything. | ||
Comment on lines
+135
to
+137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what this means. Does that mean we only rethrow an exception (in case it occurs) in JS, so we don't have any difference in Binaryen? If so, why do we rethrow only in JS? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We rethrow in JS because we hope to find VM bugs. They might create a new JS exception around the wasm one, or something like that, but I don't know the full details - this is a suggestion the VM people gave me. We could catch and rethrow in Binaryen too I guess, but that would literally be just try-catch and rethrow (we have no stack traces or other info in exceptions). |
||
|
||
// Return nothing. If we wanted to return a value we'd need to have | ||
// multiple such functions, one for each signature. | ||
return {}; | ||
|
@@ -143,9 +147,8 @@ struct LoggingExternalInterface : public ShellExternalInterface { | |
return {Literal(int32_t(1))}; | ||
} | ||
} else if (import->base == "call-ref") { | ||
// Similar to call-export*, but with a ref. | ||
callRefAsJS(arguments[0]); | ||
// Return nothing. If we wanted to return a value we'd need to have | ||
// multiple such functions, one for each signature. | ||
return {}; | ||
} else if (import->base == "call-ref-catch") { | ||
try { | ||
|
@@ -181,11 +184,13 @@ struct LoggingExternalInterface : public ShellExternalInterface { | |
} | ||
|
||
void throwJSException() { | ||
// JS exceptions contain an externref, which wasm can't read (so the actual | ||
// value here does not matter, but it does need to match what the 'throw' | ||
// import does in fuzz_shell.js, as the fuzzer will do comparisons). | ||
Literal externref = Literal::makeI31(0, Unshared).externalize(); | ||
Literals arguments = {externref}; | ||
// JS exceptions contain an externref. Use the same type of value as a JS | ||
// exception would have, which is a reference to an object, and which will | ||
// print out "object" in the logging from JS. A trivial struct is enough for | ||
// us to log the same thing here. | ||
auto empty = HeapType(Struct{}); | ||
auto inner = Literal(std::make_shared<GCData>(empty, Literals{}), empty); | ||
Literals arguments = {inner.externalize()}; | ||
auto payload = std::make_shared<ExnData>(jsTag, arguments); | ||
throwException(WasmException{Literal(payload)}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -851,12 +851,16 @@ void TranslateToFuzzReader::addImportCallingSupport() { | |
|
||
if (choice & 1) { | ||
// Given an export index, call it from JS. | ||
// A second parameter has flags. The first bit determines whether we catch | ||
// and rethrow all exceptions. (This ends up giving us the same signature | ||
// and behavior as the normal mode, so we just add the flags here rather | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's "normal mode"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "normal" is when the bit is not set. I'll clarify the comment. |
||
// than another export.) | ||
callExportImportName = Names::getValidFunctionName(wasm, "call-export"); | ||
auto func = std::make_unique<Function>(); | ||
func->name = callExportImportName; | ||
func->module = "fuzzing-support"; | ||
func->base = "call-export"; | ||
func->type = Signature({Type::i32}, Type::none); | ||
func->type = Signature({Type::i32, Type::i32}, Type::none); | ||
wasm.addFunction(std::move(func)); | ||
} | ||
|
||
|
@@ -884,7 +888,10 @@ void TranslateToFuzzReader::addImportCallingSupport() { | |
func->name = callRefImportName; | ||
func->module = "fuzzing-support"; | ||
func->base = "call-ref"; | ||
func->type = Signature({Type(HeapType::func, Nullable)}, Type::none); | ||
// As call-export, there is a flags param that allows us to catch+rethrow | ||
// all exceptions. | ||
func->type = | ||
Signature({Type(HeapType::func, Nullable), Type::i32}, Type::none); | ||
wasm.addFunction(std::move(func)); | ||
} | ||
|
||
|
@@ -1135,7 +1142,12 @@ Expression* TranslateToFuzzReader::makeImportCallCode(Type type) { | |
if ((catching && (!exportTarget || oneIn(2))) || (!catching && oneIn(4))) { | ||
// Most of the time make a non-nullable funcref, to avoid errors. | ||
auto refType = Type(HeapType::func, oneIn(10) ? Nullable : NonNullable); | ||
return builder.makeCall(refTarget, {make(refType)}, type); | ||
std::vector<Expression*> args = {make(refType)}; | ||
if (!catching) { | ||
// The first bit matters here, so we can send anything. | ||
args.push_back(make(Type::i32)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this mean? Shouldn't this be either 0 or 1 depending on whether to rthrow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is future-proof, as random values in the other bits might be used later, and have no downside now. I'll clarify in a comment. |
||
return builder.makeCall(refTarget, args, type); | ||
} | ||
} | ||
|
||
|
@@ -1163,7 +1175,15 @@ Expression* TranslateToFuzzReader::makeImportCallCode(Type type) { | |
index = builder.makeBinary( | ||
RemUInt32, index, builder.makeConst(int32_t(maxIndex))); | ||
} | ||
return builder.makeCall(exportTarget, {index}, type); | ||
|
||
// The non-catching variants send a flags argument, which says whether to | ||
// catch+rethrow. | ||
std::vector<Expression*> args = {index}; | ||
if (!catching) { | ||
// The first bit matters here, so we can send anything. | ||
args.push_back(make(Type::i32)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (also updated this comment) |
||
return builder.makeCall(exportTarget, args, type); | ||
} | ||
|
||
Expression* TranslateToFuzzReader::makeImportSleep(Type type) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this effectively reverting #7286?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially, yes. That PR fixed us to throw similar things from JS and wasm, an i31 of 0 from wasm, and a JS number of 0. But it turns out that wasn't enough as the externref inside the exnref might end up logged out, and we need that logging to be identical. Throwing an object from both wasm and JS leads to both of them logging "object".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the results in this line says not
object
butjs exception
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the difference can't show up there, because the fuzzer treats exceptions as interchangeable. That is, it converts all text beginning with "exception thrown: " into the same thing, so they all compare the same. (because different VMs have different error messages)
The content of the exception can be read without throwing it, though, if we catch it and return the contents to the outside. Then this difference is noticeable. Here is the test, showing
object
:https://github.com/WebAssembly/binaryen/pull/7287/files#diff-a32958975d55fbae4aa24cc650a4d85d1cecaa8f4a8fdb5955372eb77fb7efcaR392