Stop corrupting memory resolving an open file handle on macOS (#441)

[erikdarlingdata]

Fixes #441. dotnet test now finishes on macOS.

The defect

fcntl(2) is variadic — int fcntl(int, int, ...) — and this called it through a plain DllImport with a fixed third parameter:

[DllImport("libc", EntryPoint = "fcntl", SetLastError = true)]
private static extern int Fcntl(int fileDescriptor, int command, byte[] buffer);

On Apple arm64 the variadic ABI differs from AAPCS64: named arguments go in registers, variadic arguments go on the stack. A fixed-signature P/Invoke puts the buffer in x2, and the callee never looks there.

It does not fail, which is why nothing caught it. Standalone repro, outside the test suite, one open file:

Arch  : Arm64
FIXED rc=0 errno=0 len=0 -> ''

rc=0 is fcntl reporting success. F_GETPATH on success writes the path into the buffer it was handed; ours came back empty — so it wrote up to MAXPATHLEN bytes through whatever pointer happened to be in that stack slot. An arbitrary ~1KB write, on every call, into this process.

That is the entirety of #441. The suite wedged sometimes as a GC-suspension livelock spinning a core and sometimes as an all-threads-blocked deadlock, which is exactly what an arbitrary write into runtime memory looks like from outside. Both shapes are gone.

Diagnosed as a prediction, not a story

The three tests in McpPlanPathPolicyTests that pass are precisely the three that never reach GetFinalPath; both that reach it hung, deterministically — 5/5 and 2/2. I checked determinism explicitly because I had assumed the opposite.

It is also why CI never saw it: Linux takes the /proc/self/fd branch, Windows takes GetFinalPathNameByHandle. Only macOS goes near fcntl, and only arm64 has the mismatch — ubuntu-latest structurally cannot reproduce it.

The fix

libproc's proc_pidfdinfo answers the same question with a fixed signature, so ordinary marshalling is correct. Repairing the fcntl declaration in place is not available: .NET has no varargs P/Invoke on this target — __arglist throws Vararg calling convention not supported, which I confirmed.

Correcting myself: I recommended fstat/stat device+inode comparison on the issue, and it is wrong. stat() re-resolves the path, so if a symlink was swapped before the open it follows the swap too and the inodes match — it detects nothing. Resolving the descriptor back to its real path is the only thing that answers "where does this handle actually live", which is the question the TOCTOU check exists to ask.

Struct offsets are derived in a comment from <sys/proc_info.h> rather than being magic numbers, and the returned size is checked against the expected 1200 rather than trusted — so a future macOS layout change fails loudly instead of quietly handing back wrong bytes, which is the failure mode this file just came out of.

Note macOS answers with the canonical path (/private/var/… where Path.GetTempPath() reports /var/…). Roots are already canonicalized through ResolveLinkTarget, so containment still matches; the new test compares by identity rather than by string for the same reason.

Security note

GetFinalPath is the TOCTOU re-validation — confirming the path the kernel really opened is still inside the advertised roots. On macOS arm64 it has never performed that check. It failed closed (the empty string reached Path.GetFullPath, which throws ArgumentException, which is not in OpenAsync's catch filter), so this was not an exploitable bypass — but the guarantee OpenAsync_ValidatesAndReturnsTheSameOpenedHandle claims to prove was not being provided, and the FileStream leaked on the way out.

Tests

The old test asserted on the returned handle's .Label and passed happily on Linux and Windows while this call had never once worked on Apple silicon. The new test asserts the thing that was actually broken — that the resolver returns the real path of the file that is genuinely open — on whatever platform it runs.

	before	after
full dotnet test on macOS ARM64	never finished	16s, 305 passed, 0 failed, 2 skipped (twice)
McpPlanPathPolicyTests	5/5 hangs	3/3 passes, 3s each

dotnet build clean; the 9 warnings are the pre-existing MCP9005 obsolete-API uses in McpSmokeTests.cs.

Rule this earns

No variadic libc function through a plain DllImport in this repo — fcntl, open, ioctl, the printf family. They look fine on x64 and silently corrupt memory on Apple silicon.

🤖 Generated with Claude Code

[claude]

Reviewed. This is a solid, well-scoped fix — verified the struct-layout math for vnode_fdinfowithpath against XNU's <sys/proc_info.h> and it checks out: proc_fileinfo = 24 bytes, vinfo_stat = 136 bytes, vip_path offset = 176, total = 1200. PROC_PIDFDVNODEPATHINFO = 2 is correct.

A few things I confirmed while reviewing, no changes needed:

• proc_pidfdinfo is called through an ordinary fixed-signature DllImport, so the variadic-ABI mismatch that caused the original bug doesn't apply here — the byte[] buffer marshals normally.
• The new failure paths (written <= 0, size mismatch, unterminated path, empty path) all throw IOException, which stays inside the catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) filter in McpPlanPathPolicy.OpenAsync (src/PlanViewer.Cli/ReplSurface/McpPlanPathPolicy.cs:68) — so the stream still gets disposed on any resolver failure instead of leaking.
• tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs compiles against the internal OpenedFilePathResolver via the existing InternalsVisibleTo in PlanViewer.Cli.csproj, and the new tests assert on the actual behavior that was broken (real resolved path / non-empty result) rather than the old test's .Label check that happened not to exercise the macOS branch at all.

One observation, not a blocker: none of the workflows in .github/workflows/ run on macos-latest (only ubuntu-latest and windows-latest), which is consistent with the PR's own explanation of why this went undetected — but it also means this fix has no CI coverage going forward and could regress silently again. Worth a follow-up issue if you want a macOS job, but out of scope for this PR.

[claude]

Reviewed. This is a solid, well-contained fix.

• Verified the vnode_fdinfowithpath layout math independently against <sys/proc_info.h> (proc_fileinfo=24, vinfo_stat=136, vnode_info=152, vip_path offset=176, total=1200) — all constants in the PR match.
• GetMacOsPath's new failure paths (written <= 0, size mismatch, unterminated path, empty path) all throw IOException, which is exactly what McpPlanPathPolicy.OpenAsync already catches (catch (Exception exception) when (exception is IOException or UnauthorizedAccessException)), so the stricter validation degrades to "deny this candidate" rather than an unhandled crash — good, since this path directly feeds the TOCTOU/containment check for MCP-opened plan files.
• No other DllImport("libc", ...) variadic calls remain in the repo.
• Diff is scoped to OpenedFilePathResolver.cs + new tests + CONTRIBUTING.md; no T-SQL, versioned project, or Blazor-linked-file concerns apply here.
• Windows/Linux code paths are untouched.

No findings to flag.