Skip to content

fix(libkernel_sys): fchmodat follows a symbolic link like chmod - #11288

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
Gruzzberry:fix/fchmodat-follow-links
Oct 26, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
Gruzzberry:fix/fchmodat-follow-links

Conversation

@Gruzzberry

@Gruzzberry Gruzzberry commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

What

fchmodat without AT_SYMLINK_NOFOLLOW called the CRT _wchmod directly, which on Windows acts on the link itself. Through a symbolic link FreeBSD 11.4's kern_fchmodat looks the path up with FOLLOW, so the mode must change on the target. On main a file link returned success and left the target untouched, a dangling link returned success instead of ENOENT, and a link loop returned success instead of ELOOP. chmod already handles all of this through NativeChmod in Stdio.cpp (#9487); fchmodat now calls the same function through a new ChmodNative_nid_no_patch, for the AT_FDCWD, absolute-path and directory-descriptor cases alike. The AT_SYMLINK_NOFOLLOW branch and the existing debt entries are untouched.

I also probed fstatat, lchmod and linkat with AT_SYMLINK_NOFOLLOW / AT_SYMLINK_FOLLOW on file, directory, dangling and looping links against the same source. Those link cases match: fstatat with NOFOLLOW reports the link itself (S_IFLNK), linkat with AT_SYMLINK_FOLLOW links the target and refuses a directory link with EPERM, a dangling link with ENOENT and a loop with ELOOP, and lchmod and the NOFOLLOW link cases are already in the debt file. Not fixed here, found while probing, all older than this PR:

  • A path below a dangling link or a link loop (dangling/child, loop/child) gives ENOTDIR from fchmodat, linkat and chmod, where FreeBSD gives ENOENT or ELOOP: std::filesystem::status reports such a link as an existing regular file, in both HasNonDirectoryPrefix copies (Export.cpp and FilesystemError.hpp).
  • fstatat with a directory descriptor and AT_SYMLINK_NOFOLLOW gives ENOENT for link-to-file/child (Open.cpp, StatInDirectory_nid_no_patch), where lstat and fstatat(AT_FDCWD, ...) give ENOTDIR as FreeBSD does.
  • A trailing slash on a file, a link or a dangling link in fchmodat, chmod and lchmod gives EINVAL (Windows error 267 from GetFileAttributesW), where FreeBSD gives ENOTDIR, ENOTDIR, ENOENT and ELOOP.

Tested

Windows 11 (10.0.26200), WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release, -DBUILD_TESTING=ON, Developer Mode on, on main acb900c (four commits behind now, none touching these files).

  • guest_kernel_sys: new checks, where the guest symlink can create links, that fchmodat with flag 0 through a file link changes the target (relative to a descriptor, and with AT_FDCWD and a relative path), and that a dangling link gives ENOENT, a loop ELOOP and link/child ENOTDIR. On main the test fails; with this change it passes, three runs out of three.
  • Full suite on this branch: the only failures are pr_overlap_encoding, agc_host_import_cut_lifetime and agc_driver_depth_plane_partial_store, which fail on main in this environment too.
  • python tools/check_conventions.py --base upstream/main: passes.
  • Not tested: Linux and macOS (the call there is the same chmod), a console, a title run.

Overlaps

Checklist

  • Based on current main; no other open PR implements the same function
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); no silent stubs added
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source: none added
  • Generic behaviour, not specific to one title
  • Depends on: feat(libkernel_sys): implement lchflags #9442 (neighbouring lines); merge order: feat(libkernel_sys): implement lchflags #9442 first, then this PR rebases
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

AI-assisted (Claude Code).

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation does not change the Windows operation that this PR is meant to fix. The removed fchmodat branch called _wchmod(native.wstring().c_str(), mode & 07777). The new ChmodNative_nid_no_patch wrapper calls NativeChmod with the same path and masked mode, and the Windows NativeChmod implementation in Stdio.cpp calls that same _wchmod API. Its only additional effect is registering the path with RecordWrittenPath; the old fchmodat path already registers it after success. That bookkeeping does not make the filesystem operation follow the link.

So the link, dangling-link, and loop outcomes on Windows should remain unchanged by this patch. The reported Windows test result conflicts with the actual call paths. Please verify the test is running the changed fchmodat implementation in the built binary and add a Windows-specific operation that follows the target, with the file-link, dangling-link, and loop cases passing. The POSIX branch already called chmod and is not changed semantically by this wrapper.

@Gruzzberry
Gruzzberry marked this pull request as ready for review October 26, 2026 00:24
@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 5021 core/libs/prx/libkernel/File/src/Stdio.cpp
core/libs/prx/libkernel_sys/Export.cpp
PR 6936 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 7121 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 7201 core/libs/prx/libkernel/File/src/Stdio.cpp
core/libs/prx/libkernel_sys/Export.cpp
PR 7447 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9097 core/libs/prx/libkernel_sys/Export.cpp
core/libs/tests/GuestKernelSys.cpp
PR 9284 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9293 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9297 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9302 core/libs/prx/libkernel/File/src/Stdio.cpp
PR 9389 core/libs/prx/libkernel_sys/Export.cpp
PR 9713 core/libs/prx/libkernel_sys/Export.cpp
core/libs/tests/GuestKernelSys.cpp
PR 11341 core/libs/prx/libkernel/File/src/Stdio.cpp

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to my earlier CHANGES_REQUESTED review: I had inspected the POSIX NativeChmod implementation and missed the distinct _WIN32 branch. On Windows, NativeChmod detects a reparse point, opens it without FILE_FLAG_OPEN_REPARSE_POINT so the handle follows the link, resolves the target with HandlePath, and calls _wchmod on that target. The new ChmodNative_nid_no_patch wrapper routes fchmodat(..., flag=0) through this behavior; the old branch called _wchmod on the link path directly. The added Windows test covers a file symlink, dangling link, loop, and a child path through a link. The author reports three passing runs on Windows; I did not independently run that Windows test here. The specific blocker in my previous review was incorrect, and I found no other blocking issue in the current diff.

@wachi35676

Copy link
Copy Markdown
Contributor

Windows and Linux runs of head 7322a3b9f8e6 merged onto main 0f34ebb85. Windows used WinLibs GCC 15.2.0 (the guest symlink creates links on this machine, so the new checks run), Linux used WSL with GCC 13; both Release.

  • Windows: guest_kernel_sys passes 3/3 with no warnings. With main's DirectoryDescriptor.hpp, Stdio.cpp and Export.cpp it fails at line 1123 (errno 2).
  • Linux: guest_kernel_sys passes 2/2. It also passes with main's sources, as expected, since the old POSIX branch was already a following ::chmod. The only build warnings come from main's SystemConfiguration.cpp and Signals.cpp.

AI-assisted: yes (Claude Code); results above were run on this machine.

@MarcZX10

Copy link
Copy Markdown
Contributor

The implementation review and Windows report remain useful: the reported guest_kernel_sys checks pass on the PR head, and the current merge preview is clean. One sequencing item is still unresolved: this PR says #9442 must land first because both edit neighboring code in Stdio.cpp, but #9442 remains open and is itself awaiting the author’s requested focused checks after its latest update. Please keep the dependency explicit and rebase this branch after #9442 lands, preserving both changes, then rerun guest_kernel_sys on the resulting head. Current upstream main is 204c6747d0a1cbaf932def3dea0778810cc3935e; this branch is 273 commits behind it. The current preview merges cleanly, but it does not include intervening main changes, so please also run the repository’s required base-sync checks before merge.

@oneandonlydean
oneandonlydean merged commit 26bbcd1 into boykopovar:main Oct 26, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants