Subprocess: Use pidfd (PIDFD_GET_INFO if supported) to monitor an imported process for exit status

lxd/subprocess/bgpm_test.go

@@ -5,10 +5,12 @@ package subprocess
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
@@ -238,14 +240,9 @@ func savePidFile(t *testing.T, p *Process, mutate func(*Process)) string {
 
 	mutate(saved)
 
-	dat, err = yaml.Marshal(saved)
+	err = saved.Save(path)
 	if err != nil {
-		t.Fatal("Failed serializing pid file: ", err)
-	}
-
-	err = os.WriteFile(path, dat, 0600)
-	if err != nil {
-		t.Fatal("Failed writing pid file: ", err)
+		t.Fatal("Failed saving mutated pid file: ", err)
 	}
 
 	return path
@@ -286,6 +283,15 @@ func TestImportRoundTripLive(t *testing.T) {
 		t.Error("Failed stopping imported process: ", err)
 	}
 
+	// Stop on an imported process must block until its monitor has completed.
+	ctxWait, cancelWait := context.WithTimeout(context.Background(), 100*time.Millisecond)
+	defer cancelWait()
+
+	_, err = imp.Wait(ctxWait)
+	if errors.Is(err, context.DeadlineExceeded) {
+		t.Error("Imported Stop returned before the imported process exited")
+	}
+
 	// The kill must land on the process we saved: its monitor sees it exit.
 	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
 	defer cancel()
@@ -410,3 +416,111 @@ func TestImportOldFormat(t *testing.T) {
 		t.Error("Failed stopping process imported from old-format file: ", err)
 	}
 }
+
+// TestImportWaitExitCode checks that a live process imported from a pid file is waited on via a
+// pidfd (rather than as a child), that Wait blocks until it exits, and that its exit code is
+// reported when the kernel supports PIDFD_GET_INFO.
+func TestImportWaitExitCode(t *testing.T) {
+	// Use a FIFO to synchronise with the child. This avoids wall-clock timing
+	// assertions that can be flaky on loaded runners.
+	fifoPath := filepath.Join(t.TempDir(), "fifo")
+	err := syscall.Mkfifo(fifoPath, 0600)
+	if err != nil {
+		t.Fatal("Failed creating fifo: ", err)
+	}
+
+	p, err := NewProcess("sh", []string{"-c", fmt.Sprintf("read line < %s; exit 42", fifoPath)}, "", "")
+	if err != nil {
+		t.Fatal("Failed process creation: ", err)
+	}
+
+	err = p.Start(context.Background())
+	if err != nil {
+		t.Fatal("Failed starting process: ", err)
+	}
+
+	t.Cleanup(func() { _ = p.Stop() })
+
+	path := savePidFile(t, p, nil)
+
+	// Import while the process is still running so a pidfd-based monitor is started.
+	imp, err := ImportProcess(path)
+	if err != nil {
+		t.Fatal("Failed importing process: ", err)
+	}
+
+	if !imp.hasMonitor {
+		t.Fatal("Imported live process should have a monitor to be waited on")
+	}
+
+	// Start waiting in the background and verify it remains blocked.
+	waitDone := make(chan struct{})
+	var code int64
+	var waitErr error
+	go func() {
+		defer close(waitDone)
+		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+		defer cancel()
+		code, waitErr = imp.Wait(ctx)
+	}()
+
+	select {
+	case <-waitDone:
+		t.Fatal("Wait returned before the process was released")
+	case <-time.After(200 * time.Millisecond):
+	}
+
+	// Open the FIFO for writing and release the child. The open itself blocks until
+	// the child has opened the FIFO for reading, so do it in the background too.
+	fifoOpened := make(chan *os.File, 1)
+	fifoErr := make(chan error, 1)
+	go func() {
+		f, err := os.OpenFile(fifoPath, os.O_WRONLY, 0)
+		if err != nil {
+			fifoErr <- err
+			return
+		}
+
+		fifoOpened <- f
+	}()
+
+	var f *os.File
+	select {
+	case f = <-fifoOpened:
+	case err = <-fifoErr:
+		t.Fatal("Failed opening fifo for writing: ", err)
+	case <-time.After(2 * time.Second):
+		t.Fatal("Timed out waiting for child to open fifo")
+	}
+
+	_, err = f.WriteString("\n")
+	if err != nil {
+		t.Fatal("Failed writing to fifo: ", err)
+	}
+
+	_ = f.Close()
+
+	select {
+	case <-waitDone:
+	case <-time.After(10 * time.Second):
+		t.Fatal("Wait did not return after releasing the process")
+	}
+
+	if errors.Is(waitErr, context.DeadlineExceeded) {
+		t.Fatal("Wait on imported process timed out")
+	}
+
+	if code == -1 {
+		// The kernel lacks PIDFD_GET_INFO exit support; the wait still completed on exit.
+		t.Log("Kernel does not support PIDFD_GET_INFO; exit code unavailable")
+		return
+	}
+
+	if code != 42 {
+		t.Errorf("Expected exit code 42 but got %d", code)
+	}
+
+	if waitErr == nil {
+		t.Error("Expected a non-nil error for a non-zero exit code")
+	}
+}

lxd/subprocess/errors.go

@@ -9,5 +9,8 @@ import (
 // ErrNotRunning is returned when performing an action against a stopped process.
 var ErrNotRunning = errors.New("The process is not running")
 
+// ErrAlreadyRunning is returned when trying to start a process that is already running.
+var ErrAlreadyRunning = errors.New("The process is already running")
+
 // ErrBadPID is returned when an import file contains a facially incorrect/dangerous PID <= 0.
 var ErrBadPID = errors.New("Invalid PID")

lxd/subprocess/manager.go

@@ -83,6 +83,21 @@ func ImportProcess(path string) (*Process, error) {
 	// On unix, FindProcess always returns successfully (with a 'done' process if pidfd_open
 	// returned with ESRCH).
 	proc.proc, _ = os.FindProcess(proc.PID)
+
+	// First check whether the process is still alive. If it is not, there is no
+	// need to verify identity further; just release the handle and return a
+	// stopped Process object.
+	if proc.Signal(0) != nil {
+		if proc.proc != nil {
+			_ = proc.proc.Release()
+			proc.proc = nil
+		}
+
+		return &proc, nil
+	}
+
+	// The process appears alive; verify it is the same process we saved by
+	// comparing start times.
 	if proc.StartTime != 0 {
 		starttime, err := processStartTime(proc.PID)
 		if err == nil {
@@ -94,5 +109,10 @@ func ImportProcess(path string) (*Process, error) {
 		}
 	}
 
+	// Spawn a monitor goroutine so a running imported process can be waited on like a spawned
+	// one. Only do so when the process is actually alive; the monitor records the exit code and
+	// starting it for an already-exited process would race with any later reuse of the object.
+	proc.monitorImported()
+
 	return &proc, nil
 }

lxd/subprocess/proc.go

@@ -79,12 +79,50 @@ func (p *Process) release() {
 func (p *Process) finish() {
 	if p.hasMonitor {
 		<-p.chExit
-		return
+		p.hasMonitor = false
 	}
 
 	p.release()
 }
 
+// startMonitor spawns a goroutine that waits for the process to exit using the supplied wait
+// function, records the resulting exit code and error, and closes chExit. It is shared by spawned
+// processes (which reap via the child handle) and imported processes (which wait via a pidfd).
+func (p *Process) startMonitor(wait func() (int64, error)) {
+	p.exitCode = -1
+	p.exitErr = nil
+	chExit := make(chan struct{})
+	p.chExit = chExit
+	p.hasMonitor = true
+
+	go func() {
+		defer close(chExit)
+
+		p.exitCode, p.exitErr = wait()
+	}()
+}
+
+// monitorImported starts a monitor for an imported (non-child) process, waiting on it via a pidfd.
+// On kernels that support PIDFD_GET_INFO the exit code is recorded, otherwise it is unknown.
+func (p *Process) monitorImported() {
+	// Capture the identity locally so the wait does not touch p, which may be reused (e.g. via
+	// Start or Restart) while this monitor is still running.
+	pid := p.PID
+	startTime := p.StartTime
+	p.startMonitor(func() (int64, error) {
+		code, err := waitProcess(context.Background(), pid, startTime)
+		if err != nil {
+			return -1, err
+		}
+
+		if code > 0 {
+			return code, fmt.Errorf("Process exited with non-zero value %d", code)
+		}
+
+		return code, nil
+	})
+}
+
 // Stop will stop the given process object.
 func (p *Process) Stop() error {
 	if p.proc == nil {
@@ -118,6 +156,13 @@ func (p *Process) StartWithFiles(ctx context.Context, fds []*os.File) error {
 }
 
 func (p *Process) start(ctx context.Context, fds []*os.File) error {
+	// If this Process object is already associated with a live process, refuse
+	// to start a new one. finish() clears hasMonitor once the process has exited,
+	// so Restart() (Stop -> Start) continues to work.
+	if p.hasMonitor {
+		return ErrAlreadyRunning
+	}
+
 	var cmd *exec.Cmd
 
 	if p.Apparmor != "" && p.hasApparmor() {
@@ -174,34 +219,25 @@ func (p *Process) start(ctx context.Context, fds []*os.File) error {
 	if err == nil {
 		p.BootID = bootID
 	}
-	// Reset exitCode/exitErr
-	p.exitCode = 0
-	p.exitErr = nil
-
-	// Spawn a goroutine waiting for it to exit.
-	p.chExit = make(chan struct{})
-	p.hasMonitor = true
-	go func() {
-		defer close(p.chExit)
 
+	p.startMonitor(func() (int64, error) {
 		err := cmd.Wait()
 
+		code := int64(-1)
 		if cmd.ProcessState != nil {
-			p.exitCode = int64(cmd.ProcessState.ExitCode())
-		} else {
-			p.exitCode = -1
+			code = int64(cmd.ProcessState.ExitCode())
 		}
 
 		if err != nil {
-			p.exitErr = err
-
-			return
+			return code, err
 		}
 
-		if p.exitCode != 0 {
-			p.exitErr = fmt.Errorf("Process exited with non-zero value %d", p.exitCode)
+		if code != 0 {
+			return code, fmt.Errorf("Process exited with non-zero value %d", code)
 		}
-	}()
+
+		return code, nil
+	})
 
 	return nil
 }
@@ -242,7 +278,23 @@ func (p *Process) Reload() error {
 
 // Save will save the given process object to a YAML file. Can be imported at a later point.
 func (p *Process) Save(path string) error {
-	dat, err := yaml.Marshal(p)
+	// Marshal a copy of only the persisted fields. Marshalling the live process would have yaml
+	// read the whole struct (via reflect) and race with the monitor goroutine updating the
+	// exit state. The excluded fields are not persisted anyway.
+	saved := Process{
+		Name:        p.Name,
+		Args:        p.Args,
+		Apparmor:    p.Apparmor,
+		PID:         p.PID,
+		BootID:      p.BootID,
+		UID:         p.UID,
+		GID:         p.GID,
+		SetGroups:   p.SetGroups,
+		StartTime:   p.StartTime,
+		SysProcAttr: p.SysProcAttr,
+	}
+
+	dat, err := yaml.Marshal(&saved)
 	if err != nil {
 		return fmt.Errorf("Cannot serialize process struct to YAML: %w", err)
 	}