From bd2a4fe56e9a584a2ac14a43a294d5db1ee5abc3 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Mon, 17 May 2021 18:48:48 +0200 Subject: [PATCH] proc/native/linux: better handling of process death due to signals (#2477) Handle the signaled status for the thread leader like we handle the exited status, by returning ErrProcessExited and recording the killer signal in it. Prior to this commit we would find out about the death of the thread later in the loop, the condition would still be reported as ErrProcessExited, but without recording the signal number anywhere. Also fixes a bug in TestAttachStopOnEntry where the test would inadvertently cause a SIGPIPE to be sent to the target process, making it terminate early. --- pkg/proc/native/proc_linux.go | 10 +++++++ pkg/proc/proc_unix_test.go | 34 +++++++++++++++++++++++ service/dap/server_test.go | 52 ++++++++++++++--------------------- 3 files changed, 65 insertions(+), 31 deletions(-) diff --git a/pkg/proc/native/proc_linux.go b/pkg/proc/native/proc_linux.go index 652990f1..8b61cb21 100644 --- a/pkg/proc/native/proc_linux.go +++ b/pkg/proc/native/proc_linux.go @@ -342,6 +342,16 @@ func (dbp *nativeProcess) trapWaitInternal(pid int, options trapWaitOptions) (*n delete(dbp.threads, wpid) continue } + if status.Signaled() { + // Signaled means the thread was terminated due to a signal. + if wpid == dbp.pid { + dbp.postExit() + return nil, proc.ErrProcessExited{Pid: wpid, Status: -int(status.Signal())} + } + // does this ever happen? + delete(dbp.threads, wpid) + continue + } if status.StopSignal() == sys.SIGTRAP && status.TrapCause() == sys.PTRACE_EVENT_CLONE { // A traced thread has cloned a new thread, grab the pid and // add it to our list of traced threads. diff --git a/pkg/proc/proc_unix_test.go b/pkg/proc/proc_unix_test.go index 40034920..5d7e2f85 100644 --- a/pkg/proc/proc_unix_test.go +++ b/pkg/proc/proc_unix_test.go @@ -4,11 +4,17 @@ package proc_test import ( "fmt" + "os" + "os/exec" + "runtime" "syscall" "testing" "time" + "golang.org/x/sys/unix" + "github.com/go-delve/delve/pkg/proc" + "github.com/go-delve/delve/pkg/proc/native" protest "github.com/go-delve/delve/pkg/proc/test" ) @@ -66,3 +72,31 @@ func TestIssue419(t *testing.T) { } } } + +func TestSignalDeath(t *testing.T) { + if testBackend != "native" || runtime.GOOS != "linux" { + t.Skip("skipped on non-linux non-native backends") + } + var buildFlags protest.BuildFlags + if buildMode == "pie" { + buildFlags |= protest.BuildModePIE + } + fixture := protest.BuildFixture("loopprog", buildFlags) + cmd := exec.Command(fixture.Path) + stdout, err := cmd.StdoutPipe() + assertNoError(err, t, "StdoutPipe") + cmd.Stderr = os.Stderr + assertNoError(cmd.Start(), t, "starting fixture") + p, err := native.Attach(cmd.Process.Pid, []string{}) + assertNoError(err, t, "Attach") + stdout.Close() // target will receive SIGPIPE later on + err = p.Continue() + t.Logf("error is %v", err) + exitErr, isexited := err.(proc.ErrProcessExited) + if !isexited { + t.Fatal("did not exit") + } + if exitErr.Status != -int(unix.SIGPIPE) { + t.Fatalf("expected SIGPIPE got %d\n", exitErr.Status) + } +} diff --git a/service/dap/server_test.go b/service/dap/server_test.go index a1c354a9..64a2ed75 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -368,39 +368,21 @@ func TestAttachStopOnEntry(t *testing.T) { // 13 >> disconnect, << disconnect client.DisconnectRequestWithKillOption(true) - // Both of these scenarios are somehow possible. - // Even though the program has an infininte loop, - // it apears that a halt can cause it to terminate. - // Since we are in async mode while running, we might receive messages in either order. - msg := client.ExpectMessage(t) - switch m := msg.(type) { - case *dap.StoppedEvent: - if m.Seq != 0 || m.Body.Reason != "pause" { // continue is interrupted - t.Errorf("\ngot %#v\nwant Seq=0 Reason='pause'", m) - } - oed := client.ExpectOutputEventDetachingKill(t) - if oed.Seq != 0 || oed.Body.Category != "console" { - t.Errorf("\ngot %#v\nwant Seq=0 Category='console'", oed) - } - case *dap.TerminatedEvent: - if m.Seq != 0 { - t.Errorf("\ngot %#v\nwant Seq=0'", m) - } - oep := client.ExpectOutputEventProcessExited(t, 0) - if oep.Seq != 0 || oep.Body.Category != "console" { - t.Errorf("\ngot %#v\nwant Seq=0 Category='console'", oep) - } - oed := client.ExpectOutputEventDetaching(t) - if oed.Seq != 0 || oed.Body.Category != "console" { - t.Errorf("\ngot %#v\nwant Seq=0 Category='console'", oed) - } - default: - t.Fatalf("got %#v, want StoppedEvent or TerminatedEvent", m) + msg := expectMessageFilterStopped(t, client) + if _, ok := msg.(*dap.OutputEvent); !ok { + // want detach kill output message + t.Errorf("got %#v, want *dap.OutputEvent", msg) } - dResp := client.ExpectDisconnectResponse(t) - if dResp.Seq != 0 || dResp.RequestSeq != 13 { - t.Errorf("\ngot %#v\nwant Seq=0, RequestSeq=13", dResp) + msg = expectMessageFilterStopped(t, client) + if _, ok := msg.(*dap.DisconnectResponse); !ok { + t.Errorf("got %#v, want *dap.DisconnectResponse", msg) } + // If this call to KeepAlive isn't here there's a chance that stdout will + // be garbage collected (since it is no longer alive long before this + // point), when that happens, on unix-like OSes, the read end of the pipe + // will be closed by the finalizer and the target process will die by + // SIGPIPE, which the rest of this test does not expect. + runtime.KeepAlive(stdout) }) } @@ -741,6 +723,14 @@ func expectVarRegex(t *testing.T, got *dap.VariablesResponse, i int, name, evalN return expectVar(t, got, i, name, evalName, value, typ, false, hasRef) } +func expectMessageFilterStopped(t *testing.T, client *daptest.Client) dap.Message { + msg := client.ExpectMessage(t) + if _, isStopped := msg.(*dap.StoppedEvent); isStopped { + msg = client.ExpectMessage(t) + } + return msg +} + // validateEvaluateName issues an evaluate request with evaluateName of a variable and // confirms that it succeeds and returns the same variable record as the original. func validateEvaluateName(t *testing.T, client *daptest.Client, got *dap.VariablesResponse, i int) {