Repository navigation
fix(libc): keep the host standard stream when a guest freopen of it fails - #5668
Conversation
2f9026c to
881a109
Compare
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
|
I found a regression in FileStream::Reopen on head 881a109: successful exclusive creation of a standard stream now fails. For a fresh filename, FileStream(stdout).Reopen(path, "wx") succeeds on base d8888df. On this PR, _wfopen creates the file during the probe; the following _wfreopen of the same path with "wx" fails with EEXIST (errno 17). The guest gets a failed reopen even though its call created the file. I compiled the exact base and candidate FileStream.hpp headers independently on Windows using WinLibs GCC 15.2, C++20, O2. All ten paired runs gave base=true/errno=0 versus candidate=false/errno=17/file_exists=true. The Windows handle-preservation and file-lock integration callbacks were no-ops in this focused harness; this is method-level evidence, not a full runtime or PS5LM run. The existing missing-directory test does not cover this success path. Could the fix avoid opening the target twice with its destructive/exclusive mode, and add a regression for successful "wx" creation through a standard stream? The original missing-directory/host-output regression should remain covered as well. AI-assisted: yes, investigated with Codex. No replacement implementation is proposed in this report. Update: I also reproduced this through the real guest libc exports, with no handle/lock callback stubs. The test calls freopen_nid_postfix(path, "wx", __stdoutp_nid_postfix), then fputs_nid_postfix/fflush_nid_postfix and checks the resulting payload; all calls use the guest sysv_abi convention and errno is read through __error_nid_postfix. This was a controlled local integration: validation commit 4879dc913 plus this PR's four-file patch applied cleanly, rather than a build of the isolated PR head. After rebuilding libc, all ten candidate processes failed with nullptr/errno 17 and an empty created file. After reversing the patch and rebuilding, all ten control processes successfully created the file and verified the payload. Meanwhile guest_standard_streams, guest_standard_streams_failed_reopen and file_streams each passed five runs on the candidate (15/15). No title execution is claimed. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the fix, and for the PS5LM numbers. Checked the diff against the new head; one regression blocks it, the one @UmillD reported:
core/libs/prx/libc/include/FileStream.hpp:96-106: the probe opens the target with the guest's own mode before the real reopen. With"wx"the probe creates the file, so the followingfreopenof the same path fails withEEXIST. A validfreopen(fresh, "wx", stdout)that succeeds onmainnow fails and leaves the stream detached. The same double open also truncates/creates twice for"w"/"a".core/libs/tests/GuestStandardStreams.cpp:497-501: only the failing missing-directory case is covered. Please add a regression for a successful"wx"reopen of a standard stream (write through it and check the file contents), and keep the existing one.
Direction: avoid opening the target twice. For example, keep a dup of the host stream's descriptor, run the real freopen, and on failure restore the descriptor with dup2 (or _dup/_dup2 on Windows) and detach the guest stream; that way no destructive probe is needed. Your call on the exact design.
|
Windows run on a second PS5LM entry: PS5LM 0.9.0 (PPSA99575, catalog release; the PR text tested PPSA99581 Plus). Its eboot contains Windows 11 Pro build 26300, Intel i7-11700F, NVIDIA GeForce RTX 3070, WinLibs GCC 15.2, Release, relinked with
With gdb on the unfixed build, the first Note for the rebase: on current AI-assisted: yes |
881a109 to
2d8d3b9
Compare
|
@adriadam10 Reworked as suggested, and rebased onto
Thanks @UmillD for the |
What
A guest
freopenofstdin,stdoutorstderrreopens the host's own standard stream, since the guestFileStreamwraps it. When thatfreopenfails, the hostfreopenhas already closed the host stream. From then on, every host-side print to it fails: on Linux silently withEBADF, on Windows through the UCRT invalid-parameter handler, which reportsFATAL: UCRT invalid parameterand dumps all threads.Now, for a host standard stream:
FileStream::Reopenduplicates the stream's descriptor (DuplicateHostDescriptor_nid_no_patch) and holds the stream lock across thefreopen. If thefreopenfails, it reopens the host stream on the null device, moves the duplicate back onto its descriptor withdup2(_dup2on Windows) and closes the duplicate. The guestFileStreamstays closed, so the guest sees a closed stream as on FreeBSD (fix(libc): fail stream calls on a stream that a failed freopen closed #5632). The target is opened once, by the realfreopen, so"wx","w"and"a"behave as onmain.freopen_nid_postfixfails path resolution, it detaches the guestFileStreaminstead of callingClose(), so the host stream is never closed.stderralready moves the host onto its kept copy (KeepHostStandardError_nid_no_patch), so onlystdinandstdoutneed the restore there.Streams that aren't standard behave as before.
Tested
On the PR head 2d8d3b9 (rebased onto
main5bf2e13), Linux (Ubuntu 24.04, GCC 13) and Windows 11 (MinGW-w64 GCC 15.2 from WinLibs), Release:guest_standard_streams,guest_standard_streams_closed_stdout, the newguest_standard_streams_exclusive_stdoutandfile_streamseach pass 5/5 (--repeat until-fail:5) on both.--closed-stdoutnow also writes and flushes the hoststdoutafter the failed guestfreopen, then does the same for a failed"a"reopen ofstderr; it fails againstmain's library (on Windows with0xC0000409).--exclusive-stdoutreopensstdoutwith"wx"on a fresh file, writes through it and checks the bytes; it fails against the previous head's probe (the reportedEEXIST).PS5LM (PPSA99581 Plus 0.8.3 and PPSA99575 0.9.0) calls
freopen("/data/PS5LM/app.log", "w", stdout)and then reopensstderrwith"a"at start-up, and/data/PS5LMdoesn't exist:Linux, headless (SDL offscreen), 3 runs each (library code identical to the PR head; only the test file changed since): both stop at the same
DisplayWindowVulkan check on both builds. Onmainthe 12 host log lines after thefreopen(Vulkan device setup) are missing from stdout; with this PR they are there.Windows 11 (RTX 3080), 60 s, 2 runs each, PS5LM Plus 0.8.3 and PS5LM 0.9.0:
FATAL: UCRT invalid parameterreportsmain5bf2e13Transferring control to ELF entry point)With this PR the PS5LM Plus dashboard and the PS5LM 0.9.0 "Choose a model" page draw, and host lines keep reaching
stderrafter the failed reopens ([HUI] pad open,[HUI] audio open, theno modelsnotification). Onmain,stderrholds only the UCRT reports and their backtraces.PS5LM Plus 0.8.4, now in the catalog, creates
/data/PS5LMitself (it opens/data/PS5LM/.probewithO_CREAT), so itsfreopensucceeds and it no longer reaches this path; PS5LM 0.9.0 still does.No new warnings in the files this PR touches on either OS.
check_conventions.pypasses.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)Reopen; whichever lands second needs a small merge.