Repository navigation
fix(libkernel_sys): follow a symbolic link in link on Windows - #9336
oneandonlydean merged 5 commits into
Conversation
link_nid_postfix decided whether to follow the source with std::filesystem::is_symlink, which MinGW-w64's libstdc++ never reports for a Windows symbolic link, so on Windows link made a hard link to the link itself. Follow a reparse-point source to its final path with GetFinalPathNameByHandleW instead, and run the link-through-a-link case of guest_kernel_sys on Windows too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent Windows run of this PR (head a293868). Environment: Windows 11 10.0.26300, Ryzen 7 9800X3D, WinLibs GCC 15.2.0 r7 (MinGW), Release, BUILD_TESTING=ON. PowerShell not elevated, Developer Mode on. Results:
On the unelevated note in the description: my run also printed "symbolic links unavailable: the link cases not tested", but the new link-through-a-link cases still ran. The base failure at line 692 is inside that block, and that block's own skip message ("the symlink success cases not tested") was not printed. The "link cases not tested" message comes from the earlier readlink block, which creates its link with Not run: a symbolic link loop or a chain of links, Linux/macOS, the full repository CTest, and any title. AI-assisted: yes |
Resolves the conflict with boykopovar#8522 in link_nid_postfix: keep its ENOTDIR return for a destination below a regular file and the Windows symbolic-link follow after it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cadenasynyster thanks for the run and for the correction. You're right: Since #8522 landed in the meantime, I merged |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 8b3e398 and the Windows SDK 10.0.26100 UCRT source (ucrt/filesystem/stat.cpp):
- This merges cleanly with current main, and main still uses
is_symlinkhere, so the fix is still needed. - I think the loop → ELOOP case can't be reached.
link_nid_postfixrunsstd::filesystem::status(source)and returns ENOENT/ENOTDIR when it doesn't exist (Export.cpp:374-375), beforeFollowLinkPath(:381). On MinGW thatstatusis UCRT_wstat64, and when CreateFileW fails on a path that isn't a drive root, UCRT stat reports ERROR_FILE_NOT_FOUND (stat.cpp:378-388). So a loop comes back as ENOENT beforeFollowLinkPathruns. Example: guestsymlink("b","a"),symlink("a","b"),link("a","x")should give 62 (ELOOP) as on FreeBSD, but would give 2. CouldFollowLinkPathrun before the existence check on Windows, with a loop case added toguest_kernel_sys? I couldn't run this locally (Smart App Control blocked my probe), so a run on your side would confirm it. - For the same reason, the ERROR_FILE_NOT_FOUND → ENOENT branch only matters in a race. Dangling links still return ENOENT correctly through the earlier check.
- Minor: Windows follows up to 63 reparse hops and FreeBSD stops at 32 (MAXSYMLINKS), so a 33-63 link chain succeeds here where the console returns ELOOP. Fine to leave for the debt file if you prefer.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…indows Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@harisibrar1 thanks for reading it against the UCRT source. I ran your case here (Windows 11 10.0.26200, WinLibs GCC 15.2.0, elevated so the guest can create symbolic links): after The chain limit is real: a chain of 40 links is linked here, where FreeBSD stops at 32. I recorded it in the debt file (e9e345f). |
harisibrar1
left a comment
There was a problem hiding this comment.
Rechecked at e9e345f:
- Thanks for running it. I was wrong about the loop: I read UCRT's
_wstat64error path but didn't run it, and on a real loop it succeeds, so the existence check lets it through andFollowLinkPathreturnsELOOPas intended. Sorry for the noise. - The new Windows loop case in
guest_kernel_sys(37ba44c) pins that, and the debt line (e9e345f) records that longer chains link here, where FreeBSD stops atMAXSYMLINKS(32). Nothing left from my side.
Reviewed with AI assistance (Claude Code).
What
linkfollows a symbolic link source, as FreeBSD's does (7cba4ce). It decided that withstd::filesystem::is_symlink, and MinGW-w64's libstdc++ never reports a Windows symbolic link as one:symlink_statusof aCreateSymbolicLinkWlink withFILE_ATTRIBUTE_REPARSE_POINTset returns a regular file. So on Windowslinkmade a hard link to the symbolic link itself. The test for following is under#ifndef _WIN32, so nothing showed it.On Windows
linknow checks the source forFILE_ATTRIBUTE_REPARSE_POINT, asfchmodatin the same file does. It opens a reparse point source withCreateFileWwithoutFILE_FLAG_OPEN_REPARSE_POINTand links toGetFinalPathNameByHandleWof that handle. A dangling link returnsENOENTand a loop (ERROR_CANT_RESOLVE_FILENAME)ELOOP, instead of linking the reparse point. A source that resolves to a directory returnsEPERM, as before. Linux and macOS are unchanged.The link-through-a-link case of
guest_kernel_sysnow runs on Windows too, when symbolic links can be created. It usesdot-linkfor its link to., because the Windows part of the test already makes adirectory-linkearlier. On Windows it checks that the new name is not a reparse point, sinceis_symlinkcannot tell there.Tested
Windows 11 Home 10.0.26200, MinGW-w64 GCC 15.2.0 (ucrt-posix-seh r7), Release,
BUILD_TESTING=ON, onmain950388b:guest_kernel_sys, elevated so symbolic links can be created: passes 3 of 3.Export.cppfrommain: fails atGuestKernelSys.cpp:692(guest errno 2), the new name is a reparse point.symlinkfails there, so the new block is skipped, which the test shows withsymbolic links unavailable: the symlink success cases not tested. (Thethe link cases not testedline comes from an earlier block that usesstd::filesystem::create_symlink, which MinGW's libstdc++ does not implement.)mainwas merged into this branch (339d5c1). The only conflict was its ENOTDIR return on the destination line next to this change; both are kept.guest_kernel_syspasses 3 of 3 on the merge.symlink("loop-b", "loop-a")andsymlink("loop-a", "loop-b"),linkfromloop-areturnsELOOP(62)._wstat64succeeds on the loop, so the existence check lets it through, andCreateFileWthen fails withERROR_CANT_RESOLVE_FILENAME(1921).guest_kernel_syspasses 2 of 2 with it. A chain of 40 links is linked, where FreeBSD stops at 32; the debt file records it (e9e345f).symlink("owned.txt", "owned.link"), thenlink("owned.link", "other.txt"). Onmain,other.txtis a reparse point,owned.linkhas 2 links andowned.txt1. With this changeother.txtis a plain file andowned.txthas 2 links.python tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.py): fix(libkernel_sys): fail link, symlink, pathconf and fchmodat through a regular file with ENOTDIR #8522, now merged, is handled above. test: organize library tests and label CTest suites #6044 movesGuestKernelSys.cpp. fix(libkernel_sys): enforce readlink path length limit #5528 changes onlyreadlink, and feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021 and Macos port #7201 do not touchlink.Found while running #9103's new
linkattest on Windows; itsAT_SYMLINK_FOLLOWcase goes through thislink.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code