Ensure a requested amount of stack is avaiable before hostcalls#567
Conversation
| // trampoline to check the stack first. in that case, return the trampoline function | ||
| // instead. | ||
| let colocated = !func_decl.imported(); | ||
| let name = if colocated { |
There was a problem hiding this comment.
I also think this might be not quite right, Cranelift docs suggest that guest functions can be !colocated if the module is sufficiently large (f.ex larger than 128mb of code). We would then insert a trampoline between calls internal to the guest, and then have guests fail due to not having enough stack for a hostcall when not making a hostcall at all.
I haven't actually seen guest functions that are !colocated, and I'm not sure if there's a better way to select imported functions here. Even guest functions were erroneously picked here, it would work (with an unintentional penalty of extra function call) so long as stack is available - the error mode in case this is wrong is not too bad.
There was a problem hiding this comment.
!colocated here means the FunctionDecl has an import name, so I think our insertion of the trampoline is correct. What might not be correct is that we're assuming everything without an import name is colocated per Cranelift's definition. Based on your description, that might not be true if the module becomes too large?
acfoltzer
left a comment
acfoltzer
left a comment
There was a problem hiding this comment.
I'm so excited to see this come together in a relatively small patch!
In addition to the individual comments, I would like to see a bit more test coverage before merging. The new test case is a degenerate case where the hostcall stack limit is equal to the base of the stack. I don't see any problems with the more general case, but I would sleep better if we had a case that tested enforcement of a more useful limit.
It may help to start with one of the tests in stack.rs which already serve to bring a guest up to a specific level of stack consumption.
| let hostcall_args = builder.block_params(hostcall_block).to_vec(); | ||
| let call_inst = builder.ins().call(hostcall_ref, &hostcall_args); | ||
| let results = builder.inst_results(call_inst).to_vec(); | ||
| builder.ins().return_(&results); |
There was a problem hiding this comment.
to_vec()
Dangit, borrow checker!
| // trampoline to check the stack first. in that case, return the trampoline function | ||
| // instead. | ||
| let colocated = !func_decl.imported(); | ||
| let name = if colocated { |
There was a problem hiding this comment.
!colocated here means the FunctionDecl has an import name, so I think our insertion of the trampoline is correct. What might not be correct is that we're assuming everything without an import name is colocated per Cranelift's definition. Based on your description, that might not be true if the module becomes too large?
|
Also, if you're up for it, adding a page describing this to |
|
Docs and tests (and cleanup) have been added! The two additional tests I've added are pulled from Docs were good because I had to go through a minor exercise justifying why the trampoline stack check's condition was correct; Cranelift's operand order for |
|
|
something here is off though... need to think through the stack comparison some more, or carefully investigate stack limits. either things appear to work, but yield tests fail, or yield tests work, and the hostcall reservation test fails.
also adjust Limits so that the only Limits that can be safely constructed also has a rasonable hostcall reservation
iximeow
force-pushed
the
ixi/hostcall-stack-overflows
branch
from
ff69891 to
dbf03e9
Compare
pchickey
left a comment
pchickey
left a comment
There was a problem hiding this comment.
Excellent work, and great write up as well!
3 participants
This changes
lucetc's generation of hostcalls to, instead of directly calling hostcalls, call a synthetic trampoline to do some safety checks. That trampoline then will call the original hostcall, if it passes the required checks.In our case, the only check is that the guest's stack use is below some threshold that the instance has set, ensuring that an amount of space is available for hostcalls. My arbitrary selection for a hostcall stack space guarantee is 16kb. If the guest fails the stack availability check, it terminates with a stack overflow - this mitigates the issues if a stack overflow in host code by making it an explicit stack overflow in the guest when reasonable. Hostcall stack overflows may still occur, and are still fatal, but we can now guarantee a certain amount of space for hostcall authors. This means hostcall implementors have the same kind of constraints as writing any other function - a stack overflow in any other host code would be fatal in a way beyond our control, just the same.
This adds a notable testing complication: since lucetc inserts non-wasm-derived details now, the mock modules we use in
lucet-runtimetests are a little further from being real modules. Mock modules don't have trampolines between mocked "guest" code and hostcalls they would make, and I don't think we can insert trampolines if we wanted (without a worrying maintenance burden and risk of worse mock/real skew). My conclusion is that testing details related to hostcall stack overflows must be done using real wasm modules through lucetc, and tests not involving hostcall stack overflows (so, the ones that exist already), can continue being against mock modules.