Fuzz with start functions#8736
Conversation
| // for interesting things like modifying globals and memory etc. | ||
|
|
||
| auto* start = wasm.getFunction(wasm.start); | ||
| if (start->imported()) { |
There was a problem hiding this comment.
Can we filter this out in pickStart?
There was a problem hiding this comment.
Hmm, yeah, that would allow more working things. Let me try to allow calls to imports entirely, as you suggested below, first, and if it fails I'll do that. (if will be moot if it succeeds)
| // Fuzzing with a start function is tricky, as if it calls imports that could | ||
| // cause reentrancy issues (the module is not yet returned to the JS/VM side, | ||
| // yet, so things are not ready for calls to happen yet). Still, fuzzing the | ||
| // start is important, so just remove all calls, which still leaves a chance | ||
| // for interesting things like modifying globals and memory etc. |
There was a problem hiding this comment.
Given that we can still throw an exception or trap in the start function, how bad would it be for us to allow calling imports? The worst they can do is throw, right?
There was a problem hiding this comment.
The reentrancy is even worse than throwing. It doesn't necessarily throw, in fact - it might call an import that calls exports with a try-catch, swallowing the exception on no exports existing. I took a look at some stuff here and it was not fun.
Though maybe this is important to fuzz for VM errors, even if it isn't for real-world code?
There was a problem hiding this comment.
I guess if it silently messes up the fuzz_shell.js state, that would be bad. But I'm not sure how that would happen. Maybe we can add an initializationFinished flag in fuzz_shell.js and add logging with early exits to all the imports we provide (or maybe just those that would call back in to exports).
But if there are problems that would require more invasive fixes than that, then avoiding the issue by removing calls sounds ok.
There was a problem hiding this comment.
Ok, I found a testcase and reminded myself why this is so bad. This is confusing in VMs, but in wasm-opt's own interpreter, it just breaks entirely. We are still running initialization and I guess in the C++ constructor, when this reentrancy happens, leading to crashes. Might be worth looking into it but it could require a large refactoring of our interpreter.
There was a problem hiding this comment.
Oh yeah doing fallible work in a constructor isn't great. Want to at least add a TODO about doing that refactoring and allowing fuzzing of start functions calling other functions, including imports?
There was a problem hiding this comment.
Yes, added in the last commit actually.
2 participants
Pick a start, or allow an existing one to remain, some of the time.
Also handle traps and exceptions during start in the fuzzer.