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 |
fitzgen
left a comment
fitzgen
left a comment
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!
| 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! :/ |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
I was not merged. @fitzgen do you know why this may happen 🤔? |
|
It looks like CI failed, you can see this via the "view details" button next to the "github-merge-queue bot removed this pull request from the merge queue due to failed status checks" notification. In particular, this job failed: https://github.com/bytecodealliance/wasmtime/actions/runs/16817124055/job/47636482289 It looks like it is old enough that the logs were deleted however, so I will try re-enqueing this PR and if it fails again, you can see the failure via the method described above. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
This failure looks like some spurious networking issue involving docker or something. Retrying once more. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Looks like this time there was an internal assertion inside the macos linker?? Retrying once more... |
|
Thanks!🤞 |
* Fix Out-of-memory in table-ops * Remove missed println! --------- Co-authored-by: Khagan Karimov <[email protected]>
2 participants
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