From 16db9c0e40efacf0f0bea4fc4a6c8d546c4da953 Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Mon, 2 Nov 2026 16:15:39 +0200 Subject: [PATCH 1/3] fix(engine-manager): stop a leftover engine on macOS Before terminating an engine it lost the process handle to (after a crash, a force quit or a worker restart), the engine manager confirms the process on its managed port is the binary it manages. The image lookup read /proc, which macOS does not have, so the check failed closed every time and a stop or uninstall of PAIR's own Ollama was refused with "running under external management"; the reclaim tests skipped on macOS for the same reason. macOS now resolves the image from lsof's txt descriptor, the kernel's record of the running executable. lsof and ss are run from fixed system locations, never through PATH, since the PID they return is the one a stop signals. Both sides of the ownership comparison resolve symlinks, because macOS reports /private/var/... for a configured /var/.... Adapted from NVIDIA/Personal-AI-Router#136 (issues NVIDIA/Personal-AI-Router#17 and NVIDIA/Personal-AI-Router#47). Co-authored-by: Terve Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- services/nvpair-engine-manager/proc.go | 15 ++++- services/nvpair-engine-manager/proc_unix.go | 47 +++++++++----- .../nvpair-engine-manager/procimage_darwin.go | 63 +++++++++++++++++++ .../procimage_darwin_test.go | 46 ++++++++++++++ .../nvpair-engine-manager/procimage_linux.go | 23 +++++++ .../nvpair-engine-manager/procimage_other.go | 11 ++++ 6 files changed, 188 insertions(+), 17 deletions(-) create mode 100644 services/nvpair-engine-manager/procimage_darwin.go create mode 100644 services/nvpair-engine-manager/procimage_darwin_test.go create mode 100644 services/nvpair-engine-manager/procimage_linux.go create mode 100644 services/nvpair-engine-manager/procimage_other.go diff --git a/services/nvpair-engine-manager/proc.go b/services/nvpair-engine-manager/proc.go index 8c92c7739..87570d455 100644 --- a/services/nvpair-engine-manager/proc.go +++ b/services/nvpair-engine-manager/proc.go @@ -135,8 +135,14 @@ 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) + path = filepath.Clean(strings.TrimSuffix(path, " (deleted)")) + // macOS reports a running image by its resolved path (/private/var/...) + // while the configured path may go through a symlink (/var/...), so + // compare both sides resolved. + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return resolved + } + return path } // isOurEngineImage reports whether the listener on our managed port is @@ -168,6 +174,11 @@ func isManagedInstallPath(binPath, installDir string) bool { if err != nil { return false } + // binPath is compared resolved (see normalizeEngineImage), so the install + // directory must be too. + if resolved, err := filepath.EvalSymlinks(dirAbs); err == nil { + dirAbs = resolved + } rel, err := filepath.Rel(dirAbs, binAbs) 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 876e2b693..2618ebfb2 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 000000000..e73efbeb2 --- /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 000000000..377f8cf8e --- /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 000000000..22b41ee17 --- /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 000000000..4a5ceb289 --- /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 "" } From cb11f217d2d875bc62a471cc8558a931a7f092a9 Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Mon, 2 Nov 2026 20:16:10 +0200 Subject: [PATCH 2/3] fix(engine-manager): resolve links only to compare images, not to grant ownership Review follow-up. Resolving the install directory in the shared containment helper let an install directory that is itself a link claim whatever it points at, and turned the documented-lexical cli_roots coverage into a resolved one. Containment is lexical again. Stopping or removing an engine now uses isOwnedInstallPath, which requires the binary inside the install directory both as written and with links resolved, and only the running image comparison resolves links (for macOS /var vs /private/var). Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- services/nvpair-engine-manager/install.go | 2 +- services/nvpair-engine-manager/lifecycle.go | 2 +- .../nvpair-engine-manager/ownership_test.go | 64 +++++++++++++++++++ services/nvpair-engine-manager/proc.go | 46 ++++++++++--- 4 files changed, 102 insertions(+), 12 deletions(-) create mode 100644 services/nvpair-engine-manager/ownership_test.go diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 0c46a5649..558f78b7c 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 c0d40880d..6ee8d7a21 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 000000000..68e858572 --- /dev/null +++ b/services/nvpair-engine-manager/ownership_test.go @@ -0,0 +1,64 @@ +// 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") + } + + // 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 87570d455..a016ca560 100644 --- a/services/nvpair-engine-manager/proc.go +++ b/services/nvpair-engine-manager/proc.go @@ -135,10 +135,16 @@ 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 = filepath.Clean(strings.TrimSuffix(path, " (deleted)")) - // macOS reports a running image by its resolved path (/private/var/...) - // while the configured path may go through a symlink (/var/...), so - // compare both sides resolved. + 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 } @@ -155,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 @@ -174,12 +180,32 @@ func isManagedInstallPath(binPath, installDir string) bool { if err != nil { return false } - // binPath is compared resolved (see normalizeEngineImage), so the install - // directory must be too. - if resolved, err := filepath.EvalSymlinks(dirAbs); err == nil { - dirAbs = resolved + 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 } - rel, err := filepath.Rel(dirAbs, binAbs) + 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 } From 79cdb7dc531efedb8fd1c231569a3db542ae5ae1 Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Mon, 2 Nov 2026 20:33:35 +0200 Subject: [PATCH 3/3] fix(engine-manager): never own an engine through a linked install directory Review follow-up. A binary spelled through an install directory that is itself a link passed both containment checks. PAIR creates that directory, so a link there leads somewhere PAIR did not install into; ownership is now refused for it. Links above it (/var on macOS) still work. Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- services/nvpair-engine-manager/ownership_test.go | 3 +++ services/nvpair-engine-manager/proc.go | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/services/nvpair-engine-manager/ownership_test.go b/services/nvpair-engine-manager/ownership_test.go index 68e858572..d86b3499b 100644 --- a/services/nvpair-engine-manager/ownership_test.go +++ b/services/nvpair-engine-manager/ownership_test.go @@ -37,6 +37,9 @@ func TestIsOwnedInstallPathRejectsLinksOut(t *testing.T) { 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") diff --git a/services/nvpair-engine-manager/proc.go b/services/nvpair-engine-manager/proc.go index a016ca560..07b4d4272 100644 --- a/services/nvpair-engine-manager/proc.go +++ b/services/nvpair-engine-manager/proc.go @@ -192,6 +192,13 @@ 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