[tor-commits] [sandboxed-tor-browser/master] Bug 21184: Do a better job of killing/cleaning up bwrap children.
yawning at torproject.org
yawning at torproject.org
Tue Jan 10 07:45:54 UTC 2017
commit d34355a097590c900217acc397ad6a383af04267
Author: Yawning Angel <yawning at 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
More information about the tor-commits
mailing list