From aee401b69a798034aa6d0b30aeba99ef7aaa5b35 Mon Sep 17 00:00:00 2001 From: Andrei Matei Date: Wed, 4 Jan 2023 12:07:23 -0500 Subject: [PATCH] pkg/proc: populate pointer values (#3229) * proc: add a test for dangling unsafe pointers This new tests checks the behavior when dereferencing dangling pointers. The behavior does not fully make sense; the test checks the current behavior for now, which will be improved in subsequent commits. * proc: populate pointer values This patch changes how Value and Unreadable are populated for pointer Variables. Before this patch, variables of kind reflect.Ptr did not have their Value field populated. This patch populates it in Variable.loadValue(), which seems natural and consistent with other types of variables. The Value is the address that the pointer points to. The Unreadable field was populated inconsistently for pointer variables: it was never populated for an outer pointer, but it could be populated for an inner pointer in pointer-to-pointer types. Before this patch, in pointer whose value could not be read was not easily distinguishable from a pointer with a value that could be read, but that cannot be dereferenced (i.e. a dangling pointer): neither of these would be marked as Unreadable, and both would have a child marked as Unreadable. This patch makes it so that a pointer variable whose pointer value cannot be read is marked as Unreadable. Using this new distinction, this patch fixes a bug around dereferencing dangling pointers: before, attempting such a dereference produced a "nil pointer dereference" error. This was bogus, since the pointer was not nil. Now, we are more discerning and generate a different error. --- _fixtures/testfnpos.go | 16 +++++++++ _fixtures/testunsafepointers.go | 17 +++++++++ pkg/proc/eval.go | 9 ++++- pkg/proc/variables.go | 40 +++++++++++++++++++-- pkg/proc/variables_test.go | 63 +++++++++++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 3 deletions(-) create mode 100644 _fixtures/testfnpos.go create mode 100644 _fixtures/testunsafepointers.go diff --git a/_fixtures/testfnpos.go b/_fixtures/testfnpos.go new file mode 100644 index 00000000..9572603f --- /dev/null +++ b/_fixtures/testfnpos.go @@ -0,0 +1,16 @@ +package main + +import "fmt" + +func f2() { + fmt.Printf("f2\n") +} + +func f1() { + fmt.Printf("f1\n") +} + +func main() { + f1() + f2() +} diff --git a/_fixtures/testunsafepointers.go b/_fixtures/testunsafepointers.go new file mode 100644 index 00000000..13bbecab --- /dev/null +++ b/_fixtures/testunsafepointers.go @@ -0,0 +1,17 @@ +package main + +import ( + "runtime" + "unsafe" +) + +func main() { + // We're going to produce a pointer with a bad address. + badAddr := uintptr(0x42) + unsafeDanglingPtrPtr := unsafe.Pointer(badAddr) + // We produce a **int, instead of more simply a *int, in order for the test + // program to test more complex Delve behavior. + danglingPtrPtr := (**int)(unsafeDanglingPtrPtr) + _ = danglingPtrPtr + runtime.Breakpoint() +} diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index 94ef2784..1afa2294 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -1571,7 +1571,14 @@ func (scope *EvalScope) evalPointerDeref(node *ast.StarExpr) (*Variable, error) xev.Children[0].OnlyAddr = false return &(xev.Children[0]), nil } - rv := xev.maybeDereference() + xev.loadPtr() + if xev.Unreadable != nil { + val, ok := constant.Uint64Val(xev.Value) + if ok && val == 0 { + return nil, fmt.Errorf("couldn't read pointer: %w", xev.Unreadable) + } + } + rv := &xev.Children[0] if rv.Addr == 0 { return nil, fmt.Errorf("nil pointer dereference") } diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go index bef2287d..58308942 100644 --- a/pkg/proc/variables.go +++ b/pkg/proc/variables.go @@ -124,6 +124,9 @@ type Variable struct { // number of elements to skip when loading a map mapSkip int + // Children lists the variables sub-variables. What constitutes a child + // depends on the variable's type. For pointers, there's one child + // representing the pointed-to variable. Children []Variable loaded bool @@ -1244,6 +1247,40 @@ func (v *Variable) maybeDereference() *Variable { } } +// loadPtr assumes that v is a pointer and loads its value. v also gets a child +// variable, representing the pointed-to value. If v is already loaded, +// loadPtr() is a no-op. +func (v *Variable) loadPtr() { + if len(v.Children) > 0 { + // We've already loaded this variable. + return + } + + t := v.RealType.(*godwarf.PtrType) + v.Len = 1 + + var child *Variable + if v.Unreadable == nil { + ptrval, err := readUintRaw(v.mem, v.Addr, t.ByteSize) + if err == nil { + child = v.newVariable("", ptrval, t.Type, DereferenceMemory(v.mem)) + } else { + // We failed to read the pointer value; mark v as unreadable. + v.Unreadable = err + } + } + + if v.Unreadable != nil { + // Pointers get a child even if their value can't be read, to + // maintain backwards compatibility. + child = v.newVariable("", 0 /* addr */, t.Type, DereferenceMemory(v.mem)) + child.Unreadable = fmt.Errorf("parent pointer unreadable: %w", v.Unreadable) + } + + v.Children = []Variable{*child} + v.Value = constant.MakeUint64(v.Children[0].Addr) +} + func loadValues(vars []*Variable, cfg LoadConfig) { for i := range vars { vars[i].loadValueInternal(0, cfg) @@ -1263,8 +1300,7 @@ func (v *Variable) loadValueInternal(recurseLevel int, cfg LoadConfig) { v.loaded = true switch v.Kind { case reflect.Ptr, reflect.UnsafePointer: - v.Len = 1 - v.Children = []Variable{*v.maybeDereference()} + v.loadPtr() if cfg.FollowPointers { // Don't increase the recursion level when dereferencing pointers // unless this is a pointer to interface (which could cause an infinite loop) diff --git a/pkg/proc/variables_test.go b/pkg/proc/variables_test.go index a46826ea..9af5956b 100644 --- a/pkg/proc/variables_test.go +++ b/pkg/proc/variables_test.go @@ -1610,3 +1610,66 @@ func TestEvalExpressionGenerics(t *testing.T) { } }) } + +// Test the behavior when reading dangling pointers produced by unsafe code. +func TestBadUnsafePtr(t *testing.T) { + withTestProcess("testunsafepointers", t, func(p *proc.Target, fixture protest.Fixture) { + assertNoError(p.Continue(), t, "Continue()") + + // danglingPtrPtr is a pointer with value 0x42, which is an unreadable + // address. + danglingPtrPtr, err := evalVariableWithCfg(p, "danglingPtrPtr", pnormalLoadConfig) + assertNoError(err, t, "eval returned an error") + t.Logf("danglingPtrPtr (%s): unreadable: %v. addr: 0x%x, value: %v", + danglingPtrPtr.TypeString(), danglingPtrPtr.Unreadable, danglingPtrPtr.Addr, danglingPtrPtr.Value) + assertNoError(danglingPtrPtr.Unreadable, t, "danglingPtrPtr is unreadable") + if val := danglingPtrPtr.Value; val == nil { + t.Fatal("Value not set danglingPtrPtr") + } + val, ok := constant.Uint64Val(danglingPtrPtr.Value) + if !ok { + t.Fatalf("Value not uint64: %v", danglingPtrPtr.Value) + } + if val != 0x42 { + t.Fatalf("expected value to be 0x42, got 0x%x", val) + } + if len(danglingPtrPtr.Children) != 1 { + t.Fatalf("expected 1 child, got: %d", len(danglingPtrPtr.Children)) + } + + badPtr, err := evalVariableWithCfg(p, "*danglingPtrPtr", pnormalLoadConfig) + assertNoError(err, t, "error evaluating *danglingPtrPtr") + t.Logf("badPtr: (%s): unreadable: %v. addr: 0x%x, value: %v", + badPtr.TypeString(), badPtr.Unreadable, badPtr.Addr, badPtr.Value) + if badPtr.Unreadable == nil { + t.Fatalf("badPtr should be unreadable") + } + if badPtr.Addr != 0x42 { + t.Fatalf("expected danglingPtr to point to 0x42, got 0x%x", badPtr.Addr) + } + if len(badPtr.Children) != 1 { + t.Fatalf("expected 1 child, got: %d", len(badPtr.Children)) + } + badPtrChild := badPtr.Children[0] + t.Logf("badPtr.Child (%s): unreadable: %v. addr: 0x%x, value: %v", + badPtrChild.TypeString(), badPtrChild.Unreadable, badPtrChild.Addr, badPtrChild.Value) + // We expect the dummy child variable to be marked as unreadable. + if badPtrChild.Unreadable == nil { + t.Fatalf("expected x to be unreadable, but got value: %v", badPtrChild.Value) + } + + // Evaluating **danglingPtrPtr fails. + _, err = evalVariableWithCfg(p, "**danglingPtrPtr", pnormalLoadConfig) + if err == nil { + t.Fatalf("expected error doing **danglingPtrPtr") + } + expErr := "couldn't read pointer" + if !strings.Contains(err.Error(), expErr) { + t.Fatalf("expected \"%s\", got: \"%s\"", expErr, err) + } + nexpErr := "nil pointer dereference" + if strings.Contains(err.Error(), nexpErr) { + t.Fatalf("shouldn't have gotten \"%s\", but got: \"%s\"", nexpErr, err) + } + }) +}