Fuzz bug: interpretation of return_call is not correct.

This module behaves differently in our interpreter and in v8:

(module
 (import "fuzzing-support" "log-i32" (func $log (param i32)))
 (tag $t (param))

 (func $throw
  (throw $t)
 )

 (func $try-retcall
  (try $label$3
   (do
    (return_call $throw)
   )
   (catch $t)
  )
 )

 (func $test (export "test")
  (call $try-retcall)
  (call $log
   (i32.const 0)
  )
 )
)

Since we interpret return_call as call + return, the thrown exception is caught by the try and $test continues on to call $log. The correct behavior is that $try-retcall should return and then $throw should be called, so $test should propagate the exception and never call $log.

@aheejin

Wait, what? A return_call nested in a try-catch can throw without the try-catch catching it? Then I'm pretty sure we have several bugs around that, from effects.h to specific passes, because we assume that structured control flow implies what can be caught: the return_call is inside a try-catch, so we consider it catch-able.

The spec says Tail calls behave like a combination of return followed by a respective call which seems ambiguous at best, and it does not mention the interaction with try-catch anywhere that I can see.

cc @aheejin - did this EH/tail-call interaction come up on the EH side?

Yes! The return gets you out of the try and if the subsequent call throws, you're no longer in the try to catch it. Let's fix some bugs 🐛

Not that I doubt you 😄 but this is a huge gotcha in the spec - where is it documented?

Here's where the tail calling semantics are described: https://webassembly.github.io/tail-call/core/exec/instructions.html#tail-invocation-of-function-address-a

The important part is that it pops everything up to and including the current call frame. This will pop any handlers off the stack.

Here's an issue where this was explicitly discussed: WebAssembly/exception-handling#249

I see, thanks...

To me this is extremely surprising as it breaks the basic nesting property:

(func $i-do-not-throw
  (try
    ..body..
    (catch_all)
  )
)

That try is the entire function body, and the catch-all seems like it should capture any exceptions from ..body.., no matter what they are. That is how JS works:

function iDoNotThrow() {
  try {
    ..body..
  } catch (c) {}
}

But in wasm, a "clever" use of return_call in place of ..body.. can break that expectation. To me this is pretty shocking, personally, aside from the work it will take to fix in Binaryen (which I don't have an immediate estimate of).

By "clever" I mean that inlining code that contains a return_call becomes dangerous here, for example 😱

By "clever" I mean that inlining code that contains a return_call becomes dangerous here, for example 😱

I think that's ok, since this return_call is going to be turned into a call. That's inlining a return_call within a try which is dangerous.

I suspect the sane thing to do is to maintain the invariant that there is no return_call within a try, with a pass at the start which would move any such occurrence outside of the try.

@kripken

Ah, right, this was indeed surprising when I first discovered it, and I somehow forgot to fix it in Binaryen... Do others currently generate tail calls by default? Emscripten doesn't, but do you know about others including J2Wasm and others?

I asked a question on changing the spec here (WebAssembly/exception-handling#249 (comment)) but I don't think people would have much appetite for changing the already-half-deprecated spec at this point.

@kripken @vouillon

By "clever" I mean that inlining code that contains a return_call becomes dangerous here, for example 😱

I think that's ok, since this return_call is going to be turned into a call. That's inlining a return_call within a try which is dangerous.

Do we turn a return_call into a call when inlining? (If so, why?)

I suspect the sane thing to do is to maintain the invariant that there is no return_call within a try, with a pass at the start which would move any such occurrence outside of the try.

How can we move a return_call outside of a try without changing the semantics?

We can fix effects.h so that we scan the body of try-catch_all does not contain return_call before marking it as nothrow. But yeah if inlining happens within this function this effect has to be recomputed, which I don't think we would do. Should we just mark try-catch_all as mayThrow when tail-call is enabled?

A try or try_table with a catch_all (that does not itself throw, in the case of try) will never throw from the perspective of the surrounding code, but it may return if it contains a return or return_call (or any other tail call variant). So I think the only effect we need to add is branchesOut, which we already do for return_call and friends.

Do we turn a return_call into a call when inlining? (If so, why?)

If the caller contains call $callee and the callee contains a return_call $f, then before inlining, the return value of $f would have been returned from the call $callee instruction. When $callee is inlined, we still need that return value to be returned to the caller. If the caller contained return_call $f after inlining, it would incorrectly return the return value of $f instead of receiving it. To fix this, we currently replace return_call with call + br $inlined-block in the inlined code, but that's not correct in the presence of EH.

I can take a look at fixing inlining to handle this correctly.

@aheejin

@aheejin

Should we just mark try-catch_all as mayThrow when tail-call is enabled?

Maybe we can add to that, to only do it when calls has been set. That is, we can take advantage of the children that have already been scanned, to see if a call exists. (Though that won't be fully optimal in case of tested try's etc.)

@tlively

A try or try_table with a catch_all (that does not itself throw, in the case of try) will never throw from the perspective of the surrounding code, but it may return if it contains a return or return_call (or any other tail call variant).

Yes to the first part, but it may still throw from the perspective of other functions calling it. So perhaps we can get away with only adding mayThrow effect on entire functions, here?

binaryen/src/ir/effects.h

Line 66 in 1125750

// Walk an entire function body. This will ignore effects that are not

But I'm not sure. We should only have very few places that compute effects in a way observable by other functions, at least, and hopefully they all call that one method.

Ah, makes sense. Yes, hopefully we can make a local change in that function.

Can you explain the mayThrow stuff more? I don't see that term in effects.h.

Sorry, looks like the field is called throws_.

Do J2Wasm or other toolchains currently generate return_calls by default?

@vouillon

I don't believe j2wasm emits return_call, but it looks like @vouillon's OCaml compiler does.

@aheejin

OCaml optimizes tail calls when it does not change the semantics. So, my compiler changes all call + return into return_call except inside try blocks.

I don't know of any language which allows tail-call in the scope of an exception handler. And I don't see any useful optimization which would move a tail-call within the scope of an exception handler. So, I think it's going to be exceedingly rare to find a return_call within a try block.

Do we turn a return_call into a call when inlining? (If so, why?)

[...] To fix this, we currently replace return_call with call + br $inlined-block in the inlined code, but that's not correct in the presence of EH.

Oh, indeed, it is not correct to turn a return_call inside a try block into a call + br. I did not thought about that. But if there is no return_call inside a try block anywhere in the initial code, I believe the current implementation of inlining is correct. Maybe you could just disable inlining when it might not be correct? That is, you do not inline a return_call within a try block and you bails out if you encounter one in the body of the inlined function.

@aheejin

I suspect the sane thing to do is to maintain the invariant that there is no return_call within a try, with a pass at the start which would move any such occurrence outside of the try.

How can we move a return_call outside of a try without changing the semantics?

Well, the semantics of a return_call is to pop all the exception handlers and then to perform the call. But popping the exception handlers is also what branching outside a try block does.

We can fix effects.h so that we scan the body of try-catch_all does not contain return_call before marking it as nothrow. But yeah if inlining happens within this function this effect has to be recomputed, which I don't think we would do. Should we just mark try-catch_all as mayThrow when tail-call is enabled?

Inlining should not change the semantics of the code, so one should not need to recompute the effect.

To fix effects.h, would it be enough to set parent.throws_ not only when parent.tryDepth == 0 but also when parent.tryDepth > 0 && curr->isReturn?

Fixed by #6451

Navigation Menu

Repository navigation

Fuzz bug: interpretation of `return_call` is not correct. #6436

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Fuzz bug: interpretation of return_call is not correct. #6436

Description

Activity

kripken commented on Mar 25, 2024

tlively commented on Mar 25, 2024

kripken commented on Mar 25, 2024

tlively commented on Mar 25, 2024

kripken commented on Mar 25, 2024

vouillon commented on Mar 25, 2024

aheejin commented on Mar 26, 2024

aheejin commented on Mar 26, 2024

tlively commented on Mar 26, 2024

kripken commented on Mar 26, 2024

tlively commented on Mar 26, 2024

kripken commented on Mar 26, 2024

aheejin commented on Mar 26, 2024

tlively commented on Mar 26, 2024

vouillon commented on Mar 26, 2024

tlively commented on Apr 9, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Fuzz bug: interpretation of `return_call` is not correct. #6436