proc: fix signal handling during stepping (#2803)

Fix signal handling during thread single stepping so that signals that
are generated by executing the current instruction are immediately
propagated to the inferior, while signals other signals sent to the
thread are delayed until the full resume happens.

Fixes a bug where a breakpoint set on an instruction that causes a
SIGSEGV would make Delve hang and a bug where signals received during
single step would make it look like an instruction is executed twice.

Fixes #2801
Fixes #2792

_fixtures/asmnilptr/main.go

@@ -0,0 +1,9 @@
+package main
+
+import "fmt"
+
+func asmFunc(*int) int
+
+func main() {
+	fmt.Printf("%d\n", asmFunc(nil))
+}

_fixtures/asmnilptr/main_386.s

@@ -0,0 +1,7 @@
+#include "textflag.h"
+
+TEXT ·asmFunc(SB),0,$0-16
+	MOVL arg+0(FP), AX
+	MOVL (AX), AX
+	MOVL AX, ret+4(FP)
+	RET

_fixtures/asmnilptr/main_amd64.s

@@ -0,0 +1,7 @@
+#include "textflag.h"
+
+TEXT ·asmFunc(SB),0,$0-16
+	MOVQ arg+0(FP), AX
+	MOVQ (AX), AX
+	MOVQ AX, ret+8(FP)
+	RET

_fixtures/asmnilptr/main_arm64.s

@@ -0,0 +1,7 @@
+#include "textflag.h"
+
+TEXT ·asmFunc(SB),0,$0-16
+	MOVD arg+0(FP), R5
+	MOVD (R5), R5
+	MOVD R5, ret+8(FP)
+	RET

pkg/proc/gdbserial/gdbserver.go

@@ -232,6 +232,7 @@ func newProcess(process *os.Process) *gdbProcess {
 			direction:           proc.Forward,
 			log:                 logger,
 			goarch:              runtime.GOARCH,
+			goos:                runtime.GOOS,
 		},
 		threads:        make(map[int]*gdbThread),
 		bi:             proc.NewBinaryInfo(runtime.GOOS, runtime.GOARCH),
@@ -790,6 +791,9 @@ const (
 	childSignal      = 0x11
 	stopSignal       = 0x13
 
+	_SIGILL = 0x4 // ok
+	_SIGFPE = 0x8 // ok
+
 	debugServerTargetExcBadAccess      = 0x91
 	debugServerTargetExcBadInstruction = 0x92
 	debugServerTargetExcArithmetic     = 0x93
@@ -880,21 +884,7 @@ continueLoop:
 		return nil, stopReason, fmt.Errorf("could not find thread %s", threadID)
 	}
 
-	var err error
-	switch trapthread.sig {
-	case 0x91:
-		err = errors.New("bad access")
-	case 0x92:
-		err = errors.New("bad instruction")
-	case 0x93:
-		err = errors.New("arithmetic exception")
-	case 0x94:
-		err = errors.New("emulation exception")
-	case 0x95:
-		err = errors.New("software exception")
-	case 0x96:
-		err = errors.New("breakpoint exception")
-	}
+	err := machTargetExcToError(trapthread.sig)
 	if err != nil {
 		// the signals that are reported here can not be propagated back to the target process.
 		trapthread.sig = 0
@@ -1522,7 +1512,7 @@ func (t *gdbThread) StepInstruction() error {
 	// Reset thread registers so the next call to
 	// Thread.Registers will not be cached.
 	t.regs.regs = nil
-	return t.p.conn.step(t.strID, &threadUpdater{p: t.p}, false)
+	return t.p.conn.step(t, &threadUpdater{p: t.p}, false)
 }
 
 // Blocked returns true if the thread is blocked in runtime or kernel code.
@@ -1784,7 +1774,7 @@ func (t *gdbThread) reloadGAtPC() error {
 		}
 	}()
 
-	err = t.p.conn.step(t.strID, nil, true)
+	err = t.p.conn.step(t, nil, true)
 	if err != nil {
 		if err == errThreadBlocked {
 			t.regs.tls = 0
@@ -1837,7 +1827,7 @@ func (t *gdbThread) reloadGAlloc() error {
 		}
 	}()
 
-	err = t.p.conn.step(t.strID, nil, true)
+	err = t.p.conn.step(t, nil, true)
 	if err != nil {
 		if err == errThreadBlocked {
 			t.regs.tls = 0
@@ -2056,3 +2046,21 @@ func registerName(arch *proc.Arch, regNum uint64) string {
 	regName, _, _ := arch.DwarfRegisterToString(int(regNum), nil)
 	return strings.ToLower(regName)
 }
+
+func machTargetExcToError(sig uint8) error {
+	switch sig {
+	case 0x91:
+		return errors.New("bad access")
+	case 0x92:
+		return errors.New("bad instruction")
+	case 0x93:
+		return errors.New("arithmetic exception")
+	case 0x94:
+		return errors.New("emulation exception")
+	case 0x95:
+		return errors.New("software exception")
+	case 0x96:
+		return errors.New("breakpoint exception")
+	}
+	return nil
+}

pkg/proc/gdbserial/gdbserver_conn.go

@@ -48,6 +48,7 @@ type gdbConn struct {
 	isDebugserver         bool // true if the stub is debugserver
 	xcmdok                bool // x command can be used to transfer memory
 	goarch                string
+	goos                  string
 
 	useXcmd bool // forces writeMemory to use the 'X' command
 
@@ -603,7 +604,8 @@ func (conn *gdbConn) resume(threads map[int]*gdbThread, tu *threadUpdater) (stop
 }
 
 // step executes a 'vCont' command on the specified thread with 's' action.
-func (conn *gdbConn) step(threadID string, tu *threadUpdater, ignoreFaultSignal bool) error {
+func (conn *gdbConn) step(th *gdbThread, tu *threadUpdater, ignoreFaultSignal bool) error {
+	threadID := th.strID
 	if conn.direction != proc.Forward {
 		if err := conn.selectThread('c', threadID, "step"); err != nil {
 			return err
@@ -616,6 +618,17 @@ func (conn *gdbConn) step(threadID string, tu *threadUpdater, ignoreFaultSignal
 		_, err := conn.waitForvContStop("singlestep", threadID, tu)
 		return err
 	}
+
+	var _SIGBUS uint8
+	switch conn.goos {
+	case "linux":
+		_SIGBUS = 0x7
+	case "darwin":
+		_SIGBUS = 0xa
+	default:
+		panic(fmt.Errorf("unknown GOOS %s", conn.goos))
+	}
+
 	var sig uint8 = 0
 	for {
 		conn.outbuf.Reset()
@@ -640,6 +653,8 @@ func (conn *gdbConn) step(threadID string, tu *threadUpdater, ignoreFaultSignal
 			if ignoreFaultSignal { // we attempting to read the TLS, a fault here should be ignored
 				return nil
 			}
+		case _SIGILL, _SIGBUS, _SIGFPE:
+			// propagate these signals to inferior immediately
 		case interruptSignal, breakpointSignal, stopSignal:
 			return nil
 		case childSignal: // stop on debugserver but SIGCHLD on lldb-server/linux
@@ -647,9 +662,15 @@ func (conn *gdbConn) step(threadID string, tu *threadUpdater, ignoreFaultSignal
 				return nil
 			}
 		case debugServerTargetExcBadAccess, debugServerTargetExcBadInstruction, debugServerTargetExcArithmetic, debugServerTargetExcEmulation, debugServerTargetExcSoftware, debugServerTargetExcBreakpoint:
-			return nil
+			if ignoreFaultSignal {
+				return nil
+			}
+			return machTargetExcToError(sig)
+		default:
+			// delay propagation of any other signal to until after the stepping is done
+			th.sig = sig
+			sig = 0
 		}
-		// any other signal is propagated to the inferior
 	}
 }
 

pkg/proc/native/ptrace_linux.go

@@ -24,3 +24,12 @@ func ptraceDetach(tid, sig int) error {
 func ptraceCont(tid, sig int) error {
 	return sys.PtraceCont(tid, sig)
 }
+
+// ptraceSingleStep executes ptrace PTRACE_SINGLESTEP
+func ptraceSingleStep(pid, sig int) error {
+	_, _, e1 := sys.Syscall6(sys.SYS_PTRACE, uintptr(sys.PTRACE_SINGLESTEP), uintptr(pid), uintptr(0), uintptr(sig), 0, 0)
+	if e1 != 0 {
+		return e1
+	}
+	return nil
+}

pkg/proc/native/threads_linux.go

@@ -48,8 +48,10 @@ func (t *nativeThread) resumeWithSig(sig int) (err error) {
 }
 
 func (t *nativeThread) singleStep() (err error) {
+	sig := 0
 	for {
-		t.dbp.execPtraceFunc(func() { err = sys.PtraceSingleStep(t.ID) })
+		t.dbp.execPtraceFunc(func() { err = ptraceSingleStep(t.ID, sig) })
+		sig = 0
 		if err != nil {
 			return err
 		}
@@ -65,8 +67,19 @@ func (t *nativeThread) singleStep() (err error) {
 			}
 			return proc.ErrProcessExited{Pid: t.dbp.pid, Status: rs}
 		}
-		if wpid == t.ID && status.StopSignal() == sys.SIGTRAP {
-			return nil
+		if wpid == t.ID {
+			switch s := status.StopSignal(); s {
+			case sys.SIGTRAP:
+				return nil
+			case sys.SIGSTOP:
+				// delayed SIGSTOP, ignore it
+			case sys.SIGILL, sys.SIGBUS, sys.SIGFPE, sys.SIGSEGV, sys.SIGSTKFLT:
+				// propagate signals that can have been caused by the current instruction
+				sig = int(s)
+			default:
+				// delay propagation of all other signals
+				t.os.delayedSignal = int(s)
+			}
 		}
 	}
 }

pkg/proc/proc_test.go

@@ -5736,3 +5736,42 @@ func TestSetYMMRegister(t *testing.T) {
 		}
 	})
 }
+
+func TestNilPtrDerefInBreakInstr(t *testing.T) {
+	// Checks that having a breakpoint on the exact instruction that causes a
+	// nil pointer dereference does not cause problems.
+
+	var asmfile string
+	switch runtime.GOARCH {
+	case "amd64":
+		asmfile = "main_amd64.s"
+	case "arm64":
+		asmfile = "main_arm64.s"
+	case "386":
+		asmfile = "main_386.s"
+	default:
+		t.Fatalf("assembly file for %s not provided", runtime.GOARCH)
+	}
+
+	withTestProcess("asmnilptr/", t, func(p *proc.Target, fixture protest.Fixture) {
+		f := filepath.Join(fixture.BuildDir, asmfile)
+		f = strings.Replace(f, "\\", "/", -1)
+		setFileBreakpoint(p, t, f, 5)
+		t.Logf("first continue")
+		assertNoError(p.Continue(), t, "Continue()")
+		t.Logf("second continue")
+		err := p.Continue()
+		if runtime.GOOS == "darwin" && err != nil && err.Error() == "bad access" {
+			// this is also ok
+			return
+		}
+		assertNoError(err, t, "Continue()")
+		bp := p.CurrentThread().Breakpoint()
+		if bp != nil {
+			t.Logf("%#v\n", bp.Breakpoint)
+		}
+		if bp == nil || (bp.Name != proc.UnrecoveredPanic) {
+			t.Fatalf("no breakpoint hit or wrong breakpoint hit: %#v", bp)
+		}
+	})
+}