Fix Out-of-memory in table-ops #11392
Conversation
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Looks good to me with the nitpick about println! below addressed.
I think we should also switch to calling fixup at the start of to_wasm_binary instead of after each particular mutation, since it has to process all ops and can't take advantage of our knowledge of which mutation we performed and where that mutation was anymore. This change will cut down on the number of call sites and also make it more obvious that the clamping in to_wasm_binary won't ever produce invalid Wasm binaries. (With this PR now, I think we could produce invalid Wasm binaries from to_wasm_binary's clamping due to deserializing some ops that haven't been fixup'd to work with the clamping yet.) This can happen in a follow up PR if you'd prefer.
Thanks!
| @@ -503,6 +541,7 @@ mod tests { | |||
| let wasm = res.to_wasm_binary(); | |||
| let mut validator = Validator::new(); | |||
| let wat = wasmprinter::print_bytes(&wasm).expect("[-] Failed .print_bytes(&wasm)."); | |||
| println!("{wat}"); | |||
There was a problem hiding this comment.
This should be log::debug! and not a println!.
|
Thanks! Yes that makes sense. Initially, I did that. Calling fixup in to_wasm_binary(). It hit assertion failure at for limit > 0. I guess after addressing it we can do that. I will make another PR where clamping happen at the beginning of fixup and fixup is called in encoding. I forgot to remove println! :/ |
TableOpsandTableOpsLimits,which are now passed explicitly toTableOp::fixupTableOps::fixupnow processes the entire sequence of operations instead of starting from specific indexI placed clamping logic at the beginning of the
to_wasm_binarymethod since the OOM issues originate in to_wasm_binaryThis relies onTableOp::fixupto ensure that values respect those clamped limits to avoid potential traps.I previously added clamping at the start of
TableOps::fixupbut the OOM still occurred there.I let it fuzz for a while, and it ran fine but I sense something might be missing.
Related Issues: #11345 and #11346