pkg/proc/native/linux: fix target crashes induced by RequestManualStop (#2484)

A RequestManualStop received while the target program is stopped can
induce a crash when the target is restarted.
This is caused by the phantom breakpoint detection that was introduced
in PR #2179 / commit e69d536.
Instead of always interpreting an unexplained SIGTRAP as a phantom
breakpoint memorize all possible unreported breakpoint hits and only
act on it when the thread hasn't moved from one.

Also clarifies the behavior of the halt command when it is received
while the target is stopped or in the process of stopping.

Documentation/backend_test_health.md

@@ -1,30 +1,30 @@
 Tests skipped by each supported backend:
 
-* 386 skipped = 4% (6/149)
+* 386 skipped = 4% (6/150)
 	* 1 broken
 	* 3 broken - cgo stacktraces
 	* 2 not implemented
-* arm64 skipped = 3.4% (5/149)
+* arm64 skipped = 3.3% (5/150)
 	* 1 broken
 	* 1 broken - cgo stacktraces
 	* 1 broken - global variable symbolication
 	* 2 not implemented
-* darwin skipped = 1.3% (2/149)
+* darwin skipped = 1.3% (2/150)
 	* 2 not implemented
-* darwin/arm64 skipped = 0.67% (1/149)
+* darwin/arm64 skipped = 0.67% (1/150)
 	* 1 broken - cgo stacktraces
-* darwin/lldb skipped = 0.67% (1/149)
+* darwin/lldb skipped = 0.67% (1/150)
 	* 1 upstream issue
-* freebsd skipped = 9.4% (14/149)
+* freebsd skipped = 9.3% (14/150)
 	* 11 broken
 	* 3 not implemented
-* linux/386/pie skipped = 0.67% (1/149)
+* linux/386/pie skipped = 0.67% (1/150)
 	* 1 broken
-* pie skipped = 0.67% (1/149)
+* pie skipped = 0.67% (1/150)
 	* 1 upstream issue - https://github.com/golang/go/issues/29322
-* rr skipped = 1.3% (2/149)
+* rr skipped = 1.3% (2/150)
 	* 2 not implemented
-* windows skipped = 2.7% (4/149)
+* windows skipped = 2.7% (4/150)
 	* 1 broken
 	* 2 not implemented
 	* 1 upstream issue

pkg/proc/native/proc_linux.go

@@ -576,12 +576,28 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
 	switchTrapthread := false
 
 	// set breakpoints on SIGTRAP threads
+	var err1 error
 	for _, th := range dbp.threads {
-		if th.CurrentBreakpoint.Breakpoint == nil && th.os.setbp {
-			if err := th.SetCurrentBreakpoint(true); err != nil {
-				return nil, err
+		pc, _ := th.PC()
+
+		if !th.os.setbp && pc != th.os.phantomBreakpointPC {
+			// check if this could be a breakpoint hit anyway that the OS hasn't notified us about, yet.
+			if _, ok := dbp.FindBreakpoint(pc, dbp.BinInfo().Arch.BreakInstrMovesPC()); ok {
+				th.os.phantomBreakpointPC = pc
 			}
 		}
+
+		if pc != th.os.phantomBreakpointPC {
+			th.os.phantomBreakpointPC = 0
+		}
+
+		if th.CurrentBreakpoint.Breakpoint == nil && th.os.setbp {
+			if err := th.SetCurrentBreakpoint(true); err != nil {
+				err1 = err
+				continue
+			}
+		}
+
 		if th.CurrentBreakpoint.Breakpoint == nil && th.os.setbp && (th.Status != nil) && ((*sys.WaitStatus)(th.Status).StopSignal() == sys.SIGTRAP) && dbp.BinInfo().Arch.BreakInstrMovesPC() {
 			manualStop := false
 			if th.ThreadID() == trapthread.ThreadID() {
@@ -589,7 +605,7 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
 				manualStop = dbp.manualStopRequested
 				dbp.stopMu.Unlock()
 			}
-			if !manualStop {
+			if !manualStop && th.os.phantomBreakpointPC == pc {
 				// Thread received a SIGTRAP but we don't have a breakpoint for it and
 				// it wasn't sent by a manual stop request. It's either a hardcoded
 				// breakpoint or a phantom breakpoint hit (a breakpoint that was hit but
@@ -624,6 +640,9 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
 			}
 		}
 	}
+	if err1 != nil {
+		return nil, err1
+	}
 
 	if switchTrapthread {
 		trapthreadID := trapthread.ID

pkg/proc/native/threads_linux.go

@@ -13,9 +13,10 @@ type waitStatus sys.WaitStatus
 // osSpecificDetails hold Linux specific
 // process details.
 type osSpecificDetails struct {
-	delayedSignal int
-	running       bool
-	setbp         bool
+	delayedSignal       int
+	running             bool
+	setbp               bool
+	phantomBreakpointPC uint64
 }
 
 func (t *nativeThread) stop() (err error) {

pkg/proc/proc_test.go

@@ -5307,3 +5307,51 @@ func TestWatchpointCounts(t *testing.T) {
 		}
 	})
 }
+
+func TestManualStopWhileStopped(t *testing.T) {
+	// Checks that RequestManualStop sent to a stopped thread does not cause the target process to die.
+	withTestProcess("loopprog", t, func(p *proc.Target, fixture protest.Fixture) {
+		asyncCont := func(done chan struct{}) {
+			defer close(done)
+			err := p.Continue()
+			t.Logf("%v\n", err)
+			if err != nil {
+				panic(err)
+			}
+			for _, th := range p.ThreadList() {
+				if th.Breakpoint().Breakpoint != nil {
+					t.Logf("unexpected stop at breakpoint: %v", th.Breakpoint().Breakpoint)
+					panic("unexpected stop at breakpoint")
+				}
+			}
+		}
+
+		const (
+			repeatsSlow = 3
+			repeatsFast = 5
+		)
+
+		for i := 0; i < repeatsSlow; i++ {
+			t.Logf("Continue %d (slow)", i)
+			done := make(chan struct{})
+			go asyncCont(done)
+			time.Sleep(1 * time.Second)
+			p.RequestManualStop()
+			time.Sleep(1 * time.Second)
+			p.RequestManualStop()
+			time.Sleep(1 * time.Second)
+			<-done
+		}
+		for i := 0; i < repeatsFast; i++ {
+			t.Logf("Continue %d (fast)", i)
+			rch := make(chan struct{})
+			done := make(chan struct{})
+			p.ResumeNotify(rch)
+			go asyncCont(done)
+			<-rch
+			p.RequestManualStop()
+			p.RequestManualStop()
+			<-done
+		}
+	})
+}

service/api/types.go

@@ -414,6 +414,9 @@ const (
 	// SwitchGoroutine switches the debugger's current thread context to the thread running the specified goroutine
 	SwitchGoroutine = "switchGoroutine"
 	// Halt suspends the process.
+	// The effect of Halt while the target process is stopped, or in the
+	// process of stopping, is operating system and timing dependent. It will
+	// either have no effect or cause the following resume to stop immediately.
 	Halt = "halt"
 	// Call resumes process execution injecting a function call.
 	Call = "call"