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) {