Repository navigation
Conversation
…ponents Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 69a7bd2c19dd68c1b269705ef33bfbf35609452c. The change permits relative .. targets only when the host’s physical traversal agrees with the guest path resolution: every directory being left exists and is not a symlink, and the resulting host path matches the resolved guest target. It keeps absolute targets and paths that escape mount aliases or traverse symlink directories unsupported. The added tests cover preserved readlink text, file reads through valid parent targets, dangling targets, a symlinked path component, mount escape, and an in-mount parent traversal. I checked the reported #9442 overlap: it only adds a separate declaration in the shared test file.
git diff --check upstream/main...HEAD and python3 tools/check_conventions.py --base upstream/main --head HEAD pass. Hosted eligibility, conventions, intake, labels, and report pass; hosted product jobs were skipped. The Windows 11 filesystem/CTest results in the PR description are author-reported; I did not independently run Windows. I found no blocking issue in this implementation.
|
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.
Follow-up on the new overlap warning at exact head 69a7bd2c19dd68c1b269705ef33bfbf35609452c: I manually compared #9442 and #10625. git merge-tree --write-tree upstream/pr/9442 upstream/pr/10625 reports a content conflict in core/libs/tests/GuestKernelSys.cpp at the adjacent prototype block: #9442 adds the lchflags_nid_postfix declaration, while this PR adds AddPathAlias_nid_no_patch and RemovePathAlias_nid_no_patch there. The declarations are independent and can be retained together, but the authors still need an agreed merge order and the dependent PR should record Depends on: #N per CONTRIBUTING. The current checklist says Depends on: none.
Please coordinate with #9442's author and update the dependency/order note. My earlier approval covered the symlink implementation; this follow-up requests changes only for the unresolved shared-file merge coordination.
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 69a7bd2c19dd68c1b269705ef33bfbf35609452c. The overlap issue is now handled: the author proposed #9442 first, updated the checklist to Depends on: #9442, and will retain both declaration blocks on rebase. The symlink path checks and test coverage remain as previously reviewed. Current git diff --check, repository conventions and merge-tree checks pass; hosted eligibility, intake, conventions, labels and report pass, while product jobs are skipped. The Windows test results are author-reported and were not independently reproduced here.
|
@Dom3005 Please drop the two newly added SymlinkTargetThrows cases for symlink-prefix traversal and mount escape (GuestKernelSys.cpp lines 340 and 344), together with any fixture/cleanup used only by those throw assertions. They pin currently unimplemented paths. Keep the successful parent targets, dangling target, unchanged readlink contents, actual reads and the in-mount success case. The new host/guest target checks otherwise look consistent; #9442 remains the declared integration prerequisite. |
What
symlinkthrewparent-directory targetsfor any target with a..component, such as../file.txtor... FreeBSD 11.4 stores the target unchanged and itsnameiresolves the..later, from the directory that holds the link.Here the link is a host symbolic link, so the host follows it, not the guest path resolver. A
..target is therefore created only when both reach the same place:..leaves must exist and not be a symbolic link, so the host's physical..gives the same directory as the resolver's lexical one;Anything else still throws, with a message that names the reason: a
..that climbs out of a mount such as/savedata0, or a..past a directory reached through a symbolic link. Absolute targets still throw as well; they would need a host path stored in the link and a translation back inreadlink. The checks run after the existingEEXIST,ENOENTandENOTDIRchecks on the new name, so those errors come first, as in FreeBSD'skern_symlinkat.A new
symlinkentry indocs/dev/debt/libkernel_sys.mddescribes the rule and what still throws.Tested
Windows 11 26200, WinLibs MinGW-w64 GCC 15.2.0 r7, CMake + Ninja, Release,
-DBUILD_TESTING=ON, Developer Mode on (so the link success cases run), onmain6cd36e6517.guest_kernel_sysgets cases that:../file.txt,deeper/../../file.txtand.., checkreadlinkreturns each target unchanged, and read the file through the link;../missing/x;../file.txtin an aliased mount, where the guest path/file.txtand the host's parent directory differ;deeper/../upinside that mount and read through it.a/../bwith a missinga,a/../../xand/absstill throw.guest_kernel_syspasses 3 of 3, and the 82 tests matchingkernel_sys|filesystem|symlink|readlink|lstat|stat|rename|link|chmod|access|open|pathpass. The full build has 0 failed steps.main'sExport.cppwith the new test:guest_kernel_sysaborts withsymlink: parent-directory targets not implemented.python tools/check_conventions.py --base upstream/mainpasses.tools/pr_overlap.pyreports only feat(libkernel_sys): implement lchflags #9442, which adds a declaration on the same line ofGuestKernelSys.cpp. None of the other open PRs that touchlibkernel_sys/Export.cppchangesymlink.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