Skip to content
This repository has been archived by the owner on May 11, 2020. It is now read-only.

Fix type conversion panic for host-provided funcs #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ericflo
Copy link

@ericflo ericflo commented Nov 13, 2019

It was panicking because it's of type goFunction, not *goFunction as the line is checking for.

It was panicking because it's of type goFunction, not *goFunction as the line is checking for.
Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR.

could you add a test/example showing the panic you are trying to fix with this PR?

thanks again.

@@ -91,7 +91,7 @@ func (vm *VM) tryNativeCompile() error {
}

for i := range vm.funcs {
if _, isGoFunc := vm.funcs[i].(*goFunction); isGoFunc {
if _, isGoFunc := vm.funcs[i].(goFunction); isGoFunc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum...
this kind of type assertion, using the "comma-ok" variant, shouldn't panic.
so I must admit I am not completely clear on what panic this should cure.

that said, it seems indeed that we should probably type-assert for goFunction:

$> git grep goFunction
exec/func.go:38:type goFunction struct {
exec/func.go:43:func (fn goFunction) call(vm *VM, index int64) {
exec/native_compile.go:94:              if _, isGoFunc := vm.funcs[i].(*goFunction); isGoFunc {
exec/native_compile.go:181:             if _, isGoFunc := vm.funcs[i].(*goFunction); isGoFunc {
exec/vm.go:130:                 vm.funcs[i] = goFunction{

(as we only ever fill vm.funcs with goFunction{...} values...)

Copy link
Author

@ericflo ericflo Nov 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my fault for underspecifying - it passes this check, so in the code below it (line 98) it panics trying to access it as a compiledFunction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I dont understand how we would hit this panic. Consider that vm.funcs is of type []function, and function is an interface type. The goFunction type only implements the necessary interface methods on itself, it does not implement it on its pointer receiver.

As such, I was thinking only goFunction{} could implement function, not a *goFunction ? Now I'm not sure ...

Either way, the current logic is negated, considering we only want to run the loop logic on the func if it is of type compiledFunction.

What do you think of a fix like this?

fn, isCompiledFn := vm.funcs[i].(compiledFunction)
if !isCompiledFn {
  continue
}

@ericflo
Copy link
Author

ericflo commented Nov 13, 2019

I'll look into adding a new test, but one way to test it would be to run the existing host-provided func test with AOT enabled

@sbinet
Copy link
Contributor

sbinet commented Nov 27, 2019

ping?

@fabian-z
Copy link

fabian-z commented Apr 16, 2020

I can confirm that using host provided functions with the AOT backend enabled causes a panic for me that is fixed by this change (I tested with rebasing on top of master). If needed, I can share a minimal reproducer.

The panic happens here:

fn := vm.funcs[i].(compiledFunction)

Stack trace:

panic: interface conversion: exec.function is exec.goFunction, not exec.compiledFunction

goroutine 1 [running]:
github.com/go-interpreter/wagon/exec.(*VM).tryNativeCompile(0xc0007fc000, 0xc00006f770, 0x0)
	/home/fabian/go/pkg/mod/github.com/go-interpreter/[email protected]/exec/native_compile.go:98 +0x85d
github.com/go-interpreter/wagon/exec.NewVM(0xc00013c000, 0xc0007e7f30, 0x1, 0x1, 0x0, 0x0, 0x0)
	/home/fabian/go/pkg/mod/github.com/go-interpreter/[email protected]/exec/vm.go:175 +0x4cd
main.main()
	/home/fabian/playground/wasm-runtime/main.go:285 +0x1a8

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants