Repository navigation
fix(libkernel): follow a Windows symbolic link in chmod - #9487
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the Windows chmod path and its focused link cases. The non-link path retains its existing _wchmod behavior; for a reparse point the code opens the target (without OPEN_REPARSE_POINT), resolves the handle to the target path, records that target as written, and then changes its mode. The resulting error path uses the repository Windows error mapper, which maps ERROR_CANT_RESOLVE_FILENAME to ELOOP and missing paths to ENOENT. The tests cover changing permissions through a link, ensuring the link itself stays unchanged on Windows, a dangling target, and a loop; they clean up the links afterward. I checked the neighboring open filesystem PRs #9469 and #9474: they touch separate Stdio.cpp regions and the same test file at separate cases, with no overlapping behavior that needs a Depends on relationship. Conventions, intake, eligibility, labels and progress-report checks pass. The automatic product build jobs were skipped; the PR reports Windows focused testing locally.
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed the Windows path and regression cases. The non-reparse path retains _wchmod; a reparse point is opened following its target, resolved to the target path, recorded as the written path, and then chmodded. The error mapping covers a missing target and a loop. Tests cover changing the target through the symlink, checking the link's own attributes, dangling and loop cases, and cleanup. I manually checked the reported shared Stdio.cpp warnings (#5021, #6879, #6936 and #7201); none changes the chmod or handle-path helpers. Hosted eligibility, intake, conventions, labels and progress-report checks pass; product/relinker jobs are skipped. The author reports focused Windows testing; I did not run local tests.
MarcZX10
left a comment
There was a problem hiding this comment.
The chmod path appears correct for the cases reported: non-reparse paths keep _wchmod, a reparse point is followed through a handle-derived target path, and tests cover target mode changes, dangling targets, loops, and cleanup. The prior manual review found the #5021/#6879/#6936/#7201 Stdio warnings outside the chmod/helper path, and #6044’s GuestFilesystem changes were in separate cases. The current branch and Windows tests are based on a6dcea9, while upstream main is ab347d1. Please rebase onto current main, rerun the focused guest_filesystem checks and conventions, then refresh overlap/manual comparisons. I could not retrieve the path-filtered PR patches for the warned PRs in this pass, so I am relying on the earlier review’s manual diff; do not treat this message as a fresh independent diff verification. Hosted product jobs are skipped; I did not run local tests.
74d4c1a to
3fe0d9b
Compare
|
Rebased onto |
|
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 |
_wchmod changes the read-only attribute of the path it is given, so on Windows chmod through a symbolic link changed the link and left the file it names unchanged, and a dangling link succeeded. A reparse point is now opened through its target and the target's path is changed, as FreeBSD 11.4's sys_chmod passes FOLLOW: a missing target fails with ENOENT and a link loop with ELOOP. The path lookup of a handle moves out of WindowsDescriptorPath_nid_no_patch so both use it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3fe0d9b to
4e5db13
Compare
|
Rebased onto |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the reparse-point path against FreeBSD's |
What
On Windows,
chmodthrough a symbolic link changed the link and left the file it names unchanged, and a dangling link succeeded._wchmodsets the read-only attribute of the path it is given without following a link. FreeBSD 11.4'ssys_chmodcallskern_fchmodatwith flag 0, which looks the path up withFOLLOW.NativeChmodnow opens a reparse point withoutFILE_FLAG_OPEN_REPARSE_POINT(so Windows follows it), takes the target's path from the handle and changes that path. A failed open goes throughWindowsOpenError_nid_no_patch:ENOENTfor a missing target,ELOOPfor a loop. The handle-to-path code moves out ofWindowsDescriptorPath_nid_no_patchintoHandlePathso both use it;fchmodgoes through the same descriptor path as before. Paths that are not reparse points take the same_wchmodcall as onmain.fchmodatwithAT_SYMLINK_NOFOLLOWreturns before reachingchmodand is unchanged. Linux is unchanged.Tested
Windows 11 (10.0.26200), WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release,
-DBUILD_TESTING=ON, onmaind6eafa5 (rebased), Developer Mode on so links can be created.guest_filesystem: new checks next to thefchmodones. Through a link to the test file,chmod 0400clears the file's owner write bit while the link keeps no read-only attribute, andchmod 0600restores it; a dangling link fails withENOENT; a link to itself fails withELOOP(Windows only). The links are removed afterwards; without link permission the checks are skipped with a message. Onmainthe test fails at the first new check (the file stays writable); with this PR it passes.maind6eafa5 and onmainwith 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 merged together, 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.std::filesystem::create_symlink, and the loop check is Windows only), a console, a title run.Overlaps
tools/pr_overlap.py --branchon the rebased head lists no direct overlap. Of the 12 pull requests it could not compare, feat(libc): support archive-backed stdio reads #7447 only declaresfchmodin a test and feat(libkernel): enumerate archived app directories #8520 callsWindowsDescriptorPath_nid_no_patch, whose result this PR keeps; neither changesNativeChmodor the handle-path code.GuestFilesystem.cpptotests/files/; if it lands first, the new cases move with the file in the rebase.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