Repository navigation
fix(libc): report ENOTDIR for a path under a regular file in access, remove, opendir and chdir on Windows - #5530
DavidSamir wants to merge 11 commits into
Conversation
…r file on Windows
…at report ENOTDIR under a regular file
CnRJay
left a comment
There was a problem hiding this comment.
The behaviour is right. My concern is how the same check is spreading through the code. With this PR, the "is a prefix of this path a regular file" walk exists in at least four places:
HasNonDirectoryPrefixinlibkernel/File/src/Stdio.cpp(already onmain, used by mkdir, rmdir and truncate);- the inline loop in
sceKernelOpenfrom #5504; UnderRegularFile_nid_no_patchhere, in libc;- the chmod/utimes/lutimes copy in #5549.
They're all meant to be the same rule, and the debt entry still lists stat, lstat, unlink, rename and fopen as still to do, so that's another five call sites. One helper (this PR's UnderRegularFile_nid_no_patch is the natural candidate, since libc is below libkernel) used by all of them, with HasNonDirectoryPrefix and the #5504 loop rewritten on top of it, would keep the rule in one place. Fixing an edge case (a prefix that is a symbolic link to a file, a drive root, or UNC paths) would then fix every caller at once. Alternatively, map ERROR_PATH_NOT_FOUND in the shared Windows error translation (FilesystemError) whenever the path is available, so new call sites get it for free.
Could #5504, #5530 and #5549 agree on that order? Whichever lands first can introduce the shared helper, and the others rebase onto it.
|
Closing in favour of #5549, which contains these commits (and #5504's), to keep the queue shorter. @CnRJay, thanks for the review. #5549 has no walk of its own: its AI-assisted: yes |
What
On Windows,
access,remove,opendirandchdirreportENOENTfor a path that runs through a regular file, such assave.dat/x. FreeBSD reportsENOTDIRfor each of them ("A component of the path prefix is not a directory" in access(2), unlink(2), rmdir(2), chdir(2) and open(2), through whichopendiropens its directory), and the Linux build does.This continues #5504, which does the same for
open, and takes four functions off the list its debt note records.Cause. Each of the four asks the host and maps its error. For a path whose directory part contains a file, Windows answers
ERROR_PATH_NOT_FOUND, the same as for a missing directory, and both becomeENOENT.Observed on
main18c438b with #5504, Windows 11 (10.0.26300), WinLibs GCC 15.2.0 r7, Release.fileis a regular file:access("file/x", F_OK)remove("file/x")opendir("file/x")chdir("file/x")file/x/ymissing/xI have not found a title that stops on this. It came out of running one probe of the path functions on the Windows build and on the Linux build and comparing the two outputs.
Seven commits:
fix(libc): report ENOTDIR from access for a path under a regular file on Windows: addsUnderRegularFile_nid_no_patchtoGeneral.cppand uses it inaccess.fix(libc): report ENOTDIR from remove for a path under a regular file on Windows.fix(libc): report ENOTDIR from opendir for a path under a regular file on Windows.fix(libc): report ENOTDIR from chdir for a path under a regular file on Windows: inSetWorkingDirectory, sofchdirand the other callers share it.test(libc): access and remove a path under a regular file.test(libc): opendir and chdir a path under a regular file.docs(debt): list access, remove, opendir and chdir among the calls that report ENOTDIR under a regular file.Windows only.
UnderRegularFile_nid_no_patchwalks a host path's prefixes from the nearest one up and answers for the first that exists: true when it is not a directory. Each of the four functions calls it only after its host call has failed withENOENT, and then reportsENOTDIR. The host call is made once and its failure stands; this only names it. A call that succeeds, and one that fails for another reason, make the same host calls as before.Not changed:
stat,lstat,unlink,rename(for its source),chmod,utimesandfopenstill reportENOENTfor such a path on Windows; the debt note says so. The resolver's check for a..component andsceKernelRmdireach keep their own copy of this walk;General.cpparound the resolver is being changed by #4938 and #4681, so I left those lines alone.guest_filesystemgains two groups of checks beside the existingremoveones. For a name one and two levels under a regular file,accesswithF_OKand withR_OKandremovemust fail with errno 20, and for a name under a missing directory with errno 2, with the file still there afterwards. Then the same foropendirandchdir.Tested
Windows (73658bc on
main18c438b plus #5504, Windows 11 10.0.26300, WinLibs GCC 15.2.0 r7, Release):General.cpp,General.hpp,Filesystem.cppandDirectory.cppas in fix(libkernel): fail open under a regular file with ENOTDIR on Windows #5504 and the tests kept,guest_filesystemfails (exit 0xc0000409,Filesystem check failed at line 355 (guest errno 2, Win32 error 3)). With the change it passes 5/5 (--repeat until-fail:5), andguest_kernel_fcntl,guest_truncate,guest_dup,guest_file_vector,file_streams,guest_standard_streams,guest_posix_filesystem,guest_fdopendir,guest_kernel_renameandguest_path_casepass 3/3.openfamily, thestatfamily,access,realpath,opendir,chdir,truncate,utimes,chmod,mkdir,rmdir,unlink,removeandrename) on each offile/x,file/x/y,file/,file/.,file/..andfile/../file. Ten lines change: the eight of the table, andopendir("file/")andopendir("file/."), which also go fromENOENTtoENOTDIR. With the branch, every line foraccess,remove,opendirandchdirequals the Linux build's.flock, 44 over pipe streams, 15 overfcntlstatus flags, 127 overfreopen, 130 over directory descriptors, 117 over streams used the other way, 144 over truncating opens, 164 overunlinkandremove, 156 over opens of names ending in a slash, 102 overrename, 135 overstat,chmodandutimes, 55 over the working directory and directory streams and 313 overfdopen, run with both sets of libraries: one line differs,remove("file/x")in theunlinkprobe (the change above), apart from a file size in thefdopenprobe that changes from run to run with either set.ctest --parallel 4on a full build of the last commit: 1097 of 1097 pass, as withmain18c438b. That is the second run. In the first,guest_none_relocationstimed out at its 20 s limit while I was doing other work on the machine; it takes 1.2 s on its own and passed 5/5 alone and in the second full run.guest_filesystem,guest_posix_filesystem,guest_fdopendir,guest_path_caseandfile_streamspass 3/3 at each. At no commit does a rebuilt library lose an exported name. The full build of everything was done at the last commit.tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.py --branchreports no overlap with open pull requests.Linux (same commit, WSL2 Ubuntu 24.04, GCC 13.3.0, Release):
libsbuilds, andguest_filesystemandguest_kernel_fcntlpass 3/3, so the new checks hold on the Linux build as it is. The changed code is Windows only.Not checked on a console or with a commercial title.
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)