From 3cf1dfa046f9ae86fd104f5fb5e448ff758d31e9 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Wed, 9 Sep 2026 15:40:41 +0800 Subject: [PATCH 1/5] fix(daemon): recover stale service PID files safely --- pkg/service/daemon/daemon.go | 75 +++++++++++++++++++++++++------ pkg/service/daemon/daemon_test.go | 62 +++++++++++++++++++++---- 2 files changed, 115 insertions(+), 22 deletions(-) diff --git a/pkg/service/daemon/daemon.go b/pkg/service/daemon/daemon.go index 28ec0aa0b..88c492a1c 100644 --- a/pkg/service/daemon/daemon.go +++ b/pkg/service/daemon/daemon.go @@ -218,13 +218,21 @@ func validatePIDFileInfo(info os.FileInfo) error { return nil } -func servicePIDConflictError(pid int) error { - return fmt.Errorf( +type servicePIDMismatchError struct { + pid int +} + +func (e *servicePIDMismatchError) Error() string { + return fmt.Sprintf( "service PID file points to live process %d that does not match the Zaparoo service binary", - pid, + e.pid, ) } +func servicePIDConflictError(pid int) error { + return &servicePIDMismatchError{pid: pid} +} + // Running returns true if the service is running. func (s *Service) Running() (bool, error) { pid, err := s.Pid() @@ -237,7 +245,11 @@ func (s *Service) Running() (bool, error) { } if pidRunning(pid) { - if s.pidMatchesService(pid) { + matches, identityErr := s.serviceProcessIdentity(pid) + if identityErr != nil { + return false, fmt.Errorf("error identifying service PID %d: %w", pid, identityErr) + } + if matches { return true, nil } log.Warn(). @@ -1089,6 +1101,23 @@ func (s *Service) Start() error { defer release() running, err := s.Running() + var conflict *servicePIDMismatchError + if errors.As(err, &conflict) { + // Only Start may recover a confirmed foreign PID, under the same gate + // that protects PID publication. Never signal the unrelated process. + pid, pidErr := s.Pid() + if pidErr != nil { + return pidErr + } + if pid != conflict.pid { + return errors.New("service PID changed during start") + } + if removeErr := s.removePidFile(); removeErr != nil { + return removeErr + } + log.Warn().Int("pid", pid).Msg("removed stale service PID file without signaling unrelated process") + err = nil + } if err != nil { return err } @@ -1283,27 +1312,45 @@ func pidIsZombie(pid int) bool { } func (s *Service) pidMatchesService(pid int) bool { + matches, _ := s.serviceProcessIdentity(pid) + return matches +} + +// Unknown identity must not be treated as a confirmed PID reuse: removing its +// PID file could let Start create a second service when procfs is inaccessible. +func (s *Service) serviceProcessIdentity(pid int) (bool, error) { if runtime.GOOS != "linux" { - return true + return true, nil } dataDir := helpers.DataDir(s.pl) - exePath, err := os.Readlink(filepath.Join(procDir(), strconv.Itoa(pid), "exe")) - if err == nil && pathLooksLikeServiceBinary(exePath, dataDir) { - return true + exePath, exeErr := os.Readlink(filepath.Join(procDir(), strconv.Itoa(pid), "exe")) + exePath = strings.TrimSuffix(exePath, " (deleted)") + if exeErr == nil && pathLooksLikeServiceBinary(exePath, dataDir) { + return true, nil } cmdlinePath := filepath.Join(procDir(), strconv.Itoa(pid), "cmdline") cmdline, err := os.ReadFile(cmdlinePath) //nolint:gosec // reads process status for service management if err != nil { - return false + return false, fmt.Errorf("reading process command line: %w", err) } - for _, arg := range strings.Split(string(cmdline), "\x00") { - if pathLooksLikeServiceBinary(arg, dataDir) { - return true - } + if exeErr != nil { + return false, fmt.Errorf("reading process executable: %w", exeErr) + } + return serviceScriptIdentity(exePath, cmdline, dataDir), nil +} + +func serviceScriptIdentity(exePath string, cmdline []byte, dataDir string) bool { + // A data argument to cat/tail/etc. is not service identity. The supported + // shell-backed caches are executed as interpreter argv[1] by the shebang. + switch filepath.Base(exePath) { + case "sh", "bash", "dash", "busybox": + args := strings.Split(string(cmdline), "\x00") + return len(args) > 1 && pathLooksLikeServiceBinary(args[1], dataDir) + default: + return false } - return false } func procDir() string { diff --git a/pkg/service/daemon/daemon_test.go b/pkg/service/daemon/daemon_test.go index 651f072db..4451e8d0a 100644 --- a/pkg/service/daemon/daemon_test.go +++ b/pkg/service/daemon/daemon_test.go @@ -716,23 +716,69 @@ func TestRunningReturnsFalseForLiveUnrelatedPID(t *testing.T) { assert.FileExists(t, pidFile) } -func TestStartFailsForLiveUnrelatedPID(t *testing.T) { +func TestStartRecoversLiveUnrelatedPID(t *testing.T) { requireLinuxProc(t, "service PID identity checks") svc := newTestService(t) - settings := svc.pl.Settings() - pidFile := filepath.Join(settings.TempDir, config.PidFile) + pidFile := filepath.Join(svc.pl.Settings().TempDir, config.PidFile) + eventLog := filepath.Join(t.TempDir(), "events.log") + t.Setenv(config.AppEnv, writeFakeServiceScript(t, pidFile, eventLog)) process := exec.CommandContext(context.Background(), "sleep", "1000") require.NoError(t, process.Start()) - t.Cleanup(func() { _ = process.Process.Kill() }) + t.Cleanup(func() { + _ = process.Process.Kill() + _ = process.Wait() + }) require.NoError(t, os.WriteFile(pidFile, []byte(strconv.Itoa(process.Process.Pid)), 0o600)) + t.Cleanup(func() { + pid, err := svc.Pid() + if err == nil && pid > 0 && pid != process.Process.Pid { + require.NoError(t, svc.Stop()) + } + }) - err := svc.Start() + // Competing ensures must recover the stale file and publish only one PID. + const racers = 4 + results := make(chan error, racers) + for range racers { + go func() { results <- svc.Start() }() + } + for range racers { + require.NoError(t, <-results) + } + pid, err := svc.Pid() + require.NoError(t, err) + assert.NotEqual(t, process.Process.Pid, pid) + assert.True(t, requireServiceRunning(t, svc)) + assert.True(t, pidRunning(process.Process.Pid), "unrelated process must not be signaled") + + // An idempotent ensure after recovery must retain the same service. + require.NoError(t, svc.Start()) + nextPID, err := svc.Pid() + require.NoError(t, err) + assert.Equal(t, pid, nextPID) +} + +func TestServiceScriptIdentityRejectsDataArguments(t *testing.T) { + t.Parallel() + + dataDir := t.TempDir() + service := filepath.Join(dataDir, "zaparoo.0123456789abcdef.sh") + assert.True(t, serviceScriptIdentity("/bin/sh", []byte("sh\x00"+service+"\x00-service\x00exec\x00"), dataDir)) + assert.True(t, serviceScriptIdentity("/bin/busybox", []byte("sh\x00"+service+"\x00"), dataDir)) + assert.False(t, serviceScriptIdentity("/bin/tail", []byte("tail\x00"+service+"\x00"), dataDir)) + assert.False(t, serviceScriptIdentity("/bin/sh", []byte("sh\x00-c\x00"+service+"\x00"), dataDir)) +} + +func TestUnavailableProcessIdentityIsNotPIDConflict(t *testing.T) { + requireLinuxProc(t, "service PID identity checks") + + matches, err := newTestService(t).serviceProcessIdentity(-1) require.Error(t, err) - assert.Contains(t, err.Error(), "does not match the Zaparoo service binary") - assert.True(t, pidRunning(process.Process.Pid)) - assert.FileExists(t, pidFile) + assert.False(t, matches) + var conflict *servicePIDMismatchError + assert.NotErrorAs(t, err, &conflict, "unknown identity must not authorize PID-file removal") } func TestRestartFailsForLiveUnrelatedPID(t *testing.T) { From 45d7349d660c1331145f8bcaa4d786412f0c0213 Mon Sep 17 00:00:00 2001 From: Callan Barrett Date: Thu, 10 Sep 2026 07:38:45 +0800 Subject: [PATCH 2/5] fix(daemon): identify shell-run services past a shebang option The kernel places the interpreted script after any shebang option, so "#!/bin/sh -e" puts the option at argv[1] and the script at argv[2]. Reading only argv[1] reported a live shell-backed service as foreign, and Start now removes a foreign PID file, so the wrapper would have started a second service against the running one. Scan for the first argument that is not a flag instead, and keep -c and -s rejected so a path in a command string stays data rather than identity. Resolved interpreters on buildroot images are also ash, ksh or zsh rather than a busybox symlink, so accept those. --- pkg/service/daemon/daemon.go | 32 +++++++++++++++++++++++----- pkg/service/daemon/daemon_test.go | 35 +++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/pkg/service/daemon/daemon.go b/pkg/service/daemon/daemon.go index 88c492a1c..4b3102b47 100644 --- a/pkg/service/daemon/daemon.go +++ b/pkg/service/daemon/daemon.go @@ -233,6 +233,15 @@ func servicePIDConflictError(pid int) error { return &servicePIDMismatchError{pid: pid} } +// IsStalePIDConflict reports whether err is the PID-file conflict that Start +// clears under the start gate. Callers that auto-start the service must treat +// it as "not running" and let Start recover it, otherwise a reused PID leaves +// the service unstartable from the wrapper. +func IsStalePIDConflict(err error) bool { + var conflict *servicePIDMismatchError + return errors.As(err, &conflict) +} + // Running returns true if the service is running. func (s *Service) Running() (bool, error) { pid, err := s.Pid() @@ -1342,15 +1351,28 @@ func (s *Service) serviceProcessIdentity(pid int) (bool, error) { } func serviceScriptIdentity(exePath string, cmdline []byte, dataDir string) bool { - // A data argument to cat/tail/etc. is not service identity. The supported - // shell-backed caches are executed as interpreter argv[1] by the shebang. + // A data argument to cat/tail/etc. is not service identity, so only a known + // interpreter can vouch for a shell-backed cache. switch filepath.Base(exePath) { - case "sh", "bash", "dash", "busybox": - args := strings.Split(string(cmdline), "\x00") - return len(args) > 1 && pathLooksLikeServiceBinary(args[1], dataDir) + case "sh", "bash", "dash", "ash", "busybox", "ksh", "zsh": default: return false } + // The kernel places the script after any shebang option, so the interpreted + // script is the first argument that is not a flag. Misreading it as foreign + // would let Start delete a live service's PID file and start a second one. + for _, arg := range strings.Split(string(cmdline), "\x00")[1:] { + if arg == "-c" || arg == "-s" { + // The program text comes from the next argument or stdin, so a + // matching path after this point is data rather than the script. + return false + } + if strings.HasPrefix(arg, "-") { + continue + } + return pathLooksLikeServiceBinary(arg, dataDir) + } + return false } func procDir() string { diff --git a/pkg/service/daemon/daemon_test.go b/pkg/service/daemon/daemon_test.go index 4451e8d0a..cb7aefd4f 100644 --- a/pkg/service/daemon/daemon_test.go +++ b/pkg/service/daemon/daemon_test.go @@ -30,6 +30,7 @@ package daemon import ( "context" + "errors" "fmt" "io/fs" "net" @@ -714,6 +715,12 @@ func TestRunningReturnsFalseForLiveUnrelatedPID(t *testing.T) { assert.Contains(t, runningErr.Error(), "does not match the Zaparoo service binary") assert.True(t, pidRunning(process.Process.Pid)) assert.FileExists(t, pidFile) + // The platform wrappers auto-start on this error instead of aborting, so + // Start can clear the stale file. Without this the recovery is unreachable + // from the only entry point users have. + assert.True(t, IsStalePIDConflict(runningErr)) + assert.False(t, IsStalePIDConflict(nil)) + assert.False(t, IsStalePIDConflict(errors.New("some other failure"))) } func TestStartRecoversLiveUnrelatedPID(t *testing.T) { @@ -769,6 +776,34 @@ func TestServiceScriptIdentityRejectsDataArguments(t *testing.T) { assert.True(t, serviceScriptIdentity("/bin/busybox", []byte("sh\x00"+service+"\x00"), dataDir)) assert.False(t, serviceScriptIdentity("/bin/tail", []byte("tail\x00"+service+"\x00"), dataDir)) assert.False(t, serviceScriptIdentity("/bin/sh", []byte("sh\x00-c\x00"+service+"\x00"), dataDir)) + assert.False(t, serviceScriptIdentity("/bin/sh", []byte("sh\x00-s\x00"+service+"\x00"), dataDir)) + // A data argument after the script is not identity either. + assert.False(t, serviceScriptIdentity( + "/bin/sh", []byte("sh\x00/opt/other.sh\x00"+service+"\x00"), dataDir)) +} + +// The kernel puts a shebang option at argv[1] and the script after it, so an +// interpreter list that only reads argv[1] reports a live service as foreign +// and lets Start remove its PID file and start a second one. +func TestServiceScriptIdentityAcceptsShebangOptionAndBusyboxShells(t *testing.T) { + t.Parallel() + + dataDir := t.TempDir() + service := filepath.Join(dataDir, "zaparoo.0123456789abcdef.sh") + + // "#!/bin/sh -e" => argv = [sh, -e,