-
Notifications
You must be signed in to change notification settings - Fork 147
Fix type conversion panic for host-provided funcs #178
base: master
Are you sure you want to change the base?
Conversation
It was panicking because it's of type goFunction, not *goFunction as the line is checking for.
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
}
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 |
ping? |
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: Line 98 in dc6758c
Stack trace:
|
It was panicking because it's of type goFunction, not *goFunction as the line is checking for.