services/mgmt/{device,suidhelper}: Quickly return forked pid from helper
Return the pid of forked child from suidhelper to the device manager, and
avoid waiting the entire timeout if the app dies while the device manager
is waiting for the init handshake, or for the app cycle mgr name.
If the app appears to be lying about its pid, we don't currently do anything
beyond treating this situation as a failure to start the app. We'll make
suidhelper kill the app in a future update.
Change-Id: I53a0fa04ea95f1332ff30bd5131194b44c4b4c32
diff --git a/services/mgmt/device/impl/app_service.go b/services/mgmt/device/impl/app_service.go
index 3952e1e..2aa4f87 100644
--- a/services/mgmt/device/impl/app_service.go
+++ b/services/mgmt/device/impl/app_service.go
@@ -840,6 +840,13 @@
appAclDir := filepath.Join(instanceDir, "debugacls", "data")
cfg.Set("v23.permissions.file", "runtime:"+appAclDir)
+ // This adds to cmd.Extrafiles. The helper expects a fixed fd, so this call needs
+ // to go before anything that conditionally adds to Extrafiles, like the agent
+ // setup code immediately below.
+ var handshaker appHandshaker
+ handshaker.prepareToStart(ctx, cmd)
+ defer handshaker.cleanup()
+
// Set up any agent-specific state.
// NOTE(caprita): This ought to belong in genCmd.
var agentCleaner func()
@@ -871,35 +878,30 @@
}
}
}()
+
// Start the child process.
- if err := handle.Start(); err != nil {
- if agentCleaner != nil {
- agentCleaner()
- }
- return 0, verror.New(ErrOperationFailed, ctx, fmt.Sprintf("Start() failed: %v", err))
- }
+ startErr := handle.Start()
+ // Perform unconditional cleanup before dealing with any error from handle.Start()
if agentCleaner != nil {
agentCleaner()
}
+ // Now react to any error in handle.Start()
+ if startErr != nil {
+ return 0, verror.New(ErrOperationFailed, ctx, fmt.Sprintf("Start() failed: %v", err))
+ }
- // Wait for the suidhelper to exit.
+ // Wait for the suidhelper to exit. This is blocking as we assume the helper won't
+ // get stuck.
if err := handle.Wait(0); err != nil {
return 0, verror.New(ErrOperationFailed, ctx, fmt.Sprintf("Wait() on suidhelper failed: %v", err))
}
- // Wait for the process invoked by suidhelper to become ready.
- if err := handle.WaitForReady(childReadyTimeout); err != nil {
- return 0, verror.New(ErrOperationFailed, ctx, fmt.Sprintf("WaitForReady(%v) failed: %v", childReadyTimeout, err))
- }
- pid := handle.ChildPid()
- childName, err := listener.waitForValue(childReadyTimeout)
+ pid, childName, err := handshaker.doHandshake(handle, listener)
+
if err != nil {
- return 0, verror.New(ErrOperationFailed, nil)
+ return 0, err
}
- // Because suidhelper uses Go's in-built support for setuid forking,
- // handle.Pid() is the pid of suidhelper, not the pid of the app
- // so use the pid returned in the app's ready status.
info.AppCycleMgrName, info.Pid = childName, pid
if err := saveInstanceInfo(ctx, instanceDir, info); err != nil {
return 0, err
diff --git a/services/mgmt/device/impl/app_starting_util.go b/services/mgmt/device/impl/app_starting_util.go
new file mode 100644
index 0000000..a75f4e3
--- /dev/null
+++ b/services/mgmt/device/impl/app_starting_util.go
@@ -0,0 +1,186 @@
+// Copyright 2015 The Vanadium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package impl
+
+// TODO -- Ideally the code in this file would be integrated with the instance reaping,
+// so we avoid having two process polling loops. This code is currently separate because
+// the actions taken when the app dies (or is caught lying about its pid) prior to being
+// considered running are fairly different from what's currently done by the reaper in
+// handling deaths that occur after the app started successfully.
+
+import (
+ "encoding/binary"
+ "fmt"
+ "os"
+ "os/exec"
+ "syscall"
+ "time"
+
+ "v.io/v23/context"
+ "v.io/v23/verror"
+ "v.io/x/lib/vlog"
+ vexec "v.io/x/ref/lib/exec"
+ helperconsts "v.io/x/ref/services/mgmt/suidhelper/constants"
+)
+
+// appWatcher watches the pid of a running app until either the pid exits or stop()
+// is called
+type appWatcher struct {
+ pid int // Pid to watch
+ callback func() // Called if the pid exits or if stop() is invoked
+ stopper chan struct{} // Used to stop the appWatcher
+}
+
+func newAppWatcher(pidToWatch int, callOnPidExit func()) *appWatcher {
+ return &appWatcher{
+ pid: pidToWatch,
+ callback: callOnPidExit,
+ stopper: make(chan struct{}, 1),
+ }
+}
+
+func (a *appWatcher) stop() {
+ close(a.stopper)
+}
+
+func (a *appWatcher) watchAppPid() {
+ defer a.callback()
+
+ ticker := time.NewTicker(1 * time.Second)
+ defer ticker.Stop()
+
+ for {
+ select {
+ case <-ticker.C:
+ if err := syscall.Kill(a.pid, 0); err != nil && err != syscall.EPERM {
+ vlog.Errorf("App died in startup: pid=%d: %v", a.pid, err)
+ return
+ } else {
+ vlog.VI(2).Infof("App pid %d is alive", a.pid)
+ }
+
+ case <-a.stopper:
+ vlog.Errorf("AppWatcher was stopped")
+ return
+ }
+ }
+ // Not reached.
+}
+
+// appHandshaker is a utility to do the app handshake for a newly started app while
+// reacting quickly if the app crashes. appHandshaker reads two pids from the app (one
+// from the helper that forked the app, and the other from the app itself). If the app
+// appears to be lying about its own pid, it will kill the app.
+type appHandshaker struct {
+ helperRead, helperWrite *os.File
+ ctx *context.T
+}
+
+func (a *appHandshaker) cleanup() {
+ if a.helperRead != nil {
+ a.helperRead.Close()
+ a.helperRead = nil
+ }
+ if a.helperWrite != nil {
+ a.helperWrite.Close()
+ a.helperWrite = nil
+ }
+}
+
+// prepareToStart sets up the pipe used to talk to the helper. It must be called before
+// the app is started so that the app will inherit the file descriptor
+func (a *appHandshaker) prepareToStart(ctx *context.T, cmd *exec.Cmd) error {
+ if helperconsts.PipeToParentFD != (len(cmd.ExtraFiles) + vexec.FileOffset) {
+ return verror.New(ErrOperationFailed, ctx,
+ fmt.Sprintf("FD expected by helper (%v) was not available (%v) (%v)",
+ helperconsts.PipeToParentFD, len(cmd.ExtraFiles), vexec.FileOffset))
+ }
+ a.ctx = ctx
+
+ var err error
+ a.helperRead, a.helperWrite, err = os.Pipe()
+ if err != nil {
+ vlog.Errorf("Failed to create pipe: %v", err)
+ return err
+ }
+ cmd.ExtraFiles = append(cmd.ExtraFiles, a.helperWrite)
+ return nil
+}
+
+// doAppHandshake executes the startup handshake for the app. Upon success, it returns the
+// pid and appCycle manager name for the started app.
+//
+// handle should have been set up to use a helper for the app and handle.Start()
+// and handle.Wait() should already have been called (so we know the helper is done)
+func (a *appHandshaker) doHandshake(handle *vexec.ParentHandle, listener callbackListener) (int, string, error) {
+ // Close our copy of helperWrite to make helperRead return EOF once the
+ // helper's copy of helperWrite is closed.
+ a.helperWrite.Close()
+ a.helperWrite = nil
+
+ // Get the app pid from the helper. This won't block as the helper is done
+ var pid32 int32
+ if err := binary.Read(a.helperRead, binary.LittleEndian, &pid32); err != nil {
+ vlog.Errorf("Error reading app pid from child: %v", err)
+ return 0, "", verror.New(ErrOperationFailed, a.ctx, fmt.Sprintf("failed to read pid from helper: %v", err))
+ }
+ pidFromHelper := int(pid32)
+ vlog.VI(1).Infof("read app pid %v from child", pidFromHelper)
+
+ // Watch the app pid in case it exits.
+ pidExitedChan := make(chan struct{}, 1)
+ watcher := newAppWatcher(pidFromHelper, func() {
+ listener.stop()
+ close(pidExitedChan)
+ })
+ go watcher.watchAppPid()
+ defer watcher.stop()
+
+ // Wait for the child to say it's ready and provide its own pid via the init handshake
+ childReadyErrChan := make(chan error, 1)
+ go func() {
+ if err := handle.WaitForReady(childReadyTimeout); err != nil {
+ childReadyErrChan <- verror.New(ErrOperationFailed, a.ctx, fmt.Sprintf("WaitForReady(%v) failed: %v", childReadyTimeout, err))
+ }
+ childReadyErrChan <- nil
+ }()
+
+ // Wait until we get the pid from the app, but return early if
+ // the watcher notices that the app failed
+ pidFromChild := 0
+
+ select {
+ case <-pidExitedChan:
+ return 0, "", verror.New(ErrOperationFailed, a.ctx,
+ fmt.Sprintf("App exited (pid %d)", pidFromHelper))
+
+ case err := <-childReadyErrChan:
+ if err != nil {
+ return 0, "", err
+ }
+ // Note: handle.Pid() is the pid of the helper, rather than that
+ // of the app that the helper then forked. ChildPid is the pid
+ // received via the app startup handshake
+ pidFromChild = handle.ChildPid()
+ }
+
+ if pidFromHelper != pidFromChild {
+ // Something nasty is going on and we should kill pidFromHelper.
+ // TODO(arup): In future we'll extend suidhelper to support a kill
+ // function, and invoke it here.
+ return 0, "", verror.New(ErrOperationFailed, a.ctx,
+ fmt.Sprintf("Child pids do not match! (%d != %d)", pidFromHelper, pidFromChild))
+ }
+
+ // The appWatcher will stop the listener if the pid dies while waiting below
+ childName, err := listener.waitForValue(childReadyTimeout)
+ if err != nil {
+ // TODO(arup) kill pidFromHelper here using suidhelper
+ return 0, "", verror.New(ErrOperationFailed, a.ctx,
+ fmt.Sprintf("Waiting for child name: %v", err))
+ }
+
+ return pidFromHelper, childName, nil
+}
diff --git a/services/mgmt/suidhelper/constants/constants.go b/services/mgmt/suidhelper/constants/constants.go
new file mode 100644
index 0000000..03b93af
--- /dev/null
+++ b/services/mgmt/suidhelper/constants/constants.go
@@ -0,0 +1,11 @@
+// Copyright 2015 The Vanadium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package constants
+
+const (
+ // fd of the pipe to be used to return the pid of the forked child to the
+ // device manager.
+ PipeToParentFD = 5
+)
diff --git a/services/mgmt/suidhelper/impl/system.go b/services/mgmt/suidhelper/impl/system.go
index 449ad38..9c90e74 100644
--- a/services/mgmt/suidhelper/impl/system.go
+++ b/services/mgmt/suidhelper/impl/system.go
@@ -7,12 +7,14 @@
package impl
import (
+ "encoding/binary"
"log"
"os"
"path/filepath"
"syscall"
"v.io/v23/verror"
+ "v.io/x/ref/services/mgmt/suidhelper/constants"
)
var (
@@ -49,7 +51,7 @@
attr := new(syscall.ProcAttr)
if dir, err := os.Getwd(); err != nil {
- log.Printf("error Getwd(): %v\n", err)
+ log.Printf("error Getwd(): %v", err)
return verror.New(errGetwdFailed, nil, err)
attr.Dir = dir
}
@@ -71,16 +73,28 @@
attr.Sys.Credential.Uid = uint32(hw.uid)
}
- _, _, err := syscall.StartProcess(hw.argv0, hw.argv, attr)
+ // Make sure the child won't talk on the fd we use to talk back to the parent
+ syscall.CloseOnExec(constants.PipeToParentFD)
+
+ // Start the child process
+ pid, _, err := syscall.StartProcess(hw.argv0, hw.argv, attr)
if err != nil {
if !hw.dryrun {
- log.Printf("StartProcess failed: attr: %#v, attr.Sys: %#v, attr.Sys.Cred: %#v error: %v\n", attr, attr.Sys, attr.Sys.Credential, err)
+ log.Printf("StartProcess failed: attr: %#v, attr.Sys: %#v, attr.Sys.Cred: %#v error: %v", attr, attr.Sys, attr.Sys.Credential, err)
} else {
log.Printf("StartProcess failed: %v", err)
}
return verror.New(errStartProcessFailed, nil, hw.argv0, err)
}
- // TODO(rjkroege): Return the pid to the node manager.
+
+ // Return the pid of the new child process
+ pipeToParent := os.NewFile(constants.PipeToParentFD, "pipe_to_parent_wr")
+ if err = binary.Write(pipeToParent, binary.LittleEndian, int32(pid)); err != nil {
+ log.Printf("Problem returning pid to parent: %v", err)
+ } else {
+ log.Printf("Returned pid %v to parent", pid)
+ }
+
os.Exit(0)
return nil // Not reached.
}
