Repository navigation
fix(libc): open NUL on standard descriptors without a stream on Windows - #9945
oneandonlydean merged 8 commits into
Conversation
|
I reviewed head |
|
Status refresh: the Windows relinker job has now passed as well. The Ubuntu, Windows, macOS, ASAN, register-context and write-watch jobs in this matrix are all green; hosted product Linux/Windows builds remain skipped. |
|
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.
|
|
Thanks @MarcZX10. One correction to the summary: Halo doesn't reach a menu here. With this PR it runs its start-up (SDL, RADV's AI-assisted: yes (Claude Code) |
|
Tested this change on Windows 11 with an NVIDIA GeForce RTX 5090 (driver 617.42) on AMD Ryzen 9 9950X3D. Test Results
Code review of the diff looks clean, properly handles UCRT's AI-assisted: yes. Written with AI assistance and reviewed by me. |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 3875f8c and the UCRT _get_osfhandle docs, plus a standalone probe I compiled with WinLibs GCC 15.2.0 (UCRT):
- The approach matches main's Linux/macOS initializer (
StandardStreams.cppon main only has the#ifndef _WIN32branch), and it still merges cleanly. Thanks for the detached-child test. - I ran a standalone exe (not the libc DLL) that runs your initializer in a static initializer and starts itself
DETACHED_PROCESSwith null std handles. After the_dup2,_get_osfhandle(0..2)are real handles and_write(1)succeeds. But_fileno(stdin/stdout/stderr)stays -2:fputs(stderr)returns -1 with errno 9,fflush(stdout)returns -1 with errno 9, andfgetc(stdin)returns -1 withferrorset. - Main's
FileStreamtakes its guest descriptor from_fileno(FileStream.hpp:78-82), so_Stdout/_Stderr/inputwould keep descriptor -1, and guest stdio on them would still give EBADF, where on Linux the same streams write to/dev/null. Could the debt entry say this, or could the streams be re-attached? A plainfreopen("NUL", ...)landed on fds 3/4/5 in my probe, so it doesn't solve it as-is. - Optional: a check in the child that
fputstostdoutsucceeds would cover this.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
@p3n1cillin @harisibrar1 I reproduced the stream attachment and initialization-order problems with the actual DLL. The expanded test catches native stdin retaining -2 and the guest stdout prefix caching -1 before guest I found two more cases while checking the fix: closing an already closed descriptor again invokes the UCRT invalid-parameter handler, and reopening an already valid FILE on NUL destroys its existing destination. The latter matters for the preloaded-CRT state handled by #10157, where stdout can be valid on fd 3 while raw fd 1 is still -2. The updated follow-up branch handles all three states: unbound streams, bound streams whose raw descriptors were closed, and valid streams on separate descriptors. It initializes before Cherry-pick order, including the earlier two commits: The detached loader test fails on the previous version when stdout's descriptor changes, and passes with the follow-up. It also checks zero invalid-parameter callbacks and successful native/guest writes. DLL paths with spaces and Unicode pass too. Tested together with #10020 on |
|
@p3n1cillin The descriptor fallback still leaves a stream-initialization gap: as @harisibrar1 and @Queuse reported, the UCRT streams retain |
|
The refreshed overlap scan reports nearby StandardStreams.cpp and libc debt hunks. My proposed #8897 follow-up changes fgetwc conversion/state, while #9945 addresses standard-handle initialization. Since #9945 is still being revised for the UCRT stream attachment issue, would it be acceptable for #9945 to land first and for me to rebase afterward, preserving the initialization fix? I would record Depends on #9945 if agreed. AI-assisted. |
A Windows process started without a standard handle, such as a detached process or a --windows-gui executable started from Explorer, has descriptors 0 to 2 whose _get_osfhandle is -2. fcntl then threw on their unknown file type, which stopped Halo: Combat Evolved (PPSA99500) at start-up. Open NUL on each of them when libc loads, as Linux and macOS already open /dev/null on a closed standard descriptor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1366ebb to
41a1e6a
Compare
|
@harisibrar1, @Queuse, @oneandonlydean: thanks, you were right that Rerun on Windows 11, WinLibs GCC 15.2.0, Release, full build:
WSL Ubuntu 26.04.1, GCC 15.2.0: @MarcZX10, landing this before #8897 and rebasing that afterwards is fine with me. AI-assisted: yes (Claude Code) |
What
A Windows process started without a standard handle has descriptors 0 to 2 whose
_get_osfhandleis -2. The UCRT documents that value for a standard descriptor with no stream. It happens to a detached process and to a--windows-guiexecutable started from Explorer.DescriptorIsOpencounts such a descriptor as open, butGetFileTypefails on it, sofcntlthrows:Halo: Combat Evolved (PPSA99500, catalog build-113) stops there at start-up. Its PS5 host checks descriptors 0 to 2 with
fcntl(fd, F_GETFD)before opening anything, because, as its comment says, a title starts without them.On Linux and macOS, libc's static initializer already opens
/dev/nullon a standard descriptor that is closed when libc loads. The libc debt entry said the Windows case was not handled. This PR adds the Windows side,PrepareStandardStreams_nid_no_patch, whichFileStreamcalls before it captures a descriptor:NUL, read-only for 0 and write-only for 1 and 2._wfreopen, because_dup2alone leaves the stream's_filenoat -2. A stream bound to the raw descriptor closes itself through_wfreopen, so the descriptor is not closed twice.The stream attachment and the follow-up cases come from @Queuse's branch, cherry-picked with authorship kept, after @harisibrar1 found the stream gap. The debt entry describes both hosts and links the title's check. What the console gives a title as its standard descriptors stays unmeasured. As on Linux, a title here finds the descriptor open where the Halo port expects it closed, and it leaves that descriptor alone.
Tested
Windows 11 Pro (26300), WinLibs GCC 15.2.0, Release, full build on
main2ace103 plus these commits:guest_closed_standard_descriptors_windowsstarts itself as a detached child without stdin, without stdout, without stderr, and without all three. For each missing descriptor the child checks a character-device handle, the guestfcntlanswers forF_GETFDandF_GETFL, and that the native and guest streams use that descriptor. It also checks that writes and flushes on stdout and stderr succeed, and that stdin reads EOF without an error. Onmainthe child exits on the -2 handle. A non-detached child doesn't show the problem, because Windows gives a new console's process console handles in place of null ones.guest_closed_standard_streams_load_windowsloads libc into a process whose standard descriptors were closed, and into one whose stdout is already valid on a separate descriptor. It checks that no invalid-parameter callback runs, that the valid stdout is kept, and that native and guest writes succeed.core/libs/prx/libcandcore/libs/prx/libkernelregister pass.mainit aborts at 0 s with thefcntlerror above. With this PR it runs its start-up, with SDL 2.30.12, RADV'svkCreateInstanceand glslang. It then quits on its own check for the game files:the game needs /data/halo, with the game's disc image in it. I don't have the disc image, so nothing past that point was run.WSL Ubuntu 26.04.1, GCC 15.2.0:
libcbuilds, andguest_closed_standard_descriptorsand the threeguest_standard_streamstests pass. The non-Windows branch of the initializer is unchanged.tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.py --branchreports only #9596, which edits thefgetwsdebt line next to this entry's line. #8150 and #7201 also touchStandardStreams.cpp, in other functions.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)🤖 Generated with Claude Code