Repository navigation
fix(libkernel): unlink a dangling Windows directory link - #12820
oneandonlydean merged 1 commit into
Conversation
|
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.
|
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — comments
The change is a small, well-reasoned reordering of the Windows directory-symlink branch in sceKernelUnlink, and I found nothing blocking. The edited code is entirely inside #ifdef _WIN32, though, so this Linux host can neither compile it nor run it, and I cannot approve on what I was able to observe.
What I ran
- Head:
21e632b9ea89383dff4b513e961ec48480b02586 main:ba5b5330b37724bca4b99f30faab504653635a04(the PR is based on this commit)- Host: Arch Linux 7.2.11-zen, GCC 16.2.1, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08). Release, Ninja,
BUILD_TESTING=ON, in a clean worktree.
git fetch -q origin main pull/12820/head:refs/review/12820
git checkout -q --detach origin/main
git merge -q --no-ff --no-edit refs/review/12820
git diff --check origin/main...refs/review/12820
python3 tools/check_conventions.py --base origin/main
nice -n 10 cmake --build build -j10 --target libkernel guest_filesystem_tests guest_kernel_sys_tests guest_kernel_rename_tests guest_posix_filesystem_tests guest_directory_tests
nice -n 10 ctest --test-dir build -j4 --output-on-failure -R '^(guest_filesystem|guest_kernel_sys|guest_kernel_rename|guest_posix_filesystem|guest_directory)$'
git rev-parse 2257ffde7b^1Results
| Check | Result |
|---|---|
Merge into main |
Clean |
git diff --check |
Clean |
tools/check_conventions.py --base origin/main |
All conventions checks passed. |
| Build (libkernel and the five test executables, Linux) | OK |
guest_filesystem, guest_kernel_sys, guest_kernel_rename, guest_posix_filesystem, guest_directory (Linux) |
5/5 passed |
| The Windows block is compiled on this host | No: it is inside #ifdef _WIN32, so the Linux results only show that the non-Windows path is unchanged |
| The description's bisect claim | 2257ffde7b is the merge of #9473 (“fix(libkernel): rmdir and unlink on a Windows directory link”), and its first parent is 430270f510, as stated. I did not reproduce the bisect itself. |
ctest output
1/5 Test #582: guest_directory .................. Passed 0.00 sec
2/5 Test #2215: guest_posix_filesystem ........... Passed 0.00 sec
3/5 Test #2417: guest_kernel_rename .............. Passed 0.00 sec
4/5 Test #399: guest_kernel_sys ................. Passed 0.03 sec
5/5 Test #2406: guest_filesystem ................. Passed 0.05 sec
100% tests passed out of 5
Negative control
None. The PR changes no tests. The existing check at GuestFilesystem.cpp:1522 is the regression test, and it only runs on Windows hosts that can create symbolic links. A Linux run cannot exercise it.
Findings
Blocking
None.
Non-blocking
- Newly reachable: wildcard characters in the last path component.
Open.cpp:491-504now callsFindFirstFileW(native)on every Windows unlink. Before this PR it only ran oncesymlink_statushad reported an existing directory.FindFirstFileWtreats*and?as wildcards. I did not find any escaping of those characters in the path resolution. Take a guestunlink("…/foo*")with a directory symlinkfoobarnext to it: the search matchesfoobar, the attribute and tag checks pass, and thenRemoveDirectoryW(L"…\\foo*")fails with an error other thanERROR_ACCESS_DENIED. The call therefore returnsEIO(5) instead of falling through to theENOENTpath. Nothing is deleted, so the only effect is the wrong errno.sceKernelRmdiralready detects links without a wildcard-capable API:Stdio.cpp:1652usesFile::FillLinkStat, andStdio.cpp:1657-1661usesGetFileAttributesWwithREPARSE_POINT | DIRECTORY. Using the same approach here, withGetFileAttributesWfor the two attribute bits andFile::WindowsSymbolicLink::Read(WindowsSymbolicLink.hpp:64-79returnsfalseunless the tag isIO_REPARSE_TAG_SYMLINK), or comparingfound.cFileNamewithnative.filename(), would keep the old exposure. - Same failure class remains for dangling junctions. Your diagnosis is that MinGW's
symlink_statusfollows the link. If that holds, a junction whose target is missing also skips the junction branch atOpen.cpp:505-515and reachesNativeUnlink. It would then fail withEACCESjust as the dangling symbolic link did. This is outside the PR's scope and is not a regression. Since theDirectoryLinkcheck now sits before thesymlink_statustest, it might be worth handlingIO_REPARSE_TAG_MOUNT_POINTthere as well in a follow-up. If not, record the gap indocs/dev/debt/libkernel.md. - Also pre-existing and unchanged in behaviour: a file symbolic link (no
FILE_ATTRIBUTE_DIRECTORY) that points at an existing directory still enters theis_directorybranch, failsReadJunctionand returnsEPERM. POSIXunlinkwould remove the link. Before this PR it returnedEACCES/EIOfromRemoveDirectoryW, so neither version removes it. Noting it only for completeness.
Not verified
- I compiled nothing inside
#ifdef _WIN32and ran nothing on Windows. That includes theguest_filesystemdirectory-link block, the description's 3/3 pass on 10.0.22621, and the full Windows ctest run. - The libstdc++/MinGW
symlink_statusbehaviour described in the table: I did not reproduce it. - Why the check passed for #9473's author on 10.0.26300.
To get this to a good state
- Ideally a second Windows run confirms the fix, for example Windows CI or another contributor with symlink rights. Otherwise this needs a maintainer who trusts the description's Windows results.
- Optionally address finding 1 so the directory-link probe cannot match a different entry through wildcards.
|
@XairRrR thanks, finding 1 is real, and I reproduced it on Windows. Finding 1 (wildcards): the probe now runs only when the last component contains none of Require(unlink_nid_postfix((root / "directory-li*").string().c_str()) == -1 && *__error_nid_postfix() != 5 && (GetFileAttributesW(directoryLink.c_str()) & FILE_ATTRIBUTE_REPARSE_POINT) != 0);One correction to the expected errno: with the probe skipped, the call doesn't reach
Finding 2 (dangling junctions): agreed, same class. I'll check a dangling junction on Windows and send it as a separate PR, or as a debt entry if the fix isn't clean. That's to keep this one to the #9473 regression. Finding 3: noted. A file symlink pointing at a directory is unchanged by this PR. AI-assisted: yes (Claude Code). |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — comments
1d6c5f549f skips the FindFirstFileW probe when the last component holds a search-pattern character and adds a regression check for that case. That resolves my wildcard finding as far as I can tell from reading the code. Nothing is blocking. All of the changed code is still inside #ifdef _WIN32, though, so this Linux host cannot compile or run it, and I am leaving a comment rather than an approval for the same reason as last time.
What changed since my last review
- New commit
1d6c5f549f("fix(libkernel): keep unlink's directory-link probe off wildcard names"):Open.cpp:492now runs the probe only whennative.filename()contains none of*?<>".GuestFilesystem.cpp:1522adds a check to the Windows directory-link block. It unlinksdirectory-li*whiledirectory-linkstill exists, and requires that the call fails, that errno is notEIO, and that the link is still a reparse point.
- The author replied to findings 1–3, and the description now covers the wildcard guard and its Windows results.
What I ran
- Head:
1d6c5f549f285766ade936771c12c75da5d56ebf main:ba5b5330b37724bca4b99f30faab504653635a04- Host: Arch Linux 7.2.11-zen, GCC 16.2.1, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08). Release, Ninja,
BUILD_TESTING=ON, in a clean worktree.
git fetch -q origin main pull/12820/head:refs/review/12820
git diff 21e632b9ea89383dff4b513e961ec48480b02586 refs/review/12820
git checkout -q --detach origin/main
git merge -q --no-ff --no-edit refs/review/12820
git diff --check origin/main...refs/review/12820
python3 tools/check_conventions.py --base origin/main
nice -n 10 cmake --build build -j10 --target libkernel guest_filesystem_tests guest_kernel_sys_tests guest_kernel_rename_tests guest_posix_filesystem_tests guest_directory_tests
nice -n 10 ctest --test-dir build -j4 --output-on-failure -R '^(guest_filesystem|guest_kernel_sys|guest_kernel_rename|guest_posix_filesystem|guest_directory)$'| Check | Result |
|---|---|
Merge into main |
Clean |
git diff --check |
Clean |
tools/check_conventions.py --base origin/main |
All conventions checks passed. |
| Build (libkernel and the five test executables, Linux) | OK |
guest_filesystem, guest_kernel_sys, guest_kernel_rename, guest_posix_filesystem, guest_directory (Linux) |
5/5 passed |
| Negative control for the new check | Not possible here: the check is inside #ifdef _WIN32, and this host has no MinGW toolchain or Windows headers |
ctest output
1/5 Test #582: guest_directory .................. Passed 0.00 sec
2/5 Test #2215: guest_posix_filesystem ........... Passed 0.00 sec
3/5 Test #2417: guest_kernel_rename .............. Passed 0.00 sec
4/5 Test #399: guest_kernel_sys ................. Passed 0.03 sec
5/5 Test #2406: guest_filesystem ................. Passed 0.05 sec
100% tests passed out of 5
Previous findings
- Wildcards in the last component reach
FindFirstFileW: Resolved. The guard covers*and?, and also the three DOS wildcards that the file system interprets (<DOS_STAR,>DOS_QM,"DOS_DOT). None of these five can appear in a Windows file name, so the guard never skips a real link.FindFirstFileWdoes not accept wildcards in earlier components, so checking only the filename is enough. The author reports that the new check fails without the guard (guest errno 5, Win32 error 123) and passes with it on Windows 11 10.0.22621. I accept that as author-reported, because I cannot run it. Your correction is right: with the probe skipped, the call falls back tomain's pre-PR behaviour (EINVALfromDeleteFileW), not toENOENTas I wrote. Asserting "notEIO, link untouched" rather than a specific host errno is reasonable. - Dangling junctions: Still open, deferred. The author plans to handle it in a separate PR, or as a
docs/dev/debt/libkernel.mdentry. It is not a regression from this PR, so it does not block. - File symlink pointing at a directory: No longer applies. It was pre-existing and noted only for completeness. The behaviour is unchanged.
New findings
Blocking
None.
Non-blocking
None.
Not verified
- Nothing inside
#ifdef _WIN32was compiled or run, neither the guard inOpen.cppnor the newGuestFilesystem.cppcheck. All Windows results in the description and the reply are author-reported. - The negative control for the new check, for the same reason.
To get this to a good state
- The only thing missing is independent Windows confirmation, from Windows CI or a second contributor with symlink rights. Failing that, a maintainer who accepts the author's Windows results. I have no code changes to request.
|
@XairRrR finding 2 is confirmed on Windows and now has its own PR, #12835 (stacked on this one). A dangling junction reports AI-assisted: yes (Claude Code). |
|
Why AI-assisted: yes (Claude Code). |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — comments
No code has changed since my last review. You have now moved the dangling-junction case to #12835 and explained why main CI missed the regression. I checked that explanation against the CI logs and it holds. It also means CI cannot give the independent Windows confirmation I asked for, so this stays a comment.
What changed since my last review
- No new commits. The head is still
1d6c5f549f. mainmoved fromba5b5330b3to5454cd2e45. That range includes #12103, which adds rename checks to the same test file,GuestFilesystem.cpp, in a different block.- Two replies from you:
- Finding 2 (dangling junctions) is confirmed on Windows and is now #12835, "fix(libkernel): unlink a dangling Windows junction", stacked on this PR.
mainCI ranguest_filesystemon Windows Server 2025 (build 26100). That build does not show the dangling directory-link failure, so CI cannot catch this regression.
What I ran
- Head:
1d6c5f549f285766ade936771c12c75da5d56ebf main:5454cd2e455994d992aca39335ce60507eeda1eb- Merged tree:
9e6ec3b7dc05d965a8cf393a7b69414182978869 - Host: Arch Linux 7.2.11-zen, GCC 16.2.1, Intel Core i9-14900KF, NVIDIA GeForce RTX 4070 Ti SUPER (driver 615.78.08). Release, Ninja,
BUILD_TESTING=ON, in a clean worktree.
git fetch -q origin main pull/12820/head:refs/review/12820
git checkout -q --detach origin/main
git merge -q --no-ff --no-edit refs/review/12820
git diff --check origin/main...refs/review/12820
python3 tools/check_conventions.py --base origin/main
nice -n 10 cmake --build build -j10 --target libkernel guest_filesystem_tests guest_kernel_sys_tests guest_kernel_rename_tests guest_posix_filesystem_tests guest_directory_tests
nice -n 10 ctest --test-dir build -j4 --output-on-failure -R '^(guest_filesystem|guest_kernel_sys|guest_kernel_rename|guest_posix_filesystem|guest_directory)$'
gh run view 40861541628 -R boykopovar/AnyPS5 --log --job 123415127398
gh api repos/boykopovar/AnyPS5/actions/jobs/123431964067 --jq '.steps[]|"\(.number) \(.conclusion) \(.name)"'
gh run view 40869073863 -R boykopovar/AnyPS5 --log --job 123431964067| Check | Result |
|---|---|
Merge into current main |
Clean (GuestFilesystem.cpp auto-merged with #12103's rename checks) |
git diff --check |
Clean |
tools/check_conventions.py --base origin/main |
All conventions checks passed. |
| Build (libkernel and the five test executables, Linux) | OK |
| The five related suites (Linux) | 5/5 passed |
| Your CI explanation | Confirmed. The windows job of the main push run for ba5b5330b3 ran on windows-2025-vs2026, OS 10.0.26100, and guest_filesystem passed there. |
This PR's own windows check |
Green, but it compiled and ran nothing from this PR (see below) |
ctest output (merged tree)
2/5 Test #2232: guest_posix_filesystem ........... Passed 0.00 sec
3/5 Test #2434: guest_kernel_rename .............. Passed 0.00 sec
4/5 Test #402: guest_kernel_sys ................. Passed 0.02 sec
5/5 Test #2423: guest_filesystem ................. Passed 0.06 sec
100% tests passed out of 5
CI excerpts
main push run 40861541628, windows job, head ba5b5330b3:
Microsoft Windows Server 2025
10.0.26100
Image: windows-2025-vs2026
2503/3000 Test #2510: guest_filesystem .... Passed 0.30 sec
100% tests passed, 0 tests failed out of 3000
This PR's run 40869073863, windows job, head 1d6c5f549f, steps:
17 success Run if exist configure-seed.cmake (cmake ... -S . -B build ...)
24 skipped Run if exist build\select.ninja (ninja -C build -f select.ninja selected) else (cmake --build build)
26 success Link changed host-only core/libs test executables
27 skipped Run cmake --build build --target libs
28 skipped Run python tools/select_tests.py ctest build ...
Step 26 printed No changed host-only core/libs test executables require a link check.
Previous findings
- Wildcards in the last component reach
FindFirstFileW: Resolved in my last review. No code changed since then. - Dangling junctions: Moved to #12835. You report that a dangling junction gets
EACCESand that #12835's new check fails with this PR'sOpen.cpp. I accept both as author-reported. I have not reviewed #12835. Leaving it out of this PR keeps this one to the #9473 regression, and it does not block here. - File symlink pointing at a directory: No longer applies. No change.
New findings
Blocking
None.
Non-blocking
- This PR's green
windowscheck does not compile the change. Onpull_requestevents, the build,libsand ctest steps of thewindowsjob are gated ongithub.event_name != 'pull_request'(build.yml:693-714). The job only configures and runs the host-only link check. That check found nothing to link for this PR. So no CI job has compiledOpen.cpporGuestFilesystem.cppwith MinGW at this head. A maintainer should not read the green check as Windows evidence. The first CI compile will be themainpush run after the merge, on build 26100, where the regression does not show. I am not asking for a change in this PR. It only means the Windows results in the description are the only evidence that the#ifdef _WIN32code builds and passes.
Not verified
- Nothing inside
#ifdef _WIN32was compiled or run, here or by this PR's CI. All Windows results, including the 3-of-3 passes on 10.0.22621 and the negative control for thedirectory-li*check, are author-reported. - I could not tell whether the directory-link block ran or took its skip path on the 26100 runner. Like you, I could not see it in the log, because ctest prints output only for failing tests.
- #12835 itself.
To get this to a good state
- I have no code changes to request. What is missing is an independent run of
guest_filesystemon a Windows 11 22H2/23H2 (10.0.22621) machine with symlink rights, or a maintainer who accepts your Windows results. A CI run on 26100 cannot replace that, as your own analysis shows.
|
Linux run at this PR's exact head Arch Linux, kernel 7.2.9, Ryzen 9 7950X, g++ (GCC) 16.2.1 20260810, glibc 2.44, x86-64. Release,
The fix and the added checks are all in Windows-only blocks; this run shows the Linux build and |
|
Refreshed on current
With these two merged, the only other known Windows CPU failure here is the intermittent AI-assisted: yes (Claude Code). |
|
Rechecked on current
AI-assisted: yes (Claude Code). |
1d6c5f5 to
79e6d32
Compare
|
@XairRrR Refreshed at exact linear head Linux focused groups: guest_filesystem, guest_kernel_sys, guest_kernel_rename, guest_posix_filesystem, guest_directory. 15 passes, zero skips; public headers and conventions pass. The raw production module, test executables and current NID patcher built, and the exact production patch command passed. MinGW syntax passed for the affected production/test files. Source proofs retain main’s fixtures and gitlinks; there are no merge commits in Earlier runtime and negative-control reports remain pinned to their original trees. These new results are Linux CPU/MinGW only. Fresh Windows validation remains separate and pending; no new native title/GPU/performance or unsupported-path behavior claim. AI-assisted: yes (Codex). Immutable source, command and test evidence, SHA-256 The existing Windows owner explicitly acknowledged the finite exact-child validation request; it remains pending behind that host’s existing FIFO work. |
What
guest_filesystemfailed on the measured Windowsmainrevisions recorded below. The first bad commit is the merge of #9473 (2257ffde7b); its first-parent predecessor430270f510passes. Found by a full Windows ctest run and a first-parent bisect over the 64 merges betweenedf6804cf5and1db077e876.The failing check is the last one in #9473's Windows directory-link block (
GuestFilesystem.cpp:1522):Require(unlink_nid_postfix(directoryLink.string().c_str()) == 0 && unlink_nid_postfix(danglingDirectoryLink.string().c_str()) == 0);with guest errno 13 (
EACCES). The cause is the dangling directory link.sceKernelUnlinkenters its directory-symlink branch only whenstd::filesystem::is_directory(std::filesystem::symlink_status(native))is true. MinGW's libstdc++symlink_statusfollows the link instead of describing it, so on this host:symlink_statusFindFirstFileW0x410, tag0xa000000c(symlink)RemoveDirectoryWsucceedsnot_found, error 20x410, tag0xa000000c(symlink)DeleteFileWfails withERROR_ACCESS_DENIED->EACCES(Measured with a standalone program built with the same WinLibs toolchain.
RemoveDirectoryWremoves the dangling link fine.)This PR moves #9473's directory-symlink check ahead of the
symlink_statustest. It now requiresFILE_ATTRIBUTE_DIRECTORYas well as the reparse point andIO_REPARSE_TAG_SYMLINKfromFindFirstFileW, so a directory symbolic link is recognised from its own attributes whether or not its target exists. File symbolic links (noFILE_ATTRIBUTE_DIRECTORY) still go throughNativeUnlink, and junctions keep the existing branch inside the directory test. Only the#ifdef _WIN32code moves; other platforms are unchanged.Because the probe now runs for every Windows unlink, a second commit (from review) skips it when the last component contains
*?<>".FindFirstFileWwould otherwise treat those as search patterns and match a different entry:unlink("…/directory-li*")next todirectory-linkreturnedEIO. A new check in the directory-link block covers that case.#9473's author measured on Windows 11 10.0.26300 against an older base; I don't know why it passed there. On 10.0.22621 it fails every run.
Tested
Latest qualification (2026-11-01): exact linear head
79e6d32c050fd04aeb746ec34bc280446e4e144c, tested main93dbd0f82aea684e95f15f06e7d42c0064af18cf. Preserves main’s GetFileAttributesW directory-link classification and existing junction fallback, while keeping the earlier wildcard-guarded FindFirstFileW symlink probe. All filesystem fixtures are the automatic union of main and the branch.Linux CPU: 5 focused groups, 15 passes, zero skips; public headers, conventions, raw production-library build and production NID patch commands passed. Two affected translation units passed MinGW syntax. No new Windows runtime, native title, console SDK, GPU or performance claim. Existing Windows observations retain their exact historical pins. Fresh Windows runtime qualification remains separate.
Earlier pinned qualifications follow.
Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
BUILD_TESTING=ON; this machine can create symbolic links without elevation, so #9473's link block runs (not the skip path).mainba5b5330b3:guest_filesystemfails (exit0xc0000409).ba5b5330b3:guest_filesystempasses 3 of 3 (ctest --repeat until-fail:3), and so doguest_kernel_sys,guest_kernel_rename,guest_posix_filesystemandguest_directory, the related suites fix(libkernel): rmdir and unlink on a Windows directory link #9473 used.guest_job_manager_worker_count(fixed by fix(libSceJobManager): spin idle workers without taking the manager lock #12085) andguest_freebsd_clocks(fixed by fix(tests): drop the guest_freebsd_clocks check that the host zone is UTC #12660). Both fail onmainwithout this change too;guest_filesystemis no longer among the failures.1d6c5f549f): the newdirectory-li*check passes, and fails without the guard (guest errno 5, Win32 error 123);guest_filesystemand the four related suites pass 3 of 3.tools/check_conventions.pypasses.#ifdef _WIN32), and Windows 10.0.26300.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