From f19d5e5c135128d35bad9a61813d293f78f99aac Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Fri, 29 Jan 2021 18:25:31 +0100 Subject: [PATCH] proc: fix embedded field search (#2320) Both structMember and findMethod implemented a depth-first search in embedded fields but the Go specification requires a breadth-first search. They also allowed promotion of fields in the concrete type of embedded interfaces even though this is not allowed by Go. Furthermore they both lacked protection from infinite recursion when a type embeds itself and the user requests a non-existent field. Fixes #2316 --- _fixtures/testvariables2.go | 40 ++++++++++- pkg/proc/eval.go | 118 +++++++++++++++++---------------- pkg/proc/variables.go | 88 ++++++++++++------------ service/dap/server_test.go | 4 +- service/test/variables_test.go | 14 ++++ 5 files changed, 162 insertions(+), 102 deletions(-) diff --git a/_fixtures/testvariables2.go b/_fixtures/testvariables2.go index 99caa7b3..7efb06aa 100644 --- a/_fixtures/testvariables2.go +++ b/_fixtures/testvariables2.go @@ -106,6 +106,38 @@ type List struct { Next *List } +type T struct { + F string +} + +type W1 struct { + T +} + +func (*W1) M() {} + +type I interface{ M() } + +type W2 struct { + W1 + T +} + +type W3 struct { + I + T +} + +type W4 struct { + I +} + +type W5 struct { + *W5 +} + +var _ I = (*W2)(nil) + func main() { i1 := 1 i2 := 2 @@ -312,6 +344,12 @@ func main() { ll := &List{0, &List{1, &List{2, &List{3, &List{4, nil}}}}} unread := (*int)(unsafe.Pointer(uintptr(12345))) + w2 := &W2{W1{T{"T-inside-W1"}}, T{"T-inside-W2"}} + w3 := &W3{&W1{T{"T-inside-W1"}}, T{"T-inside-W3"}} + w4 := &W4{&W1{T{"T-inside-W1"}}} + w5 := &W5{nil} + w5.W5 = w5 + var amb1 = 1 runtime.Breakpoint() for amb1 := 0; amb1 < 10; amb1++ { @@ -319,5 +357,5 @@ func main() { } runtime.Breakpoint() - fmt.Println(i1, i2, i3, p1, amb1, s1, s3, a0, a1, p2, p3, s2, as1, str1, f1, fn1, fn2, nilslice, nilptr, ch1, chnil, m1, mnil, m2, m3, m4, upnil, up1, i4, i5, i6, err1, err2, errnil, iface1, iface2, ifacenil, arr1, parr, cpx1, const1, iface3, iface4, recursive1, recursive1.x, iface5, iface2fn1, iface2fn2, bencharr, benchparr, mapinf, mainMenu, b, b2, sd, anonstruct1, anonstruct2, anoniface1, anonfunc, mapanonstruct1, ifacearr, efacearr, ni8, ni16, ni32, ni64, pinf, ninf, nan, zsvmap, zsslice, zsvar, tm, errtypednil, emptyslice, emptymap, byteslice, runeslice, bytearray, runearray, longstr, nilstruct, as2, as2.NonPointerRecieverMethod, s4, iface2map, issue1578, ll, unread) + fmt.Println(i1, i2, i3, p1, amb1, s1, s3, a0, a1, p2, p3, s2, as1, str1, f1, fn1, fn2, nilslice, nilptr, ch1, chnil, m1, mnil, m2, m3, m4, upnil, up1, i4, i5, i6, err1, err2, errnil, iface1, iface2, ifacenil, arr1, parr, cpx1, const1, iface3, iface4, recursive1, recursive1.x, iface5, iface2fn1, iface2fn2, bencharr, benchparr, mapinf, mainMenu, b, b2, sd, anonstruct1, anonstruct2, anoniface1, anonfunc, mapanonstruct1, ifacearr, efacearr, ni8, ni16, ni32, ni64, pinf, ninf, nan, zsvmap, zsslice, zsvar, tm, errtypednil, emptyslice, emptymap, byteslice, runeslice, bytearray, runearray, longstr, nilstruct, as2, as2.NonPointerRecieverMethod, s4, iface2map, issue1578, ll, unread, w2, w3, w4, w5) } diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index f6722fa2..ab144854 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -1963,73 +1963,79 @@ func (v *Variable) findMethod(mname string) (*Variable, error) { return v.Children[0].findMethod(mname) } - typ := v.DwarfType - ptyp, isptr := typ.(*godwarf.PtrType) - if isptr { - typ = ptyp.Type - } + queue := []*Variable{v} + seen := map[string]struct{}{} - typePath := typ.Common().Name - dot := strings.LastIndex(typePath, ".") - if dot < 0 { - // probably just a C type - return nil, nil - } - - pkg := typePath[:dot] - receiver := typePath[dot+1:] - - if fn, ok := v.bi.LookupFunc[fmt.Sprintf("%s.%s.%s", pkg, receiver, mname)]; ok { - r, err := functionToVariable(fn, v.bi, v.mem) - if err != nil { - return nil, err + for len(queue) > 0 { + v := queue[0] + queue = append(queue[:0], queue[1:]...) + if _, isseen := seen[v.RealType.String()]; isseen { + continue } + seen[v.RealType.String()] = struct{}{} + + typ := v.DwarfType + ptyp, isptr := typ.(*godwarf.PtrType) if isptr { - r.Children = append(r.Children, *(v.maybeDereference())) - } else { - r.Children = append(r.Children, *v) + typ = ptyp.Type } - return r, nil - } - if fn, ok := v.bi.LookupFunc[fmt.Sprintf("%s.(*%s).%s", pkg, receiver, mname)]; ok { - r, err := functionToVariable(fn, v.bi, v.mem) - if err != nil { - return nil, err + typePath := typ.Common().Name + dot := strings.LastIndex(typePath, ".") + if dot < 0 { + // probably just a C type + continue } - if isptr { - r.Children = append(r.Children, *v) - } else { - r.Children = append(r.Children, *(v.pointerToVariable())) - } - return r, nil - } - return v.tryFindMethodInEmbeddedFields(mname) -} -func (v *Variable) tryFindMethodInEmbeddedFields(mname string) (*Variable, error) { - structVar := v.maybeDereference() - structVar.Name = v.Name - if structVar.Unreadable != nil { - return structVar, nil - } - switch t := structVar.RealType.(type) { - case *godwarf.StructType: - for _, field := range t.Field { - if field.Embedded { - // Recursively check for promoted fields on the embedded field - embeddedVar, err := structVar.toField(field) - if err != nil { - return nil, err - } - if embeddedMethod, err := embeddedVar.findMethod(mname); err != nil { - return nil, err - } else if embeddedMethod != nil { - return embeddedMethod, nil + pkg := typePath[:dot] + receiver := typePath[dot+1:] + + if fn, ok := v.bi.LookupFunc[fmt.Sprintf("%s.%s.%s", pkg, receiver, mname)]; ok { + r, err := functionToVariable(fn, v.bi, v.mem) + if err != nil { + return nil, err + } + if isptr { + r.Children = append(r.Children, *(v.maybeDereference())) + } else { + r.Children = append(r.Children, *v) + } + return r, nil + } + + if fn, ok := v.bi.LookupFunc[fmt.Sprintf("%s.(*%s).%s", pkg, receiver, mname)]; ok { + r, err := functionToVariable(fn, v.bi, v.mem) + if err != nil { + return nil, err + } + if isptr { + r.Children = append(r.Children, *v) + } else { + r.Children = append(r.Children, *(v.pointerToVariable())) + } + return r, nil + } + + // queue embedded fields for search + structVar := v.maybeDereference() + structVar.Name = v.Name + if structVar.Unreadable != nil { + return structVar, nil + } + switch t := structVar.RealType.(type) { + case *godwarf.StructType: + for _, field := range t.Field { + if field.Embedded { + embeddedVar, err := structVar.toField(field) + if err != nil { + return nil, err + } + queue = append(queue, embeddedVar) } } } } + return nil, nil } diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go index be836944..82dfd81b 100644 --- a/pkg/proc/variables.go +++ b/pkg/proc/variables.go @@ -1060,59 +1060,61 @@ func (v *Variable) structMember(memberName string) (*Variable, error) { v = &v.Children[0] } } - structVar := v.maybeDereference() - structVar.Name = v.Name - if structVar.Unreadable != nil { - return structVar, nil - } - switch t := structVar.RealType.(type) { - case *godwarf.StructType: - for _, field := range t.Field { - if field.Name != memberName { - continue - } - return structVar.toField(field) + queue := []*Variable{v} + seen := map[string]struct{}{} // prevent infinite loops + first := true + + for len(queue) > 0 { + v := queue[0] + queue = append(queue[:0], queue[1:]...) + if _, isseen := seen[v.RealType.String()]; isseen { + continue } - // Check for embedded field only if field was - // not a regular struct member - for _, field := range t.Field { - isEmbeddedStructMember := - field.Embedded || - (field.Type.Common().Name == field.Name) || - (len(field.Name) > 1 && - field.Name[0] == '*' && - field.Type.Common().Name[1:] == field.Name[1:]) - if !isEmbeddedStructMember { - continue - } - // Check for embedded field referenced by type name - parts := strings.Split(field.Name, ".") - if len(parts) > 1 && parts[1] == memberName { + seen[v.RealType.String()] = struct{}{} + + structVar := v.maybeDereference() + structVar.Name = v.Name + if structVar.Unreadable != nil { + return structVar, nil + } + + switch t := structVar.RealType.(type) { + case *godwarf.StructType: + for _, field := range t.Field { + if field.Name == memberName { + return structVar.toField(field) + } + isEmbeddedStructMember := + field.Embedded || + (field.Type.Common().Name == field.Name) || + (len(field.Name) > 1 && + field.Name[0] == '*' && + field.Type.Common().Name[1:] == field.Name[1:]) + if !isEmbeddedStructMember { + continue + } embeddedVar, err := structVar.toField(field) if err != nil { return nil, err } - return embeddedVar, nil + // Check for embedded field referenced by type name + parts := strings.Split(field.Name, ".") + if len(parts) > 1 && parts[1] == memberName { + return embeddedVar, nil + } + embeddedVar.Name = structVar.Name + queue = append(queue, embeddedVar) } - // Recursively check for promoted fields on the embedded field - embeddedVar, err := structVar.toField(field) - if err != nil { - return nil, err - } - embeddedVar.Name = structVar.Name - embeddedField, _ := embeddedVar.structMember(memberName) - if embeddedField != nil { - return embeddedField, nil + default: + if first { + return nil, fmt.Errorf("%s (type %s) is not a struct", vname, structVar.TypeString()) } } - return nil, fmt.Errorf("%s has no member %s", vname, memberName) - default: - if v.Name == "" { - return nil, fmt.Errorf("type %s is not a struct", structVar.TypeString()) - } - return nil, fmt.Errorf("%s (type %s) is not a struct", vname, structVar.TypeString()) + first = false } + + return nil, fmt.Errorf("%s has no member %s", vname, memberName) } func readVarEntry(entry *godwarf.Tree, image *Image) (name string, typ godwarf.Type, err error) { diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 8c71bc63..aecf5520 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -1026,7 +1026,7 @@ func TestScopesAndVariablesRequests2(t *testing.T) { execute: func() { client.StackTraceRequest(1, 0, 20) stack := client.ExpectStackTraceResponse(t) - expectStackFrames(t, stack, "main.main", 317, 1000, 3, 3) + expectStackFrames(t, stack, "main.main", -1, 1000, 3, 3) client.ScopesRequest(1000) scopes := client.ExpectScopesResponse(t) @@ -1039,7 +1039,7 @@ func TestScopesAndVariablesRequests2(t *testing.T) { execute: func() { client.StackTraceRequest(1, 0, 20) stack := client.ExpectStackTraceResponse(t) - expectStackFrames(t, stack, "main.main", 322, 1000, 3, 3) + expectStackFrames(t, stack, "main.main", -1, 1000, 3, 3) client.ScopesRequest(1000) scopes := client.ExpectScopesResponse(t) diff --git a/service/test/variables_test.go b/service/test/variables_test.go index adb69fe8..4ac36268 100644 --- a/service/test/variables_test.go +++ b/service/test/variables_test.go @@ -504,6 +504,20 @@ func TestEmbeddedStruct(t *testing.T) { {"b.C.s", true, "\"hello\"", "\"hello\"", "string", nil}, {"b.s", true, "\"hello\"", "\"hello\"", "string", nil}, {"b2", true, "main.B {A: main.A {val: 42}, C: *main.C nil, a: main.A {val: 47}, ptr: *main.A nil}", "main.B {A: (*main.A)(0x…", "main.B", nil}, + + // Issue 2316: field promotion is breadth first and embedded interfaces never get promoted + {"w2.W1.T.F", true, `"T-inside-W1"`, `"T-inside-W1"`, "string", nil}, + {"w2.W1.F", true, `"T-inside-W1"`, `"T-inside-W1"`, "string", nil}, + {"w2.T.F", true, `"T-inside-W2"`, `"T-inside-W2"`, "string", nil}, + {"w2.F", true, `"T-inside-W2"`, `"T-inside-W2"`, "string", nil}, + {"w3.I.T.F", false, `"T-inside-W1"`, `"T-inside-W1"`, "string", nil}, + {"w3.I.F", false, `"T-inside-W1"`, `"T-inside-W1"`, "string", nil}, + {"w3.T.F", true, `"T-inside-W3"`, `"T-inside-W3"`, "string", nil}, + {"w3.F", true, `"T-inside-W3"`, `"T-inside-W3"`, "string", nil}, + {"w4.I.T.F", false, `"T-inside-W1"`, `"T-inside-W1"`, "string", nil}, + {"w4.I.F", false, `"T-inside-W1"`, `"T-inside-W1"`, "string", nil}, + {"w4.F", false, ``, ``, "", errors.New("w4 has no member F")}, + {"w5.F", false, ``, ``, "", errors.New("w5 has no member F")}, } assertNoError(p.Continue(), t, "Continue()")