Repository navigation
fix(libkernel_sys): resolve linkat paths relative to a directory descriptor - #10434
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.
|
|
Built and verified on Linux (Ubuntu 24.04 on WSL2, GCC 13.3.0, x86-64 AMD Ryzen 7 9800X3D) to cover the untested platform mentioned in the description:
Looks solid on Linux. LGTM! |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 11d6efa and FreeBSD 11.4 sys_link/sys_linkat/kern_linkat (vfs_syscalls.c) and namei (vfs_lookup.c):
- The resolution matches main's other *at calls (
mkdirat,unlinkat,renameat,readlinkat), and the EBADF/ENOTDIR for the descriptor line up with namei. The flag handling and the throw for a symlink source withoutAT_SYMLINK_FOLLOWmatchsys_linkat, and it still merges cleanly into main. - One ordering detail:
kern_linkatrunsnameion path1 completely, including theEPERMfor a directory source, before it looks at path2. HereResolveAtPath(fd2, …)(Export.cpp:491) runs beforeLinkNativechecks the source. Solinkat(dirfd, "missing.txt", 4321, "x", 0)returnsEBADFwhere FreeBSD returnsENOENT, and a directory source with a badfd2givesEBADFinstead ofEPERM. Checking the source (existence and directory) before resolving path2 would fix it, and a test likelinkat(directory, "missing.txt", 4321, "absent.txt", 0) == -1 && Error() == 2would cover it.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
@harisibrar1 Thanks, you're right. Fixed in 2d32e8b. The source checks move out of New checks in
These fail with the first commit's @amazedturnip Thanks for the Linux run. I added it to the body for d44d568. The ordering commit is new, so it hasn't been run on Linux yet. |
harisibrar1
left a comment
There was a problem hiding this comment.
Re-checked head 2d32e8b against main 6cd36e6:
linkatnow finishespath1before it touchespath2: resolution, the symlink throw, thenLinkSourceError(Export.cpp:495-497), and only then thepath2EFAULT/ENOENT checks andResolveAtPath(fd2, …)(Export.cpp:498-500), which matches the order inkern_linkat.linkat(directory, "missing.txt", 4321, "absent.txt", 0)now gives ENOENT, and a directory source with a badfd2gives EPERM.- Moving the checks into
LinkSourceErrorkeepslink's behaviour the same;LinkNativenow only handles the destination and the native link. - The new cases in GuestKernelSys.cpp:174-179 cover each ordering case I raised.
- It still merges cleanly into main, and main's
linkatstill throws for relative paths with a descriptor, so this is still needed. Thanks, looks good to me.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
What
linkatthrewlinkat: a relative path with a directory descriptorwheneverfd1orfd2was notAT_FDCWDand its path was relative. FreeBSD 11'skern_linkatlookspath1up fromfd1andpath2fromfd2, like every other*atcall.Each path is now resolved on its own, as
renameat,readlinkatandunlinkatalready do.AT_FDCWDor an absolute path goes throughResolvePathError_nid_no_patch. Any other relative path is joined to the descriptor's host directory fromGetDirectoryDescriptorPath_nid_no_patch, which fails withEBADFfor a descriptor that is not open andENOTDIRfor one that is not a directory, asnameidoes. A path ofGuestPathMaxor longer, or one with a component that is too long, fails withENAMETOOLONG, as inrenameat. The body oflinkafter path resolution moves intoLinkNativewithout changes, solinkandlinkatshare the checks forEPERMon a directory,EEXIST,ENOENT,ENOTDIRandEXDEV.kern_linkatfinishesnameionpath1, including theEPERMfor a directory, before it looks atpath2.linkatfollows the same order. The source checks move out ofLinkNativeintoLinkSourceError.linkatruns, in order: theEFAULTandENOENTchecks forpath1, its resolution, andLinkSourceError. Only then does it check and resolvepath2. So a missing source givesENOENTand a directory source givesEPERM, even whenfd2is not open orpath2is null or empty.linkresolves both paths and then callsLinkSourceErrorandLinkNative, so its order is unchanged.An empty path now fails with
ENOENTinlinkatitself, as it did before throughlink. A symbolic link as the source withoutAT_SYMLINK_FOLLOWstill throws, now also when it is named relative to a descriptor. The throw comes before the source checks, which would follow the link.The debt entry takes the sentence
readlinkatandunlinkatuse for descriptor-relative paths, with the same limits: guest path aliases and case-insensitive name matching do not apply, and a..component is not kept inside the guest root.Test: the two
ThrowsNamingchecks inguest_kernel_sysbecome real links:hard_link_count;EEXISTandENOENTwith both paths relative to the descriptor;EBADFfor an unopenedfd1orfd2, andENOTDIRfor a regular file's descriptor asfd1orfd2;ENOENTwith an unopenedfd2or a nullpath2, and a directory source givesEPERMwith an unopenedfd2or an emptypath2. A valid source with a null or emptypath2givesEFAULTorENOENT;Tested
Windows 11, WinLibs MinGW-w64 GCC 15.2.0 r7, CMake + Ninja, Release,
-DBUILD_TESTING=ON, onmaina23f8fc.main'sExport.cpp,guest_kernel_sysfails (exit 0xc0000409: the call throws where the test expects a link). With this PR it passes.Export.cpp(path2 resolved before the source check), the new ordering checks fail (exit 0xc0000409). With head 2d32e8b they pass.guest_kernel_sys,guest_kernel_rename,guest_rename_dotandconventions_doc_linkspass on 2d32e8b.python tools/check_conventions.py --base origin/mainreports no errors.guest_kernel_syson the first commit d44d568 (Ubuntu 24.04 on WSL2, GCC 13.3.0), and it passed. The ordering commit has not been run on Linux. No title or homebrew run.Overlap:
tools/pr_overlap.pyfinds no open PR that conflicts on the changed lines. Of the PRs it could not compare:link. This PR moveslinkbelowLinkNative. Whichever merges second needs a small rebase, andlinkatwould then take the sameCopyPathchecks.Export.cpporGuestKernelSys.cpp, and none of them changeslinkorlinkat.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)