commit d34355a097590c900217acc397ad6a383af04267 Author: Yawning Angel yawning@schwanenlied.me Date: Tue Jan 10 07:42:11 2017 +0000

Bug 21184: Do a better job of killing/cleaning up bwrap children.

Use the bwrap fork that's acting as init to kill, wait on the main bwrap process returned from os/exec when waiting/polling process status.

This can be further improved, but would require bubblewrap patches to make substantially better, and at least the cleanup/refactor lays the groundwork for everything. --- ChangeLog | 3 +- .../internal/sandbox/application.go | 6 +- .../internal/sandbox/hugbox.go | 30 ++++---- .../internal/sandbox/process/process.go | 84 ++++++++++++++++++++++ src/cmd/sandboxed-tor-browser/internal/tor/tor.go | 71 ++++++++++-------- .../sandboxed-tor-browser/internal/ui/gtk/ui.go | 7 +- src/cmd/sandboxed-tor-browser/internal/ui/ui.go | 8 +-- 7 files changed, 153 insertions(+), 56 deletions(-)

diff --git a/ChangeLog b/ChangeLog index 8ca4df4..d7693bd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ Changes in version 0.0.3 - UNRELEASED: - * Bug 21903: Go back to using gosecco for seccomp rule compilation. + * Bug 21184: Do a better job of killing/cleaning up bwrap children. + * Bug 21093: Go back to using gosecco for seccomp rule compilation. * Bug 20940: Deprecate x86 support. * Bug 20778: Check for updates in the background. * Bug 20851: If the incremental update fails, fall back to the complete diff --git a/src/cmd/sandboxed-tor-browser/internal/sandbox/application.go b/src/cmd/sandboxed-tor-browser/internal/sandbox/application.go index 8d9eca9..fa773d4 100644 --- a/src/cmd/sandboxed-tor-browser/internal/sandbox/application.go +++ b/src/cmd/sandboxed-tor-browser/internal/sandbox/application.go @@ -25,7 +25,6 @@ import ( "io/ioutil" "log" "os" - "os/exec" "path/filepath" "runtime" "sort" @@ -33,6 +32,7 @@ import ( "syscall"

"cmd/sandboxed-tor-browser/internal/dynlib" + . "cmd/sandboxed-tor-browser/internal/sandbox/process" "cmd/sandboxed-tor-browser/internal/tor" "cmd/sandboxed-tor-browser/internal/ui/config" . "cmd/sandboxed-tor-browser/internal/utils" @@ -46,7 +46,7 @@ var ( )

// RunTorBrowser launches sandboxed Tor Browser. -func RunTorBrowser(cfg *config.Config, manif *config.Manifest, tor *tor.Tor) (cmd *exec.Cmd, err error) { +func RunTorBrowser(cfg *config.Config, manif *config.Manifest, tor *tor.Tor) (process *Process, err error) { const ( profileSubDir = "TorBrowser/Data/Browser/profile.default" cachesSubDir = "TorBrowser/Data/Browser/Caches" @@ -492,7 +492,7 @@ func stageUpdate(updateDir, installDir string, mar []byte) error { }

// RunTor launches sandboxeed Tor. -func RunTor(cfg *config.Config, manif *config.Manifest, torrc []byte) (cmd *exec.Cmd, err error) { +func RunTor(cfg *config.Config, manif *config.Manifest, torrc []byte) (process *Process, err error) { defer func() { if r := recover(); r != nil { err = fmt.Errorf("%v", r) diff --git a/src/cmd/sandboxed-tor-browser/internal/sandbox/hugbox.go b/src/cmd/sandboxed-tor-browser/internal/sandbox/hugbox.go index 260be34..9d2bba4 100644 --- a/src/cmd/sandboxed-tor-browser/internal/sandbox/hugbox.go +++ b/src/cmd/sandboxed-tor-browser/internal/sandbox/hugbox.go @@ -31,6 +31,7 @@ import ( "time"

"cmd/sandboxed-tor-browser/internal/data" + . "cmd/sandboxed-tor-browser/internal/sandbox/process" . "cmd/sandboxed-tor-browser/internal/utils" )

@@ -54,6 +55,10 @@ func (u *unshareOpts) toArgs() []string { } if u.pid { args = append(args, "--unshare-pid") + } else { + // Until bubblewrap > 0.1.5 when the child calls setsid(), + // we have to rely on SIGKILL-ing the init fork for cleanup. + panic("sandbox: unshare.pid is required") } if u.net { args = append(args, "--unshare-net") @@ -152,7 +157,7 @@ func (h *hugbox) assetFile(dest, asset string) { h.file(dest, b) }

-func (h *hugbox) run() (*exec.Cmd, error) { +func (h *hugbox) run() (*Process, error) { // Create the command struct for the sandbox. cmd := &exec.Cmd{ Path: h.bwrapPath, @@ -298,10 +303,11 @@ func (h *hugbox) run() (*exec.Cmd, error) { // Do the rest of the setup in a go routine, and monitor completion and // a watchdog timer. doneCh := make(chan error) - bwrapPid := cmd.Process.Pid hz := time.NewTicker(1 * time.Second) defer hz.Stop()

+ process := NewProcess(cmd) + go func() { // Flush the pending writes. for i, wrFd := range pendingWriteFds { @@ -330,7 +336,7 @@ func (h *hugbox) run() (*exec.Cmd, error) { panic("sandbox: seccomp fd exists when there are no rules to be written") }

- // Read back the child pid. + // Read back the init child pid. decoder := json.NewDecoder(infoRdFd) info := &bwrapInfo{} if err := decoder.Decode(info); err != nil { @@ -339,11 +345,11 @@ func (h *hugbox) run() (*exec.Cmd, error) { }

Debugf("sandbox: bwrap pid is: %v", cmd.Process.Pid) - Debugf("sandbox: child pid is: %v", info.Pid) + Debugf("sandbox: bwrap init pid is: %v", info.Pid)

- // This is more useful to us, since it's the bubblewrap child inside - // the container. - cmd.Process.Pid = info.Pid + // Sending a SIGKILL to this will terminate every process in the PID + // namespace. If people aren't using unshare.pid, bad things happen. + process.SetInitPid(info.Pid)

doneCh <- nil }() @@ -354,15 +360,11 @@ timeoutLoop: select { case err = <-doneCh: if err == nil { - return cmd, nil + return process, nil } break timeoutLoop case <-hz.C: - var wstatus syscall.WaitStatus - _, err = syscall.Wait4(bwrapPid, &wstatus, syscall.WNOHANG, nil) - if err != nil { - break timeoutLoop - } else if wstatus.Exited() { + if !process.Running() { err = fmt.Errorf("sandbox: bubblewrap exited unexpectedly") break timeoutLoop } @@ -370,7 +372,7 @@ timeoutLoop: } }

- cmd.Process.Kill() + process.Kill() return nil, err }

diff --git a/src/cmd/sandboxed-tor-browser/internal/sandbox/process/process.go b/src/cmd/sandboxed-tor-browser/internal/sandbox/process/process.go new file mode 100644 index 0000000..61ab344 --- /dev/null +++ b/src/cmd/sandboxed-tor-browser/internal/sandbox/process/process.go @@ -0,0 +1,84 @@ +// process.go - Sandboxed process. +// Copyright (C) 2017 Yawning Angel. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as +// published by the Free Software Foundation, either version 3 of the +// License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see http://www.gnu.org/licenses/. + +// Package process contains a wrapper around a running bwrap instance, and is +// in a separate package just to break an import loop. +package process + +import ( + "os" + "os/exec" + "syscall" +) + +// Process is a running bwrap instance. +type Process struct { + init *os.Process + cmd *exec.Cmd +} + +// Kill terminates the bwrap instance and all of it's children. +func (p *Process) Kill() { + if p.init != nil { + p.init.Kill() + p.init = nil + } + if p.cmd != nil { + p.cmd.Process.Kill() + p.cmd.Process.Wait() + p.cmd = nil + } +} + +// Wait waits for the bwrap instance to complete. +func (p *Process) Wait() error { + // Can't wait on the init process since it's a grandchild. + if p.cmd != nil { + p.cmd.Process.Wait() + p.cmd = nil + } + return nil +} + +// Running returns true if the bwrap instance is running. +func (p *Process) Running() bool { + wpid, err := syscall.Wait4(p.cmd.Process.Pid, nil, syscall.WNOHANG, nil) + if err != nil { + return false + } + return wpid == 0 +} + +// SetInitPid sets the pid of the bwrap init fork. This should not be called +// except from the sandbox creation routine. +func (p *Process) SetInitPid(pid int) { + if p.init != nil { + panic("process: SetInitPid called when already set") + } + + proc, err := os.FindProcess(pid) + if err != nil { + panic("process: SetInitPid on invalid process:" + err.Error()) + } + p.init = proc +} + +// NewProcess creates a new Process instance from a Cmd. +func NewProcess(cmd *exec.Cmd) *Process { + process := new(Process) + process.cmd = cmd + return process +} diff --git a/src/cmd/sandboxed-tor-browser/internal/tor/tor.go b/src/cmd/sandboxed-tor-browser/internal/tor/tor.go index 0de3b6d..69d9307 100644 --- a/src/cmd/sandboxed-tor-browser/internal/tor/tor.go +++ b/src/cmd/sandboxed-tor-browser/internal/tor/tor.go @@ -27,12 +27,10 @@ import ( "log" mrand "math/rand" "os" - "os/exec" "path/filepath" "strconv" "strings" "sync" - "syscall" "time" "unicode"

@@ -41,6 +39,7 @@ import ( "golang.org/x/net/proxy"

"cmd/sandboxed-tor-browser/internal/data" + "cmd/sandboxed-tor-browser/internal/sandbox/process" . "cmd/sandboxed-tor-browser/internal/ui/async" "cmd/sandboxed-tor-browser/internal/ui/config" . "cmd/sandboxed-tor-browser/internal/utils" @@ -56,7 +55,7 @@ type Tor struct { isSystem bool isBootstrapped bool

- cmd *exec.Cmd + process *process.Process ctrl *bulb.Conn ctrlEvents chan *bulb.Response

@@ -146,37 +145,47 @@ func (t *Tor) getconf(arg string) (*bulb.Response, error) {

// Shutdown attempts to gracefully clean up the Tor instance. If it is a // system tor, only the control port connection will be closed. Otherwise, -// the tor daemon will be SIGTERMed. +// the tor daemon will be terminated, gracefully if possible. func (t *Tor) Shutdown() { t.Lock() defer t.Unlock()

+ sentHalt := false if t.ctrl != nil { - // Try extra hard to get tor to fuck off, if we spawned it. + // Try to gracefully terminate the daemon via the control port. if !t.isSystem { t.ctrl.Request("SIGNAL HALT") + sentHalt = true } t.ctrl.Close() t.ctrl = nil }

- if t.cmd != nil { - waitCh := make(chan bool) - go func() { - t.cmd.Process.Signal(syscall.SIGTERM) - t.cmd.Process.Wait() - waitCh <- true - }() + if t.process != nil { + if t.isSystem { + panic("tor: system tor has a sandbox child process") + }

- select { - case <-waitCh: - Debugf("tor: Process exited after SIGTERM") - case <-time.After(5 * time.Second): - Debugf("tor: Process timed out waiting after SIGTERM, killing.") - t.cmd.Process.Signal(syscall.SIGKILL) + if sentHalt { + waitCh := make(chan bool) + go func() { + t.process.Wait() + waitCh <- true + }() + + select { + case <-waitCh: + Debugf("tor: Process exited after HALT") + case <-time.After(5 * time.Second): + Debugf("tor: Process timed out waiting after HALT, killing.") + t.process.Kill() + } + } else { + Debugf("tor: Process has no control port, killing") + t.process.Kill() }

- t.cmd = nil + t.process = nil }

if t.ctrlSurrogate != nil { @@ -281,10 +290,10 @@ func NewSystemTor(cfg *config.Config) (*Tor, error) { }

// NewSandboxedTor creates a Tor struct around a sandboxed tor instance. -func NewSandboxedTor(cfg *config.Config, cmd *exec.Cmd) *Tor { +func NewSandboxedTor(cfg *config.Config, process *process.Process) *Tor { t := new(Tor) t.isSystem = false - t.cmd = cmd + t.process = process t.socksNet = "unix" t.socksAddr = filepath.Join(cfg.TorDataDir, "socks") t.ctrlAddr = filepath.Join(cfg.TorDataDir, "control") @@ -333,9 +342,10 @@ func (t *Tor) DoBootstrap(cfg *config.Config, async *Async) (err error) { if t.ctrl, err = bulb.Dial("unix", t.ctrlAddr); err != nil { return err } + ctrl := t.ctrl // Shadow, so that we fail gracefully on close.

// Authenticate with the control port. - if err = t.ctrl.Authenticate(cfg.Tor.CtrlPassword); err != nil { + if err = ctrl.Authenticate(cfg.Tor.CtrlPassword); err != nil { return err }

@@ -344,22 +354,22 @@ func (t *Tor) DoBootstrap(cfg *config.Config, async *Async) (err error) { // shouldn't leave a turd process lying around, though I've seen it on // occaision. :( log.Printf("tor: Taking ownership of the tor process") - if _, err = t.ctrl.Request("TAKEOWNERSHIP"); err != nil { + if _, err = ctrl.Request("TAKEOWNERSHIP"); err != nil { return err }

// Start the event async reader. - t.ctrl.StartAsyncReader() + ctrl.StartAsyncReader() go t.eventReader()

// Register the `STATUS_CLIENT` event handler. - if _, err = t.ctrl.Request("SETEVENTS STATUS_CLIENT"); err != nil { + if _, err = ctrl.Request("SETEVENTS STATUS_CLIENT"); err != nil { return err }

// Start the bootstrap. async.UpdateProgress("Connecting to the Tor network.") - if _, err = t.ctrl.Request("RESETCONF DisableNetwork"); err != nil { + if _, err = ctrl.Request("RESETCONF DisableNetwork"); err != nil { return err }

@@ -383,10 +393,9 @@ func (t *Tor) DoBootstrap(cfg *config.Config, async *Async) (err error) { case <-hz.C: const statusPrefix = "status/bootstrap-phase="

- // As a fallback, use kill(pid, 0) to detect if the process has - // puked. waitpid(2) is probably better since it's a child, but - // this should be good enough, and is only to catch tor crashing. - if err := syscall.Kill(t.cmd.Process.Pid, 0); err == syscall.ESRCH { + // As a fallback, periodicall poll to see if the process has + // crashed. + if !t.process.Running() { return fmt.Errorf("tor process appears to have crashed.") }

@@ -414,7 +423,7 @@ func (t *Tor) DoBootstrap(cfg *config.Config, async *Async) (err error) { }

// Squelch the events, and drain the event queue. - if _, err = t.ctrl.Request("SETEVENTS"); err != nil { + if _, err = ctrl.Request("SETEVENTS"); err != nil { return err } for len(t.ctrlEvents) > 0 { diff --git a/src/cmd/sandboxed-tor-browser/internal/ui/gtk/ui.go b/src/cmd/sandboxed-tor-browser/internal/ui/gtk/ui.go index 7b36638..7a6624c 100644 --- a/src/cmd/sandboxed-tor-browser/internal/ui/gtk/ui.go +++ b/src/cmd/sandboxed-tor-browser/internal/ui/gtk/ui.go @@ -213,13 +213,14 @@ func (ui *gtkUI) Run() error { }

// Kill the browser. It's not as if firefox does the right thing on - // SIGTERM/SIGINT and we have the pid of the bubblewrap child instead - // of the firefox process anyway... + // SIGTERM/SIGINT and we have the pid of init inside the sandbox + // anyway... // // https://bugzilla.mozilla.org/show_bug.cgi?id=336193 - ui.Sandbox.Process.Kill() + ui.Sandbox.Kill() <-waitCh

+ ui.Sandbox = nil ui.PendingUpdate = update ui.ForceConfig = false ui.NoKillTor = true // Don't re-lauch tor on the first pass. diff --git a/src/cmd/sandboxed-tor-browser/internal/ui/ui.go b/src/cmd/sandboxed-tor-browser/internal/ui/ui.go index 53034e2..1de7e8c 100644 --- a/src/cmd/sandboxed-tor-browser/internal/ui/ui.go +++ b/src/cmd/sandboxed-tor-browser/internal/ui/ui.go @@ -28,7 +28,6 @@ import ( "net" "net/http" "os" - "os/exec" "path/filepath" "strings" "syscall" @@ -39,6 +38,7 @@ import ( "cmd/sandboxed-tor-browser/internal/data" "cmd/sandboxed-tor-browser/internal/installer" "cmd/sandboxed-tor-browser/internal/sandbox" + "cmd/sandboxed-tor-browser/internal/sandbox/process" "cmd/sandboxed-tor-browser/internal/tor" . "cmd/sandboxed-tor-browser/internal/ui/async" "cmd/sandboxed-tor-browser/internal/ui/config" @@ -96,7 +96,7 @@ type UI interface { type Common struct { Cfg *config.Config Manif *config.Manifest - Sandbox *exec.Cmd + Sandbox *process.Process tor *tor.Tor lock *lockFile

@@ -322,14 +322,14 @@ func (c *Common) launchTor(async *Async, onlySystem bool) error { os.Remove(filepath.Join(c.Cfg.TorDataDir, "control_port"))

async.UpdateProgress("Launching Tor executable.") - cmd, err := sandbox.RunTor(c.Cfg, c.Manif, torrc) + process, err := sandbox.RunTor(c.Cfg, c.Manif, torrc) if err != nil { async.Err = err return err }

async.UpdateProgress("Waiting on Tor bootstrap.") - c.tor = tor.NewSandboxedTor(c.Cfg, cmd) + c.tor = tor.NewSandboxedTor(c.Cfg, process) if err = c.tor.DoBootstrap(c.Cfg, async); err != nil { async.Err = err return err