Repository navigation
fix(libc): rename onto or from a Windows directory symbolic link like a link - #9661
Gruzzberry wants to merge 3 commits into
Conversation
|
Independent Windows check of db944e4, on a second machine: Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 r7 (UCRT), Ninja, Release,
Not tested: Linux, a console, a title run. |
|
Thanks @JJenkx for the second-machine run and the junction check. Added in 2b62d4d: on Windows the same three calls now also run on two junctions made with |
|
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.
|
|
Reviewed; the semantic change itself is right for every reachable case: file → link-to-dir becomes a link-over-file rename instead of One nit: in the success path, after |
There was a problem hiding this comment.
The Windows-only reparse-tag check is narrowly scoped to symbolic links and mount-point junctions, and leaves other reparse directories intact. The reported table and added file/directory-link cases cover the relevant replacement directions; the second-machine junction run adds useful coverage. Exact-head diff-check and conventions pass; hosted conventions/eligibility/labels/report plus relinker Linux/Windows/macOS and ASan jobs pass. Separately, the test cleanup nit noted in the thread remains: after the link/file replacement assertions, the later RemoveDirectoryW calls for movingLink and directoryLink target paths already moved or removed; they are harmless no-ops and can be dropped.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
|
Holding this as a draft, following the direction on #5239: the open queue is above 700 and no named title needs this change on current |
2b62d4d to
528fa6a
Compare
|
Thanks @fogydes and @MarcZX10. The cleanup is fixed in 528fa6a: on success only the link now named |
MarcZX10
left a comment
There was a problem hiding this comment.
The latest head fixes the stale-path cleanup nit: success and skipped-link paths now remove only entries that still exist. I fetched the current PR head 528fa6ab and independently reran git diff --check upstream/main...HEAD and python3 tools/check_conventions.py --base upstream/main --head HEAD; both pass. Hosted conventions, eligibility, labels/report, relinker Linux/macOS/Windows matrix, ASan, register-context and write-watch checks also pass on this head.
The PR's junction and Windows rename behavior remains supported by the author and independent Windows runs; the current hosted Windows product job is skipped, and I did not rerun that Windows-only test locally. I found no new code issue in the latest diff. The PR remains draft per your stated hold, so this is a review update rather than a readiness recommendation.
|
I’ve noticed that asking you to hold this PR was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. |
… a link FreeBSD 11.4's rename does not follow the last component of either path, so a symbolic link is renamed or replaced as a link whatever it points to. Under MinGW std::filesystem::symlink_status reports a Windows directory symbolic link or junction as a directory, so rename refused a file onto a link to a directory with EISDIR, refused a link to a directory onto a file with ENOTDIR, and replaced an empty directory with such a link instead of failing with EISDIR. RenamePath now treats a symbolic link or junction, recognised by its reparse tag, as a non-directory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rename change recognises junctions by IO_REPARSE_TAG_MOUNT_POINT but the test only made directory symbolic links. The same three calls now also run on two junctions made with mklink /J, which needs no Developer Mode, so the cases run where symbolic links cannot be created. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On success the moved link and the replaced link are already gone, so only the link now named rename-occupied.txt is removed there; the links and files a skipped run may leave behind are removed in that branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
528fa6a to
d1b9c7c
Compare
|
Rebased onto |
The cleanup nit is fixed (confirmed in the follow-up review) and remains fixed on the rebased head d1b9c7c, and the hold request was withdrawn.
What
On Windows,
renametreated a symbolic link to a directory (or a junction) as the directory it names. FreeBSD 11.4'skern_renameatlooks up the last component of both paths withoutFOLLOW, so a symbolic link is renamed or replaced as the link itself, whatever it points to. Measured onmaind6eafa5 with a file, an empty directory and two links to a directory:main(Windows)rename(file, link to a directory)EISDIRrename(link to a directory, existing file)ENOTDIRrename(link to a directory, empty directory)EISDIRThe cause is
RenamePath_nid_no_patch: under MinGW,std::filesystem::symlink_statusreports a Windows directory symbolic link as a directory instead of a link.sourceIsDirectoryanddestinationIsDirectorynow also require that the path is not a symbolic link or a junction, recognised by the reparse tag (IO_REPARSE_TAG_SYMLINK,IO_REPARSE_TAG_MOUNT_POINT). Other reparse points, such as cloud placeholder folders, are still directories. Everything after that decision is unchanged, includingRenameNative_nid_no_patch, which already opens the source withFILE_FLAG_OPEN_REPARSE_POINT. Linux reports links as links and is unchanged.Tested
Windows 11 (10.0.26200), WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release,
-DBUILD_TESTING=ON, onmaind6eafa5, Developer Mode on so links can be created.guest_filesystem: new checks after the existingrenameones, with a directory, an empty directory, two directory links and two files: the three calls in the table give the FreeBSD results, the replaced file's contents and the linked directory's contents are checked, and the links are removed afterwards (withRemoveDirectoryWon Windows, sinceunlinkof a directory link is fix(libkernel): remove a Windows directory junction without emptying its target #7530's area). Without link permission those checks are skipped with a message. On Windows the same three calls also run on two junctions made withmklink /J(absolute target), which needs no Developer Mode, so they run on every Windows machine (suggested by @JJenkx, who checked junctions locally first). Onmainthe test fails at the first new check (rename(link to a directory, empty directory)returns 0), and with the symbolic-link cases skipped it fails at the first junction check; with this PR it passes, three runs out of three.file_streams,guest_kernel_rename,guest_rename_dot,guest_kernel_sync,guest_kernel_sys,guest_kqueue,guest_path_case,guest_path_name_length,guest_posix_scalar_io(the other tests that callrename): pass.maind6eafa5 and onmainwith this PR merged together with feat(libkernel_sys): implement lchflags #9442, fix(libkernel): fail chflags through a dangling Windows symbolic link with ENOENT #9469, fix(libc): follow a Windows symbolic link in access #9474, fix(libkernel): follow a Windows symbolic link in chmod #9487 and fix(libkernel): fail open with O_CREAT and O_EXCL on a Windows symbolic link #9496, same machine: 1890 tests each, identical results (1869 passed, 18 skipped; on both, the 3agc_driver_sample_order*tests fail andagc_driver_image_array_window_past_layer_limitis not built, as its 64 KiBalignasis rejected by this MinGW toolchain).python tools/check_conventions.py --base upstream/main: passes.main; see the comment.std::filesystem::create_directory_symlink), a console, a title run.Overlaps
tools/pr_overlap.py --branchlists no direct overlap; a trial merge with fix(libc): report ELOOP from rename through a Windows symlink loop #8696 (renamethrough a link loop, also inRename.cpp) is clean.sameFilecheck a few lines below the two changed lines inRenamePath_nid_no_patch, without changing the directory decision; whichever lands second may need a text-only rebase. test: organize library tests and label CTest suites #6044 movesGuestFilesystem.cpptotests/files/; if it lands first, the new cases move with the file. fix(libkernel): support lstat on long Windows paths #7202, fix(libkernel): remove a Windows directory junction without emptying its target #7530 and fix(libkernel): treat Windows junctions in unlinkat like unlink and rmdir #9095 do not changeRename.cpp.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); no silent stubs added.mdfiles or images added to the repositoryAI-assisted (Claude Code).
🤖 Generated with Claude Code