Clemens, could you please take a look (to spread reviewer load).
CC'ing Matthias to keep him in the loop wrt. Wasm-in-JS inlining.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
CHECK(v8_flags.wasm_lazy_validation);
I don't think we can have any other configuration where we end up in this branch. E.g., Wasm eager compilation, PGO, or compilation hints would not trigger JS compilation (in which we would inline the Wasm function).
try {
I manually reduced this from the Clusterfuzz test case, no idea where the try/catch came from there, but I didn't want to spend much more time on it.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
LGTM, thanks for fixing, not sure why ClusterFuzz resolved the issue already but your reproducer looks nice, so we should have something reliable now. 😊
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
CHECK(v8_flags.wasm_lazy_validation);
I don't think we can have any other configuration where we end up in this branch. E.g., Wasm eager compilation, PGO, or compilation hints would not trigger JS compilation (in which we would inline the Wasm function).
Yeah, I think it's good to encode these assumptions into the code. If they get invalidated, we can still address it. IMO better have a chromecrash for this than having a vulnerability because we unexpectedly hit this branch. 😊
I manually reduced this from the Clusterfuzz test case, no idea where the try/catch came from there, but I didn't want to spend much more time on it.
Both ochang and Fuzzilli like to insert try-catch blocks to continue execution after something that throws. E.g. in this case we need it as the first call fails and we then still need to continue to hit the interesting case.
Commit-Queue | +2 |
Thanks for the quick review, Matthias (despite the high load!). I'll land and move Clemens to CC, happy to address further issues in a follow-up.
CHECK(v8_flags.wasm_lazy_validation);
Matthias LiedtkeI don't think we can have any other configuration where we end up in this branch. E.g., Wasm eager compilation, PGO, or compilation hints would not trigger JS compilation (in which we would inline the Wasm function).
Yeah, I think it's good to encode these assumptions into the code. If they get invalidated, we can still address it. IMO better have a chromecrash for this than having a vulnerability because we unexpectedly hit this branch. 😊
Acknowledged
Matthias LiedtkeI manually reduced this from the Clusterfuzz test case, no idea where the try/catch came from there, but I didn't want to spend much more time on it.
Both ochang and Fuzzilli like to insert try-catch blocks to continue execution after something that throws. E.g. in this case we need it as the first call fails and we then still need to continue to hit the interesting case.
D'uh, good point, since this doesn't validate, it obviously will throw during execution. Resolved, thanks 😊
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
[wasm] Fix Wasm-in-JS inlining with lazy validation
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
return OpIndex::Invalid();
nit: `V<Any>::Invalid()`
(and maybe consider replacing all other occurrences of OpIndex::Invalid() throughout this function)
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
LGTM from my side (same fix as always with lazy validation).
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |