Skip to content

Conversation

@tlively
Copy link
Member

@tlively tlively commented Jul 28, 2021

The tail call spec does not include subtyping because it is based on the
upstream spec, which does not contain subtyping. However, there is no reason
that subtyping shouldn't apply to tail calls like it does for any other call or
return. Update the validator to allow subtyping and avoid a related null pointer
dereference while we're at it.

Do not run the test in with --nominal because it is buggy in that mode.

The tail call spec does not include subtyping because it is based on the
upstream spec, which does not contain subtyping. However, there is no reason
that subtyping shouldn't apply to tail calls like it does for any other call or
return. Update the validator to allow subtyping and avoid a related null pointer
dereference while we're at it.
@kripken
Copy link
Member

kripken commented Jul 28, 2021

cc @fgmccabe - I'm curious to hear your thoughts and experience on this. My intuition is that it's scary to support subtyping in tail calls, as only when the types are identical then is it really obvious that it's fine to "just" reuse the stack frame. My sense is that this type of thing could be a source of exploits. But I don't have much VM experience here - is my intuition wrong?

@fgmccabe
Copy link

Thomas is correct. The tail-call is actually return-call which gives an important clue here. When checking types, the return type of the callee should be compared against the return type of the current function. The typing of the arguments to return-call should be the same as for any call.

@tlively tlively enabled auto-merge (squash) July 28, 2021 18:42
@tlively tlively merged commit 8670f15 into WebAssembly:main Jul 28, 2021
@tlively tlively deleted the tail-call-subtyping branch July 28, 2021 19:00
@kripken
Copy link
Member

kripken commented Jul 28, 2021

Great, thanks @fgmccabe !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants