diff --git a/_fixtures/testvariables_pointers_not_loaded.go b/_fixtures/testvariables_pointers_not_loaded.go new file mode 100644 index 00000000..f21b0ea0 --- /dev/null +++ b/_fixtures/testvariables_pointers_not_loaded.go @@ -0,0 +1,17 @@ +package main + +import "runtime" + +func main() { + type StructWithInterface struct { + Val any + } + var i int + var ba = StructWithInterface{} + ba.Val = &i + + var ptrPtr **int + _ = ptrPtr + + runtime.Breakpoint() +} diff --git a/pkg/terminal/starbind/conv.go b/pkg/terminal/starbind/conv.go index 595409a9..196c295b 100644 --- a/pkg/terminal/starbind/conv.go +++ b/pkg/terminal/starbind/conv.go @@ -453,10 +453,15 @@ func (v ptrVariableAsStarlarkValue) Attr(name string) (starlark.Value, error) { } func (v ptrVariableAsStarlarkValue) AttrNames() []string { + if len(v.v.Children) == 0 { + // The pointer variable was not loaded; we don't know the field names. + return nil + } if v.v.Children[0].Kind != reflect.Struct { return nil } - // autodereference + // autodereference: present the field names of the pointed-to struct as the + // fields of this pointer variable. x := structVariableAsStarlarkValue{&v.v.Children[0], v.env} return x.AttrNames() } diff --git a/pkg/terminal/starbind/starlark.go b/pkg/terminal/starbind/starlark.go index ac74b47f..f34335b0 100644 --- a/pkg/terminal/starbind/starlark.go +++ b/pkg/terminal/starbind/starlark.go @@ -141,12 +141,13 @@ func (env *Env) printFunc() func(_ *starlark.Thread, msg string) { // Source can be either a []byte, a string or a io.Reader. If source is nil // Execute will execute the file specified by 'path'. // After the file is executed if a function named mainFnName exists it will be called, passing args to it. -func (env *Env) Execute(path string, source interface{}, mainFnName string, args []interface{}) (starlark.Value, error) { +func (env *Env) Execute(path string, source interface{}, mainFnName string, args []interface{}) (_ starlark.Value, _err error) { defer func() { err := recover() if err == nil { return } + _err = fmt.Errorf("panic executing starlark script: %v", err) fmt.Fprintf(env.out, "panic executing starlark script: %v\n", err) for i := 0; ; i++ { pc, file, line, ok := runtime.Caller(i) diff --git a/pkg/terminal/starlark_test.go b/pkg/terminal/starlark_test.go index 7a304da1..7f4f8706 100644 --- a/pkg/terminal/starlark_test.go +++ b/pkg/terminal/starlark_test.go @@ -158,3 +158,70 @@ func TestStarlarkVariable(t *testing.T) { } }) } + +// Test that pointer variables that were not loaded don't lead to crashes when +// used in Starlark scripts. +func TestStarlarkVariablePointerNotLoaded(t *testing.T) { + withTestTerminal("testvariables_pointers_not_loaded", t, func(term *FakeTerminal) { + term.MustExec("continue") + + // We're going to partially load some variables through the eval() + // builtin. Then we're going to attempt to evaluate expressions which + // try to access a field from a pointer variable that wasn't loaded + // (i.e. a ptrVariableAsStarlarkValue with no children). The tests will + // verify that we get an error. This is a regression test; we used to + // panic. + for _, tc := range []struct{ name, script, expErr string }{ + { + // In this test, we'll load v. v will have a child (i.e. + // v.Children[0]), but because v is nil, v.Children[0] will not + // be loaded. We'll turn v.Children[0] into a + // ptrVariableAsStarlarkValue, and attempt to access a field on + // it. + // + // v -> structAsStarlarkValue> + // v.Children[0] -> structAsStarlarkValue> + // v.Children[0].Value -> ptrVariableAsStarlarkValue<*int> ; this pointer variable has not been loaded + name: "partial load because of nil ptr", + script: ` +v = eval( + None, "ptrPtr", None + ).Variable +v.Children[0].Value.XXX +`, + expErr: "*int has no .XXX field or method", + }, + { + // In this test, MaxStructFields = 10 and MaxVariableRecurse = 0 + // will cause us to load v, and v.Children[0] (in the sense that + // v.Children[0] has a child), but _not_ load + // v.Children[0].Children[0] (because the recursion limit is + // exhausted by the descent into the top-level struct, so we + // don't load what hides under the interface). + // + // v -> structAsStarlarkValue> + // v.Children[0] -> structAsStarlarkValue> + // v.Children[0].Children[0] -> structAsStarlarkValue> + // v.Children[0].Children[0].Value -> ptrVariableAsStarlarkValue<*int> ; this pointer variable has not been loaded + name: "partial load because of recursion limit", + script: ` +v = eval( + None, "ba", {"MaxVariableRecurse": 0, "MaxStructFields": 10} + ).Variable; +v.Children[0].Children[0].Value.XXX +`, + expErr: "*int has no .XXX field or method", + }, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := term.ExecStarlark(tc.script) + if err == nil { + t.Fatalf("expected error %q, got success", tc.expErr) + } + if !strings.Contains(err.Error(), tc.expErr) { + t.Fatalf("expected error %q, got \"%s\"", tc.expErr, err) + } + }) + } + }) +}