Repository navigation
fix(libkernel): chmod read-only files before unlink on Windows - #2651
sridhar777911 wants to merge 70 commits into
Conversation
|
Additional platform validation on standalone PR head Linux: blocked or failed — Arch Linux in WSL2 (kernel 6.6.87.2), GCC 16.2.1 20260810; CMake 4.3.1 / Ninja. Release / Reproduce: Tests: Blocker: Command exited 1: cmake. GCC reports a conflicting declaration of readOnly in core/libs/tests/GuestFilesystem.cpp:288; the same variable is already declared at line 107 in main(). The PR adds the second declaration in the same scope, so guest_filesystem_tests does not compile. Rename or reuse the new variable before this test can run. |
Follow-up on the current PR headThis updates my earlier review because the PR head changed to
The PR overlaps issue #2601, where Achraf has Windows 11 evidence for both successful unlink and restoring read-only state when an open handle prevents deletion. Please coordinate on one canonical implementation and preserve that test evidence. The latest automatic check shown here is Conventions only; it does not validate this broad diff or compile the failing Linux target. The focused test and the collaborator-triggered Linux/Windows Build plus relinker matrix required by |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WindowsPeWriter stored every PE section with SizeOfRawData equal to its virtual size, so the zero-filled tail of a PT_LOAD segment (p_memsz above p_filesz, the .bss) was written to the executable byte for byte. A segment with 256 MiB of .bss produced a 269 MB .exe, while the Linux output of the same image is under 5 KB. Each section is now stored up to its last non-zero byte, aligned to the file alignment, and VirtualSize keeps the full size. The PE loader zero-fills the part of a section beyond SizeOfRawData (Microsoft PE format, section table: SizeOfRawData). A section with no non-zero byte gets no raw data and a zero PointerToRawData. windows_bss relinks an image with 64 MiB of .bss, checks the section table and the mapped image, and on Windows runs the executable, which reads the last word of the .bss.
…out a Vulkan device The --deferred-undecodable mode registers shaders through a Vulkan device but, unlike --deferred, did not check for one first. On the Windows CI runner, which has no Vulkan driver, vkCreateInstance returns VK_ERROR_INCOMPATIBLE_DRIVER and the test fails instead of skipping, so the windows job on main has failed since e5b32bb. It now returns VulkanTestSkipped like the other modes.
On Windows std::_Exit ends in ExitProcess, which still runs every module's DLL_PROCESS_DETACH and TLS callbacks after the other threads are gone. A static destructor that joins a worker then waits forever: a title that calls _Exit while libSceVideoOut's flip queue runs (ps5-opengl's retirement fail-stop) stays alive in FlipQueue's destructor. _Exit and libkernel's _exit now terminate the process with TerminateProcess, as _exit ends a POSIX process: no atexit handlers, stream flushes or module destructors.
…ndows On Windows libc exports __cxa_init_primary_exception, std::uncaught_exception and std::uncaught_exceptions only as _nid_postfix symbols (GCC_GLOBAL_ALIAS is empty there), so host modules bind libstdc++-6.dll's versions: - std::make_exception_ptr, which std::promise's destructor uses for broken_promise, allocated the exception with libc and initialized it with libstdc++'s __cxa_init_primary_exception, whose header layout differs. A test that makes one and rethrows it dies with an access violation. - std::uncaught_exceptions read libstdc++'s thread state, which libc's throws never touch, so it returned 0 while an exception unwound. The AGC driver's deferred-label guard relies on it to tell a failed record from a finished one. The three are now exported natively, like __cxa_throw and the rest. __cxa_init_primary_exception also sets the reference count to 0, as the Itanium ABI does: the exception_ptr created next takes the only reference, so a made exception is destroyed once instead of leaking.
…d blocks in a bitmap Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An anonymous private mapping with MAP_ALIGNED_SUPER (FreeBSD MAP_ALIGNED(1)) is placed on a 2 MiB superpage boundary. Yamagi Quake II requests it for its hunk allocations of 2 MiB and more. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tects Importing guest memory collected the write watch over the whole import, which write-protected every page of it, so pages nothing had collected yet took a write fault on their first CPU write. The import now collects only the parts a collect already protects (GuestWriteWatchCollectArmed). The rest stays unprotected until something collects it, which reports it as written; a GPU write recorded into such memory (MarkWritten) first collects the never-collected parts of its 64 KiB blocks (GuestWriteWatchCollectFresh), so that report is older than the GPU's results and does not read as a CPU write over them. Those pages are stamped as written and their CPU pages are noted, but the block's CPU stamp is left alone, so a GPU label record or a pending storage image elsewhere in the block does not see a CPU write. StoredOver and UnchangedSinceCollected read the noted pages whenever a block has them, so a store to such a page is still seen by a check over that page. The extra generation step happens only when something was collected; a block whose write protection fails is unwatched and the write returns no generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On Windows, sceKernelMprotect accepted only the executable's image and threw "Memory protection of a foreign image is not supported" for every other image. Bundled title modules are relinked into DLLs named <module>.guest.prx, so a title that reprotects its own module pages failed. Dying Light 2's bundled libNRD does this at start-up (boykopovar#1794). DoMprotect now also accepts images whose file name ends in .guest.prx, and registers their committed pages like the executable's. Other images are still refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A .guest.prx image registered by sceKernelMprotect stayed in the guest allocation registry after dlclose freed it, so its dead ranges blocked later mappings at that address and could reach GPU builds. The Windows module destructor now unregisters the image before FreeLibrary. Unregistering waits for outstanding leases on the image like unmap and protect do; if the wait fails the module stays loaded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…te failures Move the image unregistration out of the Module destructor into dlclose_nid_postfix. It runs while the handle is still registered, so a failure (the image stays leased by GPU work) propagates to the caller and leaves the module loaded and usable. Images shared by another open handle stay registered until the last handle closes. guest_memory now checks that dlclose under a held lease throws and keeps the module, its symbols and its protection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sceFontGetCharGlyphMetrics and sceFontGetRenderCharGlyphMetrics set the vertical bearing and advance to zero although the glyph is loaded with FT_LOAD_VERTICAL_LAYOUT. The library's own text writing steps the pen by that vertical advance, so every character of vertical text was placed on the previous one. The vertical metrics now follow the FreeType driver's metrics of a rendered glyph: FreeType's vertical advance, and for a face without vertical metrics a bearing of half the horizontal advance left of the horizontal bearing and of half the advance left over by the glyph height. guest_system_font checks every metric of a square glyph at 100 pixels. (cherry picked from commit 04dd69f) (cherry picked from commit 11f6f9e954756ee8d4a3fc2be574ab3b57ecd7c5)
…mplers Guest samplers with DISABLE_CUBE_WRAP (S# word 0 bit 28) are created with VK_SAMPLER_CREATE_NON_SEAMLESS_CUBE_MAP_BIT_EXT when the device enables VK_EXT_non_seamless_cube_map; RADV maps the flag to the same bit. The bit only changes cube filtering, so without the extension the sampler is still created and only a cube texture sampled through it throws.
DecodeColorTileMode rejected 2D color targets with a nonzero MIP0_DEPTH, so an array target could only be rendered into through a view of a surface that declares one slice. radeonsi sets CB_COLOR_ATTRIB3.MIP0_DEPTH to the last layer of an array target (si_state.c, ac_descriptors.c), and Mesa-built homebrew emits such targets. The view's slice already moves the target by one slice size; it is now also checked against MIP0_DEPTH.
… pipe/bank XOR Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rgv on Windows On Windows, getargc and getargv reported only the executable path, read with GetModuleFileNameA in the ANSI code page, and dropped every other argument. Linux returns the whole command line from /proc/self/cmdline, and the relinker's Windows entry stub already passes the guest entry all arguments converted to UTF-8. Parse GetCommandLineW with CommandLineToArgvW and convert each argument to UTF-8 with WC_ERR_INVALID_CHARS, as the entry stub does, so getargv matches the entry's argv and the Linux behaviour. The Linux path is unchanged.
On Windows, sceKernelPread and sceKernelPwrite called the POSIX pread and pwrite wrappers, which throw on every failure, so a bad descriptor, a negative offset or a write to a read-only descriptor aborted the process instead of returning an error as the Linux build does. ftruncate and sceKernelFtruncate threw on any host failure on both platforms. Give the Windows sceKernelPread and sceKernelPwrite the same checks and SceErrorFromErrno returns as the Linux ones, and accept a zero-length transfer with a null buffer. Report a positioned transfer on a handle opened without the needed access as EBADF instead of EIO. Return EINVAL from ftruncate for a negative length and for a descriptor opened without write access, as FreeBSD does, and pass other host errors to the guest. Fixes boykopovar#2423
Palworld (PPSA23126) imports sceSaveDataCancel (e6y3WMBYbiM) from libSceSaveData and fails to load because no library exports it. Its signature is unknown, so it is exported without an implementation and throws if it is called, like sceSaveDataConvert.
The registry records whether a range was mapped or protected with GPU access (0x10/0x20), and an address-based build leaves out ranges without it, as the console's GPU page tables do. The main image is registered without GPU access; the title's own mprotect with GPU bits adds it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Astro's Playroom (PPSA01325) calls sceRudpActivate during start-up, where the throwing stub stopped it. The host has no reliable-UDP stack to activate, so the export returns success and does nothing, matching the OK-returning stubs shadPS4 keeps for the whole library. The guest_offline_imports test now checks the success path with and without sceRudpInit, and still checks that the sceRudpGetStatus sibling throws and writes nothing.
Co-Authored-By: Claude <noreply@anthropic.com>
scePthreadCancel and pthread_cancel threw, scePthreadTestcancel did nothing and the cleanup handler exports did not exist. A cancel request is now recorded on the thread and acted on at scePthreadTestcancel, the condition variable waits, scePthreadSem and sem waits, scePthreadJoin and the sleeps, and under their POSIX names. A thread blocked in one of the waits is woken; a condition wait takes its mutex back before the thread exits. The thread exits with PTHREAD_CANCELED through scePthreadExit, which now runs the handlers of __pthread_cleanup_push_imp and __pthread_cleanup_pop_imp. scePthreadSetcancelstate and scePthreadSetcanceltype reject unknown values, and pthread_setcanceltype is added. Constants, the handler node layout and the order of events follow FreeBSD libthr. A sleep is not interrupted, file and socket calls are not cancellation points and the asynchronous type acts like the deferred one except at the two set calls (TechnicalDebt). New test: guest_pthread_cancel.
…raries Co-Authored-By: Claude <noreply@anthropic.com>
The Windows sceKernelPread stopped going through pread when it learned to return errors, and with it lost the random device read, so a pread on /dev/urandom reached ReadFile on a handle that is not a disk and failed with ESPIPE. Read random devices first, as the Linux path does. A pread into a read-only guest mapping now returns EFAULT instead of throwing, matching Linux and the AIO read beside it, so the Windows guest memory test checks for that return.
MarcZX10
left a comment
There was a problem hiding this comment.
I cannot approve the current head f42420e7 yet. The merge-base diff is far broader than the titled unlink change, and the focused files still have correctness and build blockers:
| Blocker | Evidence | Required follow-up |
|---|---|---|
| Unrelated PR scope | The current PR has 70 commits and changes 234 files (+6,050/−448) from merge base 85d16bcc, while the title is only about Windows read-only unlink. |
Rebase onto current main and keep only this topic's source and tests. |
| Test target does not compile | GuestFilesystem.cpp declares readOnly at lines 108 and 310 in the same main() scope. |
Rename or scope the added test variables and build guest_filesystem_tests. |
| Read-only state is not restored after retry failure | NativeUnlink adds owner_write before the retry; if the retry also fails, it returns without restoring the prior permissions. |
Restore the original read-only state on failure and retain coverage for a held-open file. |
This overlaps issue #2601. Please coordinate with Achraf so the canonical patch retains their Windows evidence for both successful deletion and restoring the read-only attribute when deletion fails. The current automatic checks show Conventions only; focused tests and the repository-required collaborator-triggered Linux/Windows Build and Linux/Windows/macOS relinker matrix are still outstanding.
Windows validation update from issue #2601A new native Windows report is available on issue #2601. It tested Achraf's narrow
This confirms the regression and implementation on that branch under Windows; it is not validation of this PR's head. PR #2651 remains at |
|
Hi @sridhar777911, I reported this as #2601. It has a one-commit fix, It already addresses the points from the review above:
Validation so far on #2601:
|
|
Closing: the fix for #2601 has already landed via #2910 (single commit, rebased on This branch also can't be merged as is:
@oneandonlydean asked for a reset onto fbf0ec0 and there was no update. Welcome to open a new PR from a clean branch for anything else. |
What
Fixes Issue #2601. Unlink and sceKernelUnlink previously failed with EACCES on read-only files natively on Windows due to strict POSIX vs WIN32 semantic differences.
Tested
check_conventions.pyctestsuiteChecklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)