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

@@ -0,0 +1,16 @@
+package main
+
+import "fmt"
+
+func f2() {
+	fmt.Printf("f2\n")
+}
+
+func f1() {
+	fmt.Printf("f1\n")
+}
+
+func main() {
+	f1()
+	f2()
+}

_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()
+}

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")
 	}

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)

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)
+		}
+	})
+}