Fix infinite recursion in project discovery on symlink cycles - #4421
Fix infinite recursion in project discovery on symlink cycles#4421michaelglass wants to merge 6 commits into
Conversation
ProjectFile.SearchAllProjectRelatedFiles walks the directory tree to find *proj* files, following directory symlinks with no cycle detection. A self-referential or cyclic directory symlink — e.g. the macOS SDK ncurses symlink loops inside a Nix `.devenv` profile — drives the walk into unbounded recursion, so `paket restore` hangs (surfacing downstream as "project.assets.json stale ... 'dotnet restore' timed out after 300s"). Resolve each directory to its canonical, symlink-followed path and skip ones already visited. Symlinks are still followed (legitimately symlinked project directories are still discovered) — only repeats are pruned, so cycles terminate and each physical project is reported once. Canonicalization uses libc `realpath` on Unix (Paket.Core targets net461/netstandard2.0, which lack DirectoryInfo.ResolveLinkTarget) with a lexical Path.GetFullPath fallback on Windows and on any failure. Adds a regression test: red without the fix (the project is found 32x before the path outgrows PATH_MAX), green with it (found exactly once). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| // UTF-8 bytes so non-ASCII paths round-trip, and a 4096-byte buffer (>= PATH_MAX | ||
| // on Linux (4096) and macOS (1024)), so no manual free is required. | ||
| [<DllImport("libc", EntryPoint = "realpath", SetLastError = true)>] | ||
| extern nativeint realpath(byte[] path, byte[] resolved) |
There was a problem hiding this comment.
Is there no way to do this fix using dotnet primitives?
There was a problem hiding this comment.
Not on Paket.Core's targets: FileSystemInfo.ResolveLinkTarget/LinkTarget are .NET 6+, and Paket.Core builds net461 + netstandard2.0, so there is no managed way to canonicalise a path there.
What netstandard2.0 does have is the reparse-point attribute, and a cycle can only arise through a link. So 89237a1 drops the P/Invoke entirely and just never recurses into symlinked directories (via the existing SymlinkUtils.isDirectoryLink). Net diff to src is now +2 lines. The trade is that a project reachable only through a directory symlink is no longer discovered, which matches git and most build tools.
There was a problem hiding this comment.
hmm. I think dropping symlinks altogether is probably a regression ...
There was a problem hiding this comment.
Reworked in 00b174c. Skipping links altogether was the wrong trade, so links are followed again and cycles are cut by a visited set keyed on a canonical path. The primitive is FileSystemInfo.ResolveLinkTarget(returnFinalTarget = true), bound by reflection since it is .NET 6+ and Paket.Core compiles for netstandard2.0 (adding a net10.0 target trips 17 unrelated overload-ambiguity errors). Only a runtime without it, i.e. Mono, falls back to libc realpath.
There was a problem hiding this comment.
... sorry I've gone back and forth on this and I think it just needs an executive decision. What do you think?
either we detect cycles in symlinks (retain current behavior but fix a bug, implies keeping libc fallback for linux hosts? or dropping net461?)
or we stop following symlinks altogether for package discovery (this changes paket behavior, but also incidentally fixes #3411 (no reliance on modern dotnet features)
There was a problem hiding this comment.
🟡 Changes recommended
The new native realpath P/Invoke signature should mark the output buffer as [<Out>], and the new regression test should follow the repo’s existing timeout/ignore gating to avoid unsupported timeouts on non-net461 targets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an infinite-recursion risk in Paket’s project discovery by detecting and pruning directory cycles introduced by symlink loops during recursive project-file search.
Changes:
- Add cycle detection to the recursive directory walk used by
ProjectFile.FindAllProjectFiles. - Introduce
realPath(symlink-resolving canonicalization on Unix with safe fallback) to support cycle detection. - Add a regression test that creates a symlink loop and asserts discovery terminates and de-duplicates results.
File summaries
| File | Description |
|---|---|
src/Paket.Core/PaketConfigFiles/ProjectFile.fs |
Tracks visited canonical directories during recursive search to avoid symlink-cycle recursion. |
src/Paket.Core/Common/Utils.fs |
Adds realPath (Unix realpath interop + fallback) used to canonicalize directories for cycle detection. |
tests/Paket.Tests/ProjectFile/SymlinkLoopSpecs.fs |
New regression test that creates a symlink loop and asserts discovery terminates and finds the project exactly once. |
tests/Paket.Tests/Paket.Tests.fsproj |
Includes the new regression test file in the test project compile order. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| [<DllImport("libc", EntryPoint = "realpath", SetLastError = true)>] | ||
| extern nativeint realpath(byte[] path, byte[] resolved) |
There was a problem hiding this comment.
Added [<Out>] in e42914b. For the record: byte[] is blittable, so the marshaller pins the array and passes its address, and the write was already visible; the attribute documents direction rather than changing behaviour.
There was a problem hiding this comment.
Moot as of 89237a1: the P/Invoke is gone, the walk now skips symlinked directories via the reparse-point attribute instead.
There was a problem hiding this comment.
Back as of 00b174c, as the Mono-only fallback, with [<Out>].
| [<Test>] | ||
| [<Timeout(120000)>] | ||
| let ``FindAllProjectFiles terminates on symlink cycles and finds each project once`` () = |
There was a problem hiding this comment.
Kept the unconditional [<Timeout>]. The NO_UNIT_TIMEOUTATTRIBUTE gating predates NUnit's netstandard2.0 build: the pinned NUnit 3.12 implements TimeoutAttribute on .NET Core (runs the test on a Task and fails it after the deadline). Verified here on net10.0: a probe test sleeping 4 s under [<Timeout(1000)>] fails after 1 s, and the Linux CI job ran this test (Passed, 25 ms). Gating it with Ignore would skip the regression on the only platform where the scenario exists; Windows already Assert.Ignores it.
Also checked the failure mode with the cycle check disabled: the test fails in ~1 s (project found 32 times; the walk stops once the path exceeds PATH_MAX) rather than hanging, so the timeout is only a safety net.
The native realpath call writes into the resolved buffer; the P/Invoke signature now declares that direction explicitly.
Paket.Core targets netstandard2.0 and net461, which have no managed way to read a symlink target, so the previous fix P/Invoked realpath. A cycle can only arise through a link, so not recursing into reparse-point directories (SymlinkUtils.isDirectoryLink) is enough to keep the walk finite, with no native code.
The DirectoryInfo from GetDirectories already carries its attributes, so read them directly instead of re-stat-ing through SymlinkUtils, and run the check after the free name comparisons. Apply the same guard to Utils.FindAllFiles, the other hand-rolled recursive walk over the paket root. Flatten the test's Windows guard and stop swallowing cleanup errors.
8720152 to
65a42c8
Compare
…as fallback Skipping symlinked directories dropped projects that are only reachable through a link. Both walks now follow links and key a visited set on a canonical path instead. Paket.Core compiles for netstandard2.0, so FileSystemInfo.ResolveLinkTarget (.NET 6+) is bound by reflection at run time; a Unix runtime without it falls back to libc realpath, and Windows on .NET Framework keeps the lexical path. A second test pins that a symlink to a directory outside the root is still followed.
What went wrong
ProjectFile.FindAllProjectFilesrecurses through every subdirectory, following symlinks. A cyclic directory symlink (for example the macOS SDK ncurses links inside a Nix.devenvprofile) sends that walk round the loop until the path outgrows PATH_MAX, re-finding every project on each lap.Utils.FindAllFiles(used byconvert-from-nuget) has the same shape and the same exposure.Fix
Both walks keep following symlinks and key a visited set on a canonical path, so each physical directory is entered once. Discovered files keep the path they were reached by.
Canonicalisation: Paket.Core compiles for net461 and netstandard2.0, and
FileSystemInfo.ResolveLinkTargetis .NET 6+, so it is bound by reflection once at startup and used when the runtime has it (thepakettool on net10.0, and any consumer on .NET 6+). A Unix runtime without it (Mono) falls back to libcrealpath. Windows on .NET Framework keeps the lexical path, as today.Tests
sub/loop -> ..under a temp root: discovery terminates and reports the project once. Without the visited set it reports it 32 times on macOS.Both pass with the reflection path and with the
realpathfallback forced.