[tor-commits] [meek/master] Be more careful about terminating meek-client.

[dcf at torproject.org]

commit adf41cd3adf6e3884626b6b7e4c7f131660343dd
Author: David Fifield <david at bamsoftware.com>
Date:   Sat Feb 23 02:23:05 2019 -0700

    Be more careful about terminating meek-client.
    
    First close its stdin and send SIGTERM; and then kill it if that doesn't
    work. Set TOR_PT_EXIT_ON_STDIN_EOF=1 unconditionally in the subprocess.
    On Windows it's the same, except don't send SIGTERM.
    
    https://bugs.torproject.org/15125
    https://bugs.torproject.org/25613
---
 meek-client-torbrowser/meek-client-torbrowser.go | 27 ++++++++++++++-----
 meek-client-torbrowser/terminate_other.go        | 33 ++++++++++++++++++++++++
 meek-client-torbrowser/terminate_windows.go      | 30 +++++++++++++++++++++
 3 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/meek-client-torbrowser/meek-client-torbrowser.go b/meek-client-torbrowser/meek-client-torbrowser.go
index ce79193..0f295ff 100644
--- a/meek-client-torbrowser/meek-client-torbrowser.go
+++ b/meek-client-torbrowser/meek-client-torbrowser.go
@@ -35,16 +35,27 @@ import (
 	"regexp"
 	"strings"
 	"syscall"
+	"time"
 )
 
 // This magic string is emitted by meek-http-helper.
 var helperAddrPattern = regexp.MustCompile(`^meek-http-helper: listen (127\.0\.0\.1:\d+)$`)
 
+// How long to wait for child processes to exit gracefully before killing them.
+const terminateTimeout = 2 * time.Second
+
 func usage() {
 	fmt.Fprintf(os.Stderr, "Usage: %s [meek-client-torbrowser args] -- meek-client [meek-client args]\n", os.Args[0])
 	flag.PrintDefaults()
 }
 
+// ptCmd is a *exec.Cmd augmented with an io.WriteCloser for its stdin, which we
+// can close to instruct the PT subprocess to terminate.
+type ptCmd struct {
+	*exec.Cmd
+	StdinCloser io.WriteCloser
+}
+
 // Log a call to os.Process.Kill.
 func logKill(p *os.Process) error {
 	log.Printf("killing PID %d", p.Pid)
@@ -293,14 +304,16 @@ func grepHelperAddr(r io.Reader) (string, error) {
 }
 
 // Run meek-client and return its exec.Cmd.
-func runMeekClient(helperAddr string, meekClientCommandLine []string) (cmd *exec.Cmd, err error) {
+func runMeekClient(helperAddr string, meekClientCommandLine []string) (cmd *ptCmd, err error) {
 	meekClientPath := meekClientCommandLine[0]
 	args := meekClientCommandLine[1:]
 	args = append(args, []string{"--helper", helperAddr}...)
-	cmd = exec.Command(meekClientPath, args...)
+	cmd = new(ptCmd)
+	cmd.Cmd = exec.Command(meekClientPath, args...)
 	// Give the subprocess a stdin for TOR_PT_EXIT_ON_STDIN_CLOSE purposes.
 	// https://bugs.torproject.org/24642
-	_, err = cmd.StdinPipe()
+	cmd.Env = append(os.Environ(), "TOR_PT_EXIT_ON_STDIN_CLOSE=1")
+	cmd.StdinCloser, err = cmd.StdinPipe()
 	if err != nil {
 		return
 	}
@@ -320,7 +333,7 @@ func runMeekClient(helperAddr string, meekClientCommandLine []string) (cmd *exec
 // may have failed to start. If a process did not start, its corresponding
 // return value will be nil. The caller is responsible for terminating whatever
 // processes were started, whether or not err is nil.
-func startProcesses(sigChan <-chan os.Signal, meekClientCommandLine []string) (firefoxCmd *exec.Cmd, meekClientCmd *exec.Cmd, err error) {
+func startProcesses(sigChan <-chan os.Signal, meekClientCommandLine []string) (firefoxCmd *exec.Cmd, meekClientCmd *ptCmd, err error) {
 	// Start firefox.
 	var stdout io.Reader
 	firefoxCmd, stdout, err = runFirefox()
@@ -414,7 +427,6 @@ func main() {
 		// we are instructed to stop.
 		sig := <-sigChan
 		log.Printf("sig %s", sig)
-		logSignal(meekClientCmd.Process, sig)
 	} else {
 		// Otherwise don't wait, go ahead and terminate whatever
 		// processes were started.
@@ -425,6 +437,9 @@ func main() {
 		logKill(firefoxCmd.Process)
 	}
 	if meekClientCmd != nil {
-		logKill(firefoxCmd.Process)
+		err := terminatePTCmd(meekClientCmd)
+		if err != nil {
+			log.Printf("error terminating meek-client: %v", err)
+		}
 	}
 }
diff --git a/meek-client-torbrowser/terminate_other.go b/meek-client-torbrowser/terminate_other.go
new file mode 100644
index 0000000..d38e27b
--- /dev/null
+++ b/meek-client-torbrowser/terminate_other.go
@@ -0,0 +1,33 @@
+// +build !windows
+
+// Process termination code for platforms that have SIGTERM (i.e., not Windows).
+
+package main
+
+import (
+	"syscall"
+	"time"
+)
+
+// Terminate a PT subprocess: first close its stdin and send it SIGTERM; then
+// kill it if that doesn't work.
+func terminatePTCmd(cmd *ptCmd) error {
+	err := cmd.StdinCloser.Close()
+	err2 := cmd.Process.Signal(syscall.SIGTERM)
+	if err == nil {
+		err = err2
+	}
+	ch := make(chan error, 1)
+	go func() {
+		ch <- cmd.Wait()
+	}()
+	select {
+	case <-time.After(terminateTimeout):
+		err2 = cmd.Process.Kill()
+	case err2 = <-ch:
+	}
+	if err == nil {
+		err = err2
+	}
+	return err
+}
diff --git a/meek-client-torbrowser/terminate_windows.go b/meek-client-torbrowser/terminate_windows.go
new file mode 100644
index 0000000..c00c44f
--- /dev/null
+++ b/meek-client-torbrowser/terminate_windows.go
@@ -0,0 +1,30 @@
+// +build windows
+
+// Process termination code for platforms that don't have SIGTERM (i.e.,
+// Windows).
+
+package main
+
+import (
+	"time"
+)
+
+// Terminate a PT subprocess: first close its stdin; then kill it if that
+// doesn't work.
+func terminatePTCmd(cmd *ptCmd) error {
+	err := cmd.StdinCloser.Close()
+	ch := make(chan error, 1)
+	go func() {
+		ch <- cmd.Wait()
+	}()
+	var err2 error
+	select {
+	case <-time.After(terminateTimeout):
+		err2 = cmd.Process.Kill()
+	case err2 = <-ch:
+	}
+	if err == nil {
+		err = err2
+	}
+	return err
+}