proc: fix breakpoint confusion on resume (#1738)

Fixes a case of breakpoint confusion on resume caused by having two
breakpoints one byte apart. This bug can cause the target program to
resume execution a single byte inside an instruction and crash either
with SIGILL or a SIGSEGV, or misbehave (depending on how the truncated
instruction is decoded).

native.(*Thread).StepInstruction should call FindBreakpoint using
adjustPC==false because at that point the PC of the thread should
already have been adjusted (and it has been).

_fixtures/nopbreakpoint/main.go

@@ -0,0 +1,18 @@
+package main
+
+import (
+	"fmt"
+)
+
+var g int = 0
+
+func compromised()
+
+func skipped() {
+	g++
+}
+
+func main() {
+	compromised()
+	fmt.Printf("%d\n", g)
+}

_fixtures/nopbreakpoint/main.s

@@ -0,0 +1,6 @@
+#include "textflag.h"
+
+TEXT ·compromised(SB),NOSPLIT,$0-0
+	BYTE	 $0x90   // The assembler strips NOP, this is a hardcoded NOP instruction
+	CALL main·skipped(SB)
+	RET

pkg/proc/native/threads.go

@@ -58,7 +58,7 @@ func (t *Thread) StepInstruction() (err error) {
 		return err
 	}
 
-	bp, ok := t.dbp.FindBreakpoint(pc, true)
+	bp, ok := t.dbp.FindBreakpoint(pc, false)
 	if ok {
 		// Clear the breakpoint so that we can continue execution.
 		err = t.ClearBreakpoint(bp)

pkg/proc/proc_test.go

@@ -4383,3 +4383,27 @@ func TestIssue1656(t *testing.T) {
 		assertLineNumber(p, t, 9, "wrong line number after second step")
 	})
 }
+
+func TestBreakpointConfusionOnResume(t *testing.T) {
+	// Checks that SetCurrentBreakpoint, (*Thread).StepInstruction and
+	// native.(*Thread).singleStep all agree on which breakpoint the thread is
+	// stopped at.
+	// This test checks for a regression introduced when fixing Issue #1656
+	if runtime.GOARCH != "amd64" {
+		t.Skip("amd64 only")
+	}
+	withTestProcess("nopbreakpoint/", t, func(p proc.Process, fixture protest.Fixture) {
+		maindots := filepath.ToSlash(filepath.Join(fixture.BuildDir, "main.s"))
+		maindotgo := filepath.ToSlash(filepath.Join(fixture.BuildDir, "main.go"))
+		setFileBreakpoint(p, t, maindots, 5) // line immediately after the NOP
+		assertNoError(proc.Continue(p), t, "First Continue")
+		assertLineNumber(p, t, 5, "not on main.s:5")
+		setFileBreakpoint(p, t, maindots, 4)   // sets a breakpoint on the NOP line, which will be one byte before the breakpoint we currently are stopped at.
+		setFileBreakpoint(p, t, maindotgo, 18) // set one extra breakpoint so that we can recover execution and check the global variable g
+		assertNoError(proc.Continue(p), t, "Second Continue")
+		gvar := evalVariable(p, t, "g")
+		if n, _ := constant.Int64Val(gvar.Value); n != 1 {
+			t.Fatalf("wrong value of global variable 'g': %v (expected 1)", gvar.Value)
+		}
+	})
+}