From 0cfe53905247d7657d431ea756ab18428ddff1e8 Mon Sep 17 00:00:00 2001 From: Alex Brainman Date: Thu, 27 Jul 2017 03:51:44 +0900 Subject: [PATCH] proc/native: make sure debugged executable can be deleted on windows (#890) * proc/native: make sure debugged executable can be deleted on windows Delve opens debugged executable to read binary info it contains, but it never closes the file. Windows will not let you delete file that is opened. So close Process.bi in Process.postExit, and actually call Process.postExit from windows Process.Kill. Also Windows sends some debugging events (EXIT_PROCESS_DEBUG_EVENT event in particular) after Delve calls TerminateProcess. The events need to be consumed by debugger before debugged process will be released by Windows. So call Process.waitForDebugEvent after TerminateProcess in Process.Kill. Fixes #398 * cmd/dlv: make TestIssue398 pass on darwin * cmd/dlv: add comment for TestIssue398 * proc/native: wait for debuggee to exit before returning from windows Process.Kill * proc/native: close process handle before returning from windows killProcess * proc/native: remove not used Process.Process --- cmd/dlv/dlv_test.go | 61 +++++++++++++++++++++++++++++++++ pkg/proc/native/proc.go | 15 +++----- pkg/proc/native/proc_windows.go | 18 +++++++++- 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/cmd/dlv/dlv_test.go b/cmd/dlv/dlv_test.go index 471daba9..98d02abc 100644 --- a/cmd/dlv/dlv_test.go +++ b/cmd/dlv/dlv_test.go @@ -2,7 +2,10 @@ package main import ( "bufio" + "bytes" "flag" + "fmt" + "io/ioutil" "os" "os/exec" "path/filepath" @@ -96,3 +99,61 @@ func TestBuild(t *testing.T) { client.Detach(true) cmd.Wait() } + +func testIssue398(t *testing.T, dlvbin string, cmds []string) (stdout, stderr []byte) { + var stdoutBuf, stderrBuf bytes.Buffer + buildtestdir := filepath.Join(protest.FindFixturesDir(), "buildtest") + cmd := exec.Command(dlvbin, "debug") + cmd.Dir = buildtestdir + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + cmd.Stdout = &stdoutBuf + cmd.Stderr = &stderrBuf + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + for _, c := range cmds { + fmt.Fprintf(stdin, "%s\n", c) + } + // ignore "dlv debug" command error, it returns + // errors even after successful debug session. + cmd.Wait() + stdout, stderr = stdoutBuf.Bytes(), stderrBuf.Bytes() + + debugbin := filepath.Join(buildtestdir, "debug") + _, err = os.Stat(debugbin) + if err == nil { + t.Errorf("running %q: file %v was not deleted\nstdout is %q, stderr is %q", cmds, debugbin, stdout, stderr) + return + } + if !os.IsNotExist(err) { + t.Errorf("running %q: %v\nstdout is %q, stderr is %q", cmds, err, stdout, stderr) + return + } + return +} + +// TestIssue398 verifies that the debug executable is removed after exit. +func TestIssue398(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "TestIssue398") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + + dlvbin := filepath.Join(tmpdir, "dlv.exe") + out, err := exec.Command("go", "build", "-o", dlvbin, "github.com/derekparker/delve/cmd/dlv").CombinedOutput() + if err != nil { + t.Fatalf("go build -o %v github.com/derekparker/delve/cmd/dlv: %v\n%s", dlvbin, err, string(out)) + } + + testIssue398(t, dlvbin, []string{"exit"}) + + const hello = "hello world!" + stdout, _ := testIssue398(t, dlvbin, []string{"continue", "exit"}) + if !strings.Contains(string(stdout), hello) { + t.Errorf("stdout %q should contain %q", stdout, hello) + } +} diff --git a/pkg/proc/native/proc.go b/pkg/proc/native/proc.go index b9fa14a2..39428b75 100644 --- a/pkg/proc/native/proc.go +++ b/pkg/proc/native/proc.go @@ -3,7 +3,6 @@ package native import ( "fmt" "go/ast" - "os" "runtime" "sync" @@ -13,9 +12,8 @@ import ( // Process represents all of the information the debugger // is holding onto regarding the process we are debugging. type Process struct { - bi proc.BinaryInfo - pid int // Process Pid - Process *os.Process // Pointer to process struct for the actual process we are debugging + bi proc.BinaryInfo + pid int // Process Pid // Breakpoint table, holds information on breakpoints. // Maps instruction address to Breakpoint struct. @@ -388,13 +386,7 @@ func (dbp *Process) FindBreakpoint(pc uint64) (*proc.Breakpoint, bool) { // Returns a new Process struct. func initializeDebugProcess(dbp *Process, path string) (*Process, error) { - process, err := os.FindProcess(dbp.pid) - if err != nil { - return nil, err - } - - dbp.Process = process - err = dbp.LoadInformation(path) + err := dbp.LoadInformation(path) if err != nil { return nil, err } @@ -461,6 +453,7 @@ func (dbp *Process) postExit() { dbp.exited = true close(dbp.ptraceChan) close(dbp.ptraceDoneChan) + dbp.bi.Close() } func (dbp *Process) writeSoftwareBreakpoint(thread *Thread, addr uint64) error { diff --git a/pkg/proc/native/proc_windows.go b/pkg/proc/native/proc_windows.go index b98bac2e..9440e8e2 100644 --- a/pkg/proc/native/proc_windows.go +++ b/pkg/proc/native/proc_windows.go @@ -173,11 +173,25 @@ func (dbp *Process) Kill() error { if !dbp.threads[dbp.pid].Stopped() { return errors.New("process must be stopped in order to kill it") } + + p, err := os.FindProcess(dbp.pid) + if err != nil { + return err + } + defer p.Release() + // TODO: Should not have to ignore failures here, // but some tests appear to Kill twice causing // this to fail on second attempt. _ = syscall.TerminateProcess(dbp.os.hProcess, 1) - dbp.exited = true + + dbp.execPtraceFunc(func() { + dbp.waitForDebugEvent(waitBlocking) + }) + + p.Wait() + + dbp.postExit() return nil } @@ -459,5 +473,7 @@ func killProcess(pid int) error { if err != nil { return err } + defer p.Release() + return p.Kill() }