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

CONTRIBUTING.md

@@ -62,6 +62,26 @@ PerformanceStudio/
 - No unnecessary abstractions — keep it simple and direct
 - Tests use real `.sqlplan` XML fixtures, not mocks
 
+## Calling native code
+
+**Never call a variadic C function through a plain `DllImport`.** `fcntl`, `open`, `ioctl` and the
+`printf` family all take `...`, and on Apple arm64 variadic arguments are passed on the *stack* while
+a fixed-signature P/Invoke passes them in *registers*. The callee reads a stack slot you never wrote.
+
+This does not throw, and it does not return an error. In #441 `fcntl(F_GETPATH)` returned 0 for
+success and wrote up to 1KB through whatever pointer happened to be in that slot — an arbitrary write
+into the process on every call — while the buffer we passed came back empty. It made `dotnet test`
+unrunnable on macOS for months, sometimes as a GC livelock and sometimes as a deadlock, and CI never
+saw any of it because Linux and Windows take different branches.
+
+Use a non-variadic equivalent instead (`proc_pidfdinfo` in place of `fcntl(F_GETPATH)`, for example).
+`__arglist` is not an escape hatch — it throws `Vararg calling convention not supported` on this
+target.
+
+When you do P/Invoke a struct-returning native call, derive the offsets in a comment from the system
+header rather than leaving magic numbers, and check the returned size against what you expected. A
+layout change should fail loudly, not hand back plausible-looking wrong bytes.
+
 ## Adding Analysis Rules
 
 Rules live in `PlanAnalyzer.cs`. Each rule:

src/PlanViewer.Cli/ReplSurface/OpenedFilePathResolver.cs

@@ -7,7 +7,31 @@ namespace PlanViewer.Cli.ReplSurface;
 
 internal static class OpenedFilePathResolver
 {
-    private const int MacOsGetPath = 50;
+    /* macOS resolves an open handle back to its path through libproc rather than fcntl(F_GETPATH),
+       because fcntl(2) is VARIADIC - int fcntl(int, int, ...) - and a variadic function cannot be
+       called correctly through a plain DllImport on Apple arm64. There the variadic arguments are
+       passed on the STACK while a fixed-signature P/Invoke puts them in registers, so the callee
+       reads a stack slot we never wrote. It does not fail: fcntl returns 0 for success and writes up
+       to MAXPATHLEN bytes through whatever pointer that slot happened to hold, which is an arbitrary
+       ~1KB write on every call. The buffer we passed comes back empty. See #441.
+
+       proc_pidfdinfo has a fixed signature, so the ordinary marshalling is correct. */
+    private const int ProcPidFdVNodePathInfo = 2;
+
+    /* Offset of vip_path within struct vnode_fdinfowithpath, and the struct's total size, both from
+       <sys/proc_info.h>:
+
+         vnode_fdinfowithpath { proc_fileinfo pfi; vnode_info_path vip; }
+         proc_fileinfo   = 4 + 4 + 8 + 4 + 4                    =   24
+         vnode_info      = vinfo_stat(136) + 4 + 4 + fsid_t(8)  =  152   -> vip_path begins at 176
+         vnode_info_path = 152 + MAXPATHLEN(1024)               = 1176
+         total           = 24 + 1176                            = 1200
+
+       The size is checked against the call's return value rather than trusted, so a layout change in
+       a future macOS fails loudly here instead of quietly handing back the wrong bytes - which is the
+       failure mode this whole file just came out of. */
+    private const int VNodePathInfoSize = 1200;
+    private const int VipPathOffset = 176;
 
     public static string GetFinalPath(FileStream stream)
     {
@@ -53,13 +77,28 @@ private static string GetWindowsPath(SafeFileHandle handle)
 
     private static string GetMacOsPath(SafeFileHandle handle)
     {
-        var buffer = new byte[4096];
-        if (Fcntl(handle.DangerousGetHandle().ToInt32(), MacOsGetPath, buffer) != 0)
+        var buffer = new byte[VNodePathInfoSize];
+        var written = ProcPidFdInfo(
+            Environment.ProcessId,
+            handle.DangerousGetHandle().ToInt32(),
+            ProcPidFdVNodePathInfo,
+            buffer,
+            buffer.Length);
+
+        if (written <= 0)
             throw CreateNativeIOException("Could not resolve the opened macOS file handle.");
-        var terminator = Array.IndexOf(buffer, (byte)0);
+        if (written != VNodePathInfoSize)
+            throw new IOException(
+                $"Unexpected vnode_fdinfowithpath size {written}; expected {VNodePathInfoSize}.");
+
+        var terminator = Array.IndexOf(buffer, (byte)0, VipPathOffset);
         if (terminator < 0)
-            terminator = buffer.Length;
-        return Path.GetFullPath(Encoding.UTF8.GetString(buffer, 0, terminator));
+            throw new IOException("Opened macOS file handle resolved to an unterminated path.");
+        if (terminator == VipPathOffset)
+            throw new IOException("Opened macOS file handle resolved to an empty path.");
+
+        return Path.GetFullPath(
+            Encoding.UTF8.GetString(buffer, VipPathOffset, terminator - VipPathOffset));
     }
 
     private static IOException CreateNativeIOException(string message) =>
@@ -72,6 +111,11 @@ private static extern uint GetFinalPathNameByHandle(
         uint filePathLength,
         uint flags);
 
-    [DllImport("libc", EntryPoint = "fcntl", SetLastError = true)]
-    private static extern int Fcntl(int fileDescriptor, int command, byte[] buffer);
+    [DllImport("libproc", EntryPoint = "proc_pidfdinfo", SetLastError = true)]
+    private static extern int ProcPidFdInfo(
+        int processId,
+        int fileDescriptor,
+        int flavor,
+        byte[] buffer,
+        int bufferSize);
 }

tests/PlanViewer.Core.Tests/OpenedFilePathResolverTests.cs

@@ -0,0 +1,71 @@
+using PlanViewer.Cli.ReplSurface;
+
+namespace PlanViewer.Core.Tests;
+
+/// <summary>
+/// #441: the macOS resolver called fcntl(F_GETPATH) through a plain DllImport. fcntl(2) is variadic,
+/// and on Apple arm64 variadic arguments are passed on the stack while a fixed-signature P/Invoke
+/// puts them in registers — so the callee read a stack slot we never wrote.
+///
+/// It did not fail. fcntl returned 0 for success and wrote up to MAXPATHLEN bytes through whatever
+/// pointer that slot happened to hold, an arbitrary ~1KB write on every call, while the buffer we
+/// passed came back empty.
+///
+/// Nothing caught it, and that is the interesting part: the old test asserted on the returned
+/// handle's Label and passed on Linux and Windows, which take entirely different branches. So this
+/// asserts the one thing that was actually broken — that the resolver returns the real path of the
+/// file that is genuinely open — and it asserts it on whatever platform the suite is running on.
+/// </summary>
+public class OpenedFilePathResolverTests
+{
+    [Fact]
+    public void GetFinalPath_ReturnsTheRealPathOfTheOpenHandle()
+    {
+        var path = Path.Combine(Path.GetTempPath(), $"resolver-{Guid.NewGuid():N}.sqlplan");
+        File.WriteAllText(path, "<ShowPlanXML />");
+        try
+        {
+            using var stream = new FileStream(path, FileMode.Open, FileAccess.Read);
+
+            var resolved = OpenedFilePathResolver.GetFinalPath(stream);
+
+            /* Compared by identity rather than by string, because macOS answers with the canonical
+               path — Path.GetTempPath() reports /var/folders/... while the kernel reports
+               /private/var/folders/..., and /var is a symlink to /private/var. A string comparison
+               here would fail for a reason that has nothing to do with the defect. */
+            Assert.True(File.Exists(resolved), $"Resolver returned a path that does not exist: '{resolved}'");
+            Assert.Equal(
+                new FileInfo(path).Length,
+                new FileInfo(resolved).Length);
+            Assert.Equal(
+                Path.GetFileName(path),
+                Path.GetFileName(resolved));
+        }
+        finally
+        {
+            File.Delete(path);
+        }
+    }
+
+    /// <summary>
+    /// The empty string is precisely what the broken call produced, and it is worth pinning that it
+    /// can never be mistaken for a valid answer: an empty path would sail through the extension and
+    /// containment checks in McpPlanPathPolicy as a Path.GetFullPath argument exception rather than
+    /// as a denial.
+    /// </summary>
+    [Fact]
+    public void GetFinalPath_NeverReturnsAnEmptyPath()
+    {
+        var path = Path.Combine(Path.GetTempPath(), $"resolver-{Guid.NewGuid():N}.sqlplan");
+        File.WriteAllText(path, "<ShowPlanXML />");
+        try
+        {
+            using var stream = new FileStream(path, FileMode.Open, FileAccess.Read);
+            Assert.False(string.IsNullOrWhiteSpace(OpenedFilePathResolver.GetFinalPath(stream)));
+        }
+        finally
+        {
+            File.Delete(path);
+        }
+    }
+}