bytecodealliance / wasmtime

A fast and secure runtime for WebAssembly
https://wasmtime.dev/
Apache License 2.0
14.82k stars 1.23k forks source link

wasmtime: Consume fuel on all return paths #8837

Open jameysharp opened 2 weeks ago

jameysharp commented 2 weeks ago

As far as I can tell, when functions use a return instruction rather than falling off the end, fuel was not being tracked correctly. The fuel_function_exit method was only called from after_translate_function, which is only called once all the instructions in the function have been translated, not at each return.

In this commit I switched to calling fuel_function_exit from handle_before_return instead, alongside some of the wmemcheck hooks. That should ensure that it happens on every function exit, regardless of what form that exit takes.

I think after_translate_function is fundamentally difficult to use correctly, and it wasn't used for anything else, so I've also removed it in this commit. And I did a minor cleanup at the site of one of the calls to handle_before_return while I was looking at it.

jameysharp commented 2 weeks ago

I think Nick's out for a few days so I'm handing this off to you instead, Alex.

jameysharp commented 2 weeks ago

Thanks for pointing me to those tests because I immediately got usefully confused about why the tests passed both before and after my changes.

It turns out that fuel_save_from_var is called by way of before_translate_operator for any of unreachable, return, or various kinds of calls. So doing it again in handle_before_return is unnecessary—and calling fuel_save_from_var multiple times is harmless but a waste of time—except when the return is an implicit fall-through off the end of the function. And that case is currently correctly handled by after_translate_function.

So the current implementation is not actually buggy!

But I still think the after_translate_function interface is sketchy. So I want to think more about what interface actually makes sense for instrumenting wasm translation like this, maybe drawing inspiration from whamm. Perhaps for instrumentation purposes we should behave as if there's a return instruction after the end of every function…