diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 0c46a564..558f78b7 100644 --- a/services/nvpair-engine-manager/install.go +++ b/services/nvpair-engine-manager/install.go @@ -159,7 +159,7 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { e.reporter.report(serviceError{ID: uninstallFailedID(engine), Message: err.Error(), Severity: "error", Action: "none", EngineType: engine, Operation: "uninstall"}) return err } - if st.plat.Runtime.modeOrDefault() == "process" && !isManagedInstallPath(binPath, st.installDir) { + if st.plat.Runtime.modeOrDefault() == "process" && !isOwnedInstallPath(binPath, st.installDir) { err := fmt.Errorf("cannot uninstall engine %q: its executable is outside NVPAIR's managed install directory (%s)", engine, binPath) e.reporter.report(serviceError{ID: uninstallFailedID(engine), Message: err.Error(), Severity: "error", Action: "none", EngineType: engine, Operation: "uninstall"}) return err diff --git a/services/nvpair-engine-manager/lifecycle.go b/services/nvpair-engine-manager/lifecycle.go index c0d40880..6ee8d7a2 100644 --- a/services/nvpair-engine-manager/lifecycle.go +++ b/services/nvpair-engine-manager/lifecycle.go @@ -397,7 +397,7 @@ func (e *Executor) doStop(st *engineState, engine string) error { // we manage :11435) is never touched; we decline only a process // whose image we can't confirm is ours. pid, image, ok := pidOnPort(port) - if ok && isManagedInstallPath(binPath, st.installDir) && isOurEngineImage(image, binPath) { + if ok && isOwnedInstallPath(binPath, st.installDir) && isOurEngineImage(image, binPath) { st.mu.Lock() st.stopping = true if st.healthStop != nil { diff --git a/services/nvpair-engine-manager/ownership_test.go b/services/nvpair-engine-manager/ownership_test.go new file mode 100644 index 00000000..d86b3499 --- /dev/null +++ b/services/nvpair-engine-manager/ownership_test.go @@ -0,0 +1,67 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func writeExecutable(t *testing.T, path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("x"), 0o755); err != nil { + t.Fatal(err) + } +} + +// Ownership must hold both as written and resolved, so a link at either end +// cannot make an external binary look like PAIR's. +func TestIsOwnedInstallPathRejectsLinksOut(t *testing.T) { + base := t.TempDir() + external := filepath.Join(base, "Applications", "Ollama.app", "ollama") + writeExecutable(t, external) + + // An install directory that is itself a link to the external location. + linkedDir := filepath.Join(base, "engine-bin", "ollama") + if err := os.MkdirAll(filepath.Dir(linkedDir), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(base, "Applications"), linkedDir); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if isOwnedInstallPath(external, linkedDir) { + t.Error("an external binary was owned through an install directory that links to it") + } + if isOwnedInstallPath(filepath.Join(linkedDir, "Ollama.app", "ollama"), linkedDir) { + t.Error("an external binary spelled through the linked install directory was owned") + } + + // A binary inside a real install directory that links out. + realDir := filepath.Join(base, "engine-bin", "other") + linkedBin := filepath.Join(realDir, "ollama") + if err := os.MkdirAll(realDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(external, linkedBin); err != nil { + t.Fatal(err) + } + if isOwnedInstallPath(linkedBin, realDir) { + t.Error("a binary linking out of the install directory was owned") + } + + // A real binary in a real install directory, however the temp root is + // spelled (on macOS it is reached through /var -> /private/var). + owned := filepath.Join(realDir, "Ollama.app", "ollama") + writeExecutable(t, owned) + if !isOwnedInstallPath(owned, realDir) { + t.Error("a binary inside the install directory was not owned") + } + if resolved, err := filepath.EvalSymlinks(owned); err == nil && !isOurEngineImage(resolved, owned) { + t.Error("the resolved running image did not match the configured path") + } +} diff --git a/services/nvpair-engine-manager/proc.go b/services/nvpair-engine-manager/proc.go index 8c92c773..07b4d427 100644 --- a/services/nvpair-engine-manager/proc.go +++ b/services/nvpair-engine-manager/proc.go @@ -135,8 +135,20 @@ func terminatePID(pid int, grace time.Duration) { // /proc//exe can suffix " (deleted)" when the file was replaced while // the process still runs. func normalizeEngineImage(path string) string { - path = strings.TrimSuffix(path, " (deleted)") - return filepath.Clean(path) + return filepath.Clean(strings.TrimSuffix(path, " (deleted)")) +} + +// canonicalEnginePath is normalizeEngineImage with symlinks resolved. macOS +// reports a running image by its resolved path (/private/var/...) while the +// configured path may go through a link (/var/...). Use it only to compare, +// never on its own to decide ownership: resolving an install directory that is +// itself a link would make whatever it points at look managed. +func canonicalEnginePath(path string) string { + path = normalizeEngineImage(path) + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return resolved + } + return path } // isOurEngineImage reports whether the listener on our managed port is @@ -149,7 +161,7 @@ func isOurEngineImage(image, binPath string) bool { if image == "" || binPath == "" { return false } - return strings.EqualFold(normalizeEngineImage(image), normalizeEngineImage(binPath)) + return strings.EqualFold(canonicalEnginePath(image), canonicalEnginePath(binPath)) } // isManagedInstallPath reports whether binPath is inside this engine's @@ -168,7 +180,39 @@ func isManagedInstallPath(binPath, installDir string) bool { if err != nil { return false } - rel, err := filepath.Rel(dirAbs, binAbs) + return pathUnder(dirAbs, binAbs) +} + +// isOwnedInstallPath is the ownership test for stopping or removing an engine: +// binPath must be inside the install directory both as written and with every +// link resolved. As written alone, a link inside the install directory could +// lead to an external binary; resolved alone, an install directory that is +// itself a link would claim whatever it points at. +func isOwnedInstallPath(binPath, installDir string) bool { + if !isManagedInstallPath(binPath, installDir) { + return false + } + // PAIR creates the install directory itself. One that is a link leads + // somewhere PAIR did not install into, so nothing reached through it is + // PAIR's, however the path is spelled. Links above it (/var on macOS) + // are the system's and are fine. + if info, err := os.Lstat(filepath.Clean(installDir)); err != nil || info.Mode()&os.ModeSymlink != 0 { + return false + } + binAbs, err := filepath.Abs(canonicalEnginePath(binPath)) + if err != nil { + return false + } + dirAbs, err := filepath.Abs(canonicalEnginePath(installDir)) + if err != nil { + return false + } + return pathUnder(dirAbs, binAbs) +} + +// pathUnder reports whether path is dir or inside it, by whole components. +func pathUnder(dir, path string) bool { + rel, err := filepath.Rel(dir, path) if err != nil || filepath.IsAbs(rel) || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return false } diff --git a/services/nvpair-engine-manager/proc_unix.go b/services/nvpair-engine-manager/proc_unix.go index 876e2b69..2618ebfb 100644 --- a/services/nvpair-engine-manager/proc_unix.go +++ b/services/nvpair-engine-manager/proc_unix.go @@ -8,6 +8,7 @@ package main import ( "context" "errors" + "log/slog" "os" "os/exec" "strconv" @@ -61,7 +62,11 @@ func pidOnPort(port int) (pid int, image string, ok bool) { func lsofPID(port int) (int, bool) { ctx, cancel := context.WithTimeout(context.Background(), portLookupTimeout) defer cancel() - out, err := exec.CommandContext(ctx, "lsof", "-nP", "-tiTCP:"+strconv.Itoa(port), "-sTCP:LISTEN").Output() + lsof := systemTool("lsof", lsofLocations) + if lsof == "" { + return 0, false + } + out, err := exec.CommandContext(ctx, lsof, "-nP", "-tiTCP:"+strconv.Itoa(port), "-sTCP:LISTEN").Output() if err != nil { return 0, false } @@ -77,7 +82,11 @@ func lsofPID(port int) (int, bool) { func ssPID(port int) (int, bool) { ctx, cancel := context.WithTimeout(context.Background(), portLookupTimeout) defer cancel() - out, err := exec.CommandContext(ctx, "ss", "-ltnp", "sport", "=", ":"+strconv.Itoa(port)).Output() + ss := systemTool("ss", ssLocations) + if ss == "" { + return 0, false + } + out, err := exec.CommandContext(ctx, ss, "-ltnp", "sport", "=", ":"+strconv.Itoa(port)).Output() if err != nil { return 0, false } @@ -97,19 +106,6 @@ func ssPID(port int) (int, bool) { return 0, false } -// procImage resolves a PID's executable path via /proc (Linux). macOS has no -// /proc, so it returns "" there and the image check is skipped — reclamation -// on macOS relies on the caller declining when the image can't be confirmed. -func procImage(pid int) string { - if pid <= 0 { - return "" - } - if path, err := os.Readlink("/proc/" + strconv.Itoa(pid) + "/exe"); err == nil { - return path - } - return "" -} - // signalPID sends SIGTERM (or SIGKILL when force) to the process group when // possible. It is the PID-addressed kill used only by the orphan reclaim (a // process we lost the *exec.Cmd handle to), distinct from the normal @@ -137,3 +133,24 @@ func pidAlive(pid int) bool { err := syscall.Kill(pid, 0) return err == nil || errors.Is(err, syscall.EPERM) } + +// lsofLocations and ssLocations are where the system installs each tool. The +// PID a lookup returns is the one a stop signals, so the tool is never found +// through PATH, where a directory the user or another program put first could +// supply a different binary. +var ( + lsofLocations = []string{"/usr/sbin/lsof", "/usr/bin/lsof", "/sbin/lsof", "/bin/lsof"} + ssLocations = []string{"/usr/sbin/ss", "/usr/bin/ss", "/sbin/ss", "/bin/ss"} +) + +// systemTool returns the first of locations that is an executable file, or "" +// when none is. +func systemTool(name string, locations []string) string { + for _, path := range locations { + if info, err := os.Stat(path); err == nil && info.Mode().IsRegular() && info.Mode()&0o111 != 0 { + return path + } + } + slog.Debug("system tool not found at a fixed location", "tool", name) + return "" +} diff --git a/services/nvpair-engine-manager/procimage_darwin.go b/services/nvpair-engine-manager/procimage_darwin.go new file mode 100644 index 00000000..e73efbeb --- /dev/null +++ b/services/nvpair-engine-manager/procimage_darwin.go @@ -0,0 +1,63 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin + +package main + +import ( + "context" + "os/exec" + "strconv" + "strings" +) + +// procImage resolves a PID's executable path with lsof. +// +// macOS has no /proc, and this decides whether a listener on PAIR's managed +// port is PAIR's own engine binary. Without it the check failed closed every +// time, so a stop or uninstall of an engine PAIR started but lost the handle to +// (after a crash, a force quit or a worker restart) was always refused. +// +// The `txt` descriptor is the running image as the kernel records it, not argv, +// so it is right for a process launched through a PATH lookup and cannot be +// spoofed by the process renaming itself. `ps -o comm=` is not a substitute: it +// reports a bare name in exactly that case. +// +// Empty on any error, so the caller's image check fails closed. +func procImage(pid int) string { + if pid <= 0 { + return "" + } + lsof := systemTool("lsof", lsofLocations) + if lsof == "" { + return "" + } + ctx, cancel := context.WithTimeout(context.Background(), portLookupTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, lsof, "-w", "-p", strconv.Itoa(pid), "-Fn", "-a", "-d", "txt") + // lsof escapes non-ASCII bytes in names unless the locale says UTF-8. + cmd.Env = []string{"LC_ALL=en_US.UTF-8"} + out, _ := cmd.Output() + return parseLsofImage(string(out)) +} + +// parseLsofImage extracts the executable from lsof's -F field output for txt +// descriptors: an `f` line names a descriptor and the `n` line after it is its +// path. The kernel lists the executable before the libraries it loaded, so the +// first txt path is the image. +func parseLsofImage(out string) string { + inText := false + for _, line := range strings.Split(out, "\n") { + switch { + case strings.HasPrefix(line, "f"): + inText = line == "ftxt" + case inText && strings.HasPrefix(line, "n"): + // Only a stray CR is stripped: a path really ending in a space is + // reported verbatim, and trimming it would let "/dir/engine " + // match the managed "/dir/engine" in a check that authorizes a kill. + return strings.TrimSuffix(line[1:], "\r") + } + } + return "" +} diff --git a/services/nvpair-engine-manager/procimage_darwin_test.go b/services/nvpair-engine-manager/procimage_darwin_test.go new file mode 100644 index 00000000..377f8cf8 --- /dev/null +++ b/services/nvpair-engine-manager/procimage_darwin_test.go @@ -0,0 +1,46 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func TestParseLsofImage(t *testing.T) { + out := "p123\nftxt\nn/Users/me/Library/engine-bin/ollama/Ollama.app/Contents/Resources/ollama\nftxt\nn/usr/lib/dyld\n" + if got := parseLsofImage(out); got != "/Users/me/Library/engine-bin/ollama/Ollama.app/Contents/Resources/ollama" { + t.Fatalf("image = %q", got) + } + if got := parseLsofImage("p123\nfcwd\nn/tmp\n"); got != "" { + t.Fatalf("a non-txt descriptor was taken as the image: %q", got) + } + if got := parseLsofImage("ftxt\nn/dir/engine \n"); got != "/dir/engine " { + t.Fatalf("a trailing space must survive, got %q", got) + } + if got := parseLsofImage(""); got != "" { + t.Fatalf("empty output gave %q", got) + } +} + +// The running test binary is a process whose image is known. +func TestProcImageResolvesARunningProcess(t *testing.T) { + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + want, err := filepath.EvalSymlinks(exe) + if err != nil { + t.Fatal(err) + } + if got := normalizeEngineImage(procImage(os.Getpid())); got != want { + t.Fatalf("procImage(self) = %q, want %q", got, want) + } + if got := procImage(0); got != "" { + t.Fatalf("procImage(0) = %q, want empty", got) + } +} diff --git a/services/nvpair-engine-manager/procimage_linux.go b/services/nvpair-engine-manager/procimage_linux.go new file mode 100644 index 00000000..22b41ee1 --- /dev/null +++ b/services/nvpair-engine-manager/procimage_linux.go @@ -0,0 +1,23 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package main + +import ( + "os" + "strconv" +) + +// procImage resolves a PID's executable path via /proc. Empty on any error, so +// the caller's image check fails closed. +func procImage(pid int) string { + if pid <= 0 { + return "" + } + if path, err := os.Readlink("/proc/" + strconv.Itoa(pid) + "/exe"); err == nil { + return path + } + return "" +} diff --git a/services/nvpair-engine-manager/procimage_other.go b/services/nvpair-engine-manager/procimage_other.go new file mode 100644 index 00000000..4a5ceb28 --- /dev/null +++ b/services/nvpair-engine-manager/procimage_other.go @@ -0,0 +1,11 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows && !linux && !darwin + +package main + +// procImage has no implementation on this platform. An empty image makes the +// caller's ownership check fail closed, so a stop of an engine PAIR lost the +// handle to is declined. +func procImage(int) string { return "" }