Repository navigation
fix(libkernel): run sceKernelRename through the checks rename uses - #5377
Closed
DavidSamir wants to merge 6 commits into
Closed
DavidSamir wants to merge 6 commits into
DavidSamir wants to merge 6 commits into
Conversation
…differ in on Windows
10 tasks done
10 tasks done
Collaborator
|
Closing: no title is known to reach this. With the queue at 480+ open PRs, please hold off on new PRs that don't name a title needing the change until it's down. If a title stops on this on current main, reopen with the title ID and the log line. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
On Windows,
sceKernelRenamecannot rename a directory onto an existing empty directory: it returns0x80020005(EIO) and nothing moves. FreeBSD's rename(2) replaces an empty directory, the Linux build does, andrenamein this tree already does on both hosts. A program that prepares a directory under a temporary name and renames it over the old, emptied one fails only through thesceKernelexport.sceKernelRenamealso reports0x80020005for every failure on both hosts, whererenameand FreeBSD give the specific error (ENOTDIR,EISDIR,ENOTEMPTY,EINVAL,ENOENT).Cause.
renamerunsRenamePath_nid_no_patch, which checks the two paths, removes an empty destination directory on Windows before the host rename, and maps each case to its error.sceKernelRenamehas its own shorter body that calls the host rename directly and turns any error into EIO, so on Windows the host refuses a directory onto an existing directory.Observed on
main0214069, Windows 11 (10.0.26300), WinLibs GCC 15.2.0 r7, Release:sceKernelRename(from, to)mainon Windowsmainon Linux0x800200050x800200050x800200050x80020042(ENOTEMPTY)0x800200050x800200050x80020014(ENOTDIR)0x800200050x800200050x80020015(EISDIR)0x800200050x800200050x80020016(EINVAL)0x800200050x800200050x80020002/0x80020014other/,otherbeing a fileotherreplaced0x800200050x80020014, both keptnew/,newmissingnewcreated0x800200050x80020002I have not found a title that stops on this. It is the directory case #4532 left as "not changed", and the error codes came out of a probe run on both hosts.
Six commits:
fix(libc): rename a directory onto a name written with a trailing slash:Rename.cpp.rename("dir", "new/")fails withENOENTon both hosts today; FreeBSD allows a trailing slash on the destination when the source is a directory (kern_renameatsetsWILLBEDIR).sceKernelRenameaccepts it today, so this comes first: without it the next commit would take that away.fix(libkernel): run sceKernelRename through the checks rename uses:Stdio.cpp.sceKernelRenamebecomes the null check it had plus one call toRenameGuestPath_nid_no_patch, the functionrenameandrenameatalready use, with its error returned as thesceKernelcode.test(libkernel): rename a directory onto an empty directory with sceKernelRename.test(libkernel): check the errors sceKernelRename reports.test(libc): rename a directory onto a name written with a trailing slash.docs(debt): describe what rename and sceKernelRename share and still differ in on Windows.After this,
sceKernelRenameandrenamecannot drift apart again: one path, one set of errors. The file-onto-file replacement from #4532 is unchanged;sceKernelRenamereaches it through the sameRenameNative_nid_no_patchcall as before.Other results that change, all in
sceKernelRename: a file another program holds without delete sharing gives0x80020010(EBUSY) instead of EIO on Windows, asrenamedoes; and the last two rows of the table, wheremainon Windows replaced or created a file through a name with a trailing slash.Not changed, and recorded in the debt file: on Windows the empty destination directory is removed and then the source is renamed, so the replacement is not atomic (this is what
renamedoes today); and a regular file named with a trailing slash is still accepted as the source on Windows, where FreeBSD fails withENOTDIR. Also unchanged and not in the debt file: a source path that runs through a regular file givesENOENTon Windows where FreeBSD givesENOTDIR, as in the other path functions.Overlap: #5008 adds a check at the top of both
sceKernelRenameandRenameGuestPath_nid_no_patch. Commit 2 rewrites the body ofsceKernelRename, so the two conflict in those lines ofStdio.cpp. They do not conflict in behaviour: with this PR the check #5008 puts inRenameGuestPath_nid_no_patchcoverssceKernelRenametoo. Whichever lands second needs a small rebase; I will do mine if #5008 goes first.guest_filesystemgains three groups of checks next to the existingsceKernelRenameones: a directory with a file in it renamed onto an empty directory (the file must arrive and the source be gone); the six error cases in the table, with both directories unchanged afterwards; and a directory renamed ontoname/throughsceKernelRenameand throughrename, plus a file ontoname/, which must fail withENOENTand leave the file.Tested
Windows (28b9f2d on
main0214069, Windows 11 10.0.26300, WinLibs GCC 15.2.0 r7, Release, full builds). The runs were made on 96ad4438b, which has the same code and tests; the two differ only in where the debt note of commit 6 is placed (it is indocs/dev/debt/libc.md, as the conventions check requires for a note that links a libc file):Stdio.cppandRename.cppas onmainand the tests kept,guest_filesystemfails (exit 0xc0000409,Filesystem check failed at line 517 (guest errno 5, Win32 error 87)). With the change it passes 5/5 (--repeat until-fail:5), andguest_kernel_rename,guest_posix_filesystem,guest_path_case,guest_kernel_fcntl,guest_truncate,guest_dup,file_streams,guest_standard_streamsandguest_fdopendirpass 3/3.main's libraries, the branch's, and on the Linux build before and after:renameandsceKernelRenameover every pair of six kinds of source and destination (missing, file, 0444 file, file held open, empty directory, directory with a file), then fifteen named cases (same name, into itself, onto its parent, missing parent, parent is a file, source under a file, trailing slashes on either side,.and..sources, a change of case), each printing the result and what is left at both names. On Windows 23 lines change. Five change what happens: the two directory-onto-empty-directory renames now succeed,renameof a directory ontonew/now succeeds, and the two trailing-slash rows of the table now fail. The other 18 aresceKernelRenameerror codes. On Linux 23 lines change: one isrenameof a directory ontonew/, which now succeeds, and 22 aresceKernelRenameerror codes; nothing that succeeded on Linux fails. Afterwards the two hosts differ in four lines: a file named with a trailing slash as the source, and a source under a regular file, each forrenameand forsceKernelRename. Both are listed under "Not changed".renameline changes. 56sceKernelRenamelines change: six directory-onto-directory renames that now succeed, and 50 error codes (0x8002001418,0x8002001518,0x8002001011,0x800200423). Its stress part, with both sets: 20,000 renames to a free name, 10,000 replacements of a closed file and 10,000 of an open one, half through each function, with no failure and the handle count unchanged at 140; eight threads replacing an open file 2000 times each with no failure and the expected ten directory entries left.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 overunlink, 156 over opens of names ending in a slash, 313 overfdopenand 198 over paths that run through a regular file, run with both sets of libraries: the lines that differ are fivesceKernelRenamelines in the third probe and eight in the last (the cases above), and two counters in thefdopenprobe that change from run to run with either set.sceKernelRenameto a free name 426 to 657 µs withmain's libraries and 473 to 665 µs with the branch's;renameto a free name 475 to 685 and 480 to 696; creating a file andsceKernelRenameonto an existing one 451 to 774 and 569 to 882.sceKernelRenamenow makes the status queriesrenamemakes before the host call, which is where the last pair comes from.ctest --parallel 4: 1095 of 1095 pass, as withmain0214069 built without this change.guest_filesystem,guest_kernel_rename,guest_posix_filesystem,guest_path_caseandfile_streamspass 3/3 at every one.tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.py --branchreports one overlap, fix(libc): fail a rename of a path that ends in a dot component with EINVAL #5008 inStdio.cpp, described above.Linux (96ad4438b, WSL2 Ubuntu 24.04, GCC 13.3.0, Release):
libsbuilds, andguest_filesystem,guest_kernel_renameandguest_posix_filesystempass 3/3. The probe results for Linux are in the second item above.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)