diff --git a/lxd/subprocess/bgpm_test.go b/lxd/subprocess/bgpm_test.go index f19c8249171e..0b8008cf2d9c 100644 --- a/lxd/subprocess/bgpm_test.go +++ b/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") + } +} diff --git a/lxd/subprocess/errors.go b/lxd/subprocess/errors.go index c37008d0d564..6f741345e4c3 100644 --- a/lxd/subprocess/errors.go +++ b/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") diff --git a/lxd/subprocess/manager.go b/lxd/subprocess/manager.go index f63a5d6c55cb..b67c958c7529 100644 --- a/lxd/subprocess/manager.go +++ b/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 } diff --git a/lxd/subprocess/proc.go b/lxd/subprocess/proc.go index 86e51b0d7b8d..ef73bc0f89e1 100644 --- a/lxd/subprocess/proc.go +++ b/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) } diff --git a/lxd/subprocess/procwait_linux.go b/lxd/subprocess/procwait_linux.go new file mode 100644 index 000000000000..4b2a4b2d1e17 --- /dev/null +++ b/lxd/subprocess/procwait_linux.go @@ -0,0 +1,182 @@ +//go:build linux && cgo + +package subprocess + +/* +#include +#include + +// pidfd_info_ioctl mirrors the kernel struct pidfd_info used by the +// PIDFD_GET_INFO ioctl. It is defined here so the ioctl request number can be +// built with the architecture-specific _IOWR macro, avoiding the asm-generic +// shift assumptions that break on PowerPC and MIPS. +struct pidfd_info_ioctl { + uint64_t mask; + uint64_t cgroupid; + uint32_t pid; + uint32_t tgid; + uint32_t ppid; + uint32_t ruid; + uint32_t rgid; + uint32_t euid; + uint32_t egid; + uint32_t suid; + uint32_t sgid; + uint32_t fsuid; + uint32_t fsgid; + int32_t exit_code; + uint32_t coredump_mask; + uint32_t coredump_signal; + uint32_t coredump_code; + uint32_t coredump_pad; + uint64_t supported_mask; +}; + +#define PIDFD_GET_INFO_IOCTL _IOWR(0xFF, 11, struct pidfd_info_ioctl) +*/ +import "C" + +import ( + "context" + "errors" + "fmt" + "os" + "syscall" + "time" + "unsafe" + + "golang.org/x/sys/unix" +) + +const ( + // pidfdInfoExit requests exit information from PIDFD_GET_INFO (Linux 6.15+). + pidfdInfoExit = 1 << 3 +) + +// waitProcess blocks until the process identified by pid has exited, or ctx is cancelled. +// It uses a pidfd so it can wait on a process that is not a child of the current process +// (for example one imported from a PID file). The startTime is used to guard against PID reuse. +// On kernels that support PIDFD_GET_INFO it returns the process exit code, otherwise -1. +// +// If pidfd monitoring is unavailable or fails, waitProcess falls back to polling the process +// with signal 0 so that callers still do not return until the process has exited. +func waitProcess(ctx context.Context, pid int, startTime int64) (int64, error) { + code, err := waitProcessPidfd(ctx, pid, startTime) + if err == nil { + return code, nil + } + + // pidfd_open or pidfd polling failed; preserve the wait guarantee by falling + // back to signal-0 polling. + return waitProcessSignal0(ctx, pid, startTime) +} + +// waitProcessPidfd waits for the process using a pidfd and PIDFD_GET_INFO. +func waitProcessPidfd(ctx context.Context, pid int, startTime int64) (int64, error) { + pidFd, err := unix.PidfdOpen(pid, 0) + if err != nil { + // ESRCH means the process is already gone. + if errors.Is(err, unix.ESRCH) { + return -1, nil + } + + return -1, fmt.Errorf("Failed opening pidfd for PID %d: %w", pid, err) + } + + defer func() { _ = unix.Close(pidFd) }() + + // Guard against PID reuse: if the recorded start time no longer matches, the original + // process has already exited and a new one has reused its PID. + if startTime != 0 { + currentStartTime, err := processStartTime(pid) + if err != nil || currentStartTime != startTime { + return -1, nil + } + } + + // The pidfd becomes readable once the process exits. + pollFds := []unix.PollFd{{Fd: int32(pidFd), Events: unix.POLLIN}} + for { + // Poll with a bounded timeout so context cancellation can be observed. + n, err := unix.Poll(pollFds, 100) + if err != nil { + if errors.Is(err, unix.EINTR) { + continue + } + + return -1, fmt.Errorf("Failed polling pidfd for PID %d: %w", pid, err) + } + + if n > 0 { + revents := pollFds[0].Revents + if revents&(unix.POLLERR|unix.POLLNVAL) != 0 { + return -1, fmt.Errorf("pidfd for PID %d reported error events: %d", pid, revents) + } + + if revents&unix.POLLIN != 0 { + return pidfdExitCode(pidFd), nil + } + + // POLLHUP or other unexpected events are ignored and polling continues. + continue + } + + if ctx.Err() != nil { + return -1, ctx.Err() + } + } +} + +// waitProcessSignal0 waits for the process by polling with signal 0. +func waitProcessSignal0(ctx context.Context, pid int, startTime int64) (int64, error) { + proc, err := os.FindProcess(pid) + if err != nil { + return -1, nil + } + + for { + err := proc.Signal(syscall.Signal(0)) + if err != nil { + return -1, nil + } + + // Guard against PID reuse while polling. + if startTime != 0 { + currentStartTime, err := processStartTime(pid) + if err != nil || currentStartTime != startTime { + return -1, nil + } + } + + select { + case <-ctx.Done(): + return -1, ctx.Err() + case <-time.After(50 * time.Millisecond): + } + } +} + +// pidfdExitCode returns the exit code of the exited process referenced by pidFd using the +// PIDFD_GET_INFO ioctl, or -1 if the kernel does not support it or the process was signalled. +func pidfdExitCode(pidFd int) int64 { + var info C.struct_pidfd_info_ioctl + info.mask = C.uint64_t(pidfdInfoExit) + + req := uintptr(C.PIDFD_GET_INFO_IOCTL) + _, _, errno := unix.Syscall(unix.SYS_IOCTL, uintptr(pidFd), req, uintptr(unsafe.Pointer(&info))) + if errno != 0 { + return -1 + } + + // The kernel clears the requested bit if it could not provide the information. + if info.mask&C.uint64_t(pidfdInfoExit) == 0 { + return -1 + } + + waitStatus := syscall.WaitStatus(uint32(info.exit_code)) + if !waitStatus.Exited() { + return -1 + } + + return int64(waitStatus.ExitStatus()) +} diff --git a/lxd/subprocess/procwait_other.go b/lxd/subprocess/procwait_other.go new file mode 100644 index 000000000000..0f3a05376757 --- /dev/null +++ b/lxd/subprocess/procwait_other.go @@ -0,0 +1,34 @@ +//go:build (!linux || !cgo) && !windows + +package subprocess + +import ( + "context" + "os" + "syscall" + "time" +) + +// waitProcess blocks until the process identified by pid has exited, or ctx is cancelled. +// pidfd is only available on Linux, so this fallback polls the process for liveness and cannot +// determine the exit code, which is reported as -1. +func waitProcess(ctx context.Context, pid int, startTime int64) (int64, error) { + proc, err := os.FindProcess(pid) + if err != nil { + return -1, nil + } + + for { + // Signal 0 reports whether the process is still present. + err := proc.Signal(syscall.Signal(0)) + if err != nil { + return -1, nil + } + + select { + case <-ctx.Done(): + return -1, ctx.Err() + case <-time.After(50 * time.Millisecond): + } + } +}