Repository navigation
fix(libc): fail reads on a write-only stream with EBADF on Windows - #14878
oneandonlydean merged 1 commit into
Conversation
|
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.
|
BunnyFl
left a comment
There was a problem hiding this comment.
Windows 11, WinLibs GCC 15.2.0 r7, Release (UCRT). Head e700ddb merged into upstream/main 93dbd0f:
file_streamspasses.- Negative control: with main's
FileStream.hppandStandardStreams.cpp,file_streamsaborts withfread on a write-only stream, the first new check. So main still serves the read from the write buffer as described.
Checking the UCRT _IOREAD/_IOUPDATE bits before the host is asked matches __srefill's "neither __SRD nor __SRW" case. Leaving the glibc fscanf difference as a debt entry, rather than special-casing Linux, seems right.
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (12, not counted against this PR)Validation IDs already present on main (4 IDs in 750 tests)Skipped (59)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
…eeBSD does On Linux, fscanf, vfscanf, scanf and vscanf on a stream opened only for writing returned EOF with EBADF but left the error indicator clear: SwitchToReadingUnlocked let the stream through to glibc's __vfscanf_internal, which rejects _IO_NO_READS without setting _IO_ERR_SEEN. FreeBSD 11.4's __srefill sets EBADF and __SERR. The glibc branch now fails an unreadable stream the way the Windows branch does since boykopovar#14878: errno 9, the guest error indicator, and false. OpenForReading gains a glibc branch (__freadable), so ungetc and ungetwc return EOF on such a stream without touching errno or the error indicator, as FreeBSD's __ungetc does and as Windows already did. Before, glibc pushed the character back onto the write-only stream. file_streams now checks fscanf, vfscanf, fgetwc, ungetc, ungetwc and clearerr on write-only streams on every host, not only on Windows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
On Windows, a guest read from a stream that is open only for writing (
"w"or"a";fdopen/freopenwith a write-only mode set the same UCRT flags and were not probed) reaches the UCRT. The UCRT serves the read from the unused part of the write buffer. Afterfwrite("abc")on a"wb"or"ab"stream,fread(buf, 1, 4)returns 4 zero bytes,fgetcandgetc_unlockedreturn 0, andfgetsreturns the buffer. Afterfwrite("12 "),fscanf("%d")returns 0 instead ofEOF. In every caseferrorstays 0 anderrnois not set.fclosethen writes the consumed bytes as well, so the 3-byte file ends up 4, 5, 7 or 10 bytes long.SwitchToReadingUnlockedonly acts on update streams; on any other stream it returnstrue(core/libs/prx/libc/include/FileStream.hpp:293on e0caf13; line numbers below refer to it), so a stream whose UCRT flags have neither_IOREAD(0x1) nor_IOUPDATE(0x4) goes straight to the host. FreeBSD 11.4's__srefillhandles a stream that has neither__SRDnor__SRWby settingerrno = EBADFand__SERRand returningEOF, and it does not touch pending output. POSIX.1-2008 listsEBADFforfgetcon a stream that is not open for reading. glibc's_IO_new_file_underflow(_IO_NO_READS) already does this forfread,fgetc,getc_unlockedandfgets, but its__vfscanf_internal(ARGCHECK, glibc 2.39stdio-common/vfscanf-internal.c) returnsEOFwithEBADFwithout setting the error indicator, so Linuxfscanfstill differs from FreeBSD there; this PR records that indocs/dev/debt/libc.mdand leaves the Linux code unchanged. macOS's BSD stdio is expected to behave like FreeBSD (not run). #11560 (a7b5208) added the opposite direction for Windows:SwitchToWritingUnlockedfails a write on a read-only stream witherrno9 and the error indicator (FileStream.hpp:329-334, tested atcore/libs/tests/FileStreams.cpp:447-457).SwitchToReadingUnlockedfails a stream whose UCRT flags have neither_IOREADnor_IOUPDATEthe way its writing counterpart does:errno9, the guest error indicator, andfalse. Every read path already returns on a failed switch, sofreadreturns 0 (core/libs/prx/libc/src/FileAndHeap.cpp:555);fgetc,getc,getchar,__srgetandfgetwcreturnEOF(StandardStreams.cpp:93),getsreturnsNULL(:176);getc_unlockedreturnsEOF(:150);fgetsreturnsNULL(:230); andfscanf,vfscanfand their_sforms returnEOF(Formatting.cpp:447,541,873). Pending output stays buffered, andfclosewrites only the guest's bytes. FreeBSD's__ungetc(ungetc.c) returnsEOFon such a stream without settingerrnoor__SERR, and today's guestungetcandungetwcalready do the same. A newFileStream::OpenForReading(UCRT flags & 0x5 on Windows,trueelsewhere) lets both of them return -1 before they reach the failing switch, so their result stays as it is.ungetwcstill checks its range first, as FreeBSD'sungetwcencodes before it calls__ungetc. The random-device stream opensNUL"rb"and is not affected; update streams are unchanged. Linux and macOS code is unchanged.fgetswithn== 1 fail withEBADFlike any other read. Open fix(libc): return NULL from fgets for a size below 2 as the console does #14863 makesfgetsreturnNULLfor any size below 2 before the stream is touched, as its author read from the console'slibc.prx; a trial merge of the two branches is clean, and with both, a size below 2 follows fix(libc): return NULL from fgets for a size below 2 as the console does #14863.file_streamsgainsCheckReadsOnWriteOnlyStreams, called next to the read-onlyEBADFblock inCheckStreamWritesAfterReads. For"wb"and"ab", afterfwrite("abc"), it checks each offread== 0,fgetc==EOF,getc_unlocked==EOFandfgets==NULLtogether witherrno==EBADFandferror!= 0, callingclearerrand resettingerrnobetween the calls. It also checksfscanf==EOF. On Windows only, it checkserrno==EBADFandferror!= 0 afterfscanf, and it checks thatungetc('q')returnsEOFwitherrno0 andferror0. Afterfclose, the file must hold exactlyabc. The assertions outside_WIN32follow glibc's_IO_new_file_underflow(fread,fgetc,getc_unlocked,fgets:EBADFand the error indicator) and__vfscanf_internal(fscanf:EOF), read in the glibc 2.39 sources, and BSD's__srefill; the error indicator afterfscanfis checked on Windows only because glibc does not set it.pr_overlap.py --branchreported one shared line, with fix(libc): reopen a stream onto /dev/random or /dev/urandom with freopen #10620 indocs/dev/debt/libc.md, while the new entry stood right after the /dev/random entry that fix(libc): reopen a stream onto /dev/random or /dev/urandom with freopen #10620 rewrites; the entry now follows thesetvbuf and setbufentry, and a trial merge of this branch with fix(libc): reopen a stream onto /dev/random or /dev/urandom with freopen #10620 and with fix(libc): give an unbuffered stream FreeBSD's one-byte buffer on Windows #12057 (which rewrites the update-stream entry) is clean. It cannot compare 26 pull requests that do not merge withmain(feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021, fix(libs): share Sony CRT handler and lconv state between libc and libSceAmpr #7081, feat(libc): scan 16-bit wide conversions on Linux #7386, feat(libc): support archive-backed stdio reads #7447, fix(libc): provide ASOBI_ROOT environment variable for title path resolution #7715, fix(libc): support character reads from random device streams #8150, fix(libc): use the Annex K size limit and codes of the PS5 libc, as measured on a PS5 #8965, feat(libc): decode non-ASCII scanset members and non-BMP characters in the guest wide scanf #9340, fix(libc): follow a Windows symbolic link before .. in a guest path #9371, feat(libc): add the std::__sce_v2 thread start, timed wait, clock and processor count calls #9447, feat(libc): add std::__sce_v2::_Random_device, _Fiopen and setw #9526, fix(libc): restore syslog program-name fallback #9929, fix(libc): compute expf with FreeBSD msun's algorithm, as titles' libc.prx does #10113, fix(libc): compute remainder and remquo with fprem1, as titles' libc.prx does #10349, fix(libc): share multibyte state in fgetwc #10405, fix(libc): bound narrow formatting fields #11332, feat(relinker): support short scalar guest GS instructions #11661, feat(relinker): support SSE guest GS memory accesses #11689, feat(relinker): support VEX guest GS memory accesses #11698, feat(libc): implement fcloseall with tracked stream lifetimes #11831, fix(libkernel): preserve DOS device names and trailing dots or spaces on Windows #12560, feat(libkernel): cancel blocked POSIX reads with owned operations #12682, fix(libkernel): preserve blocking sockets over nonblocking native transport #12688, feat(libkernel): cancel blocked POSIX writes with partial progress #12700, feat(libkernel): support deferred cancellation in POSIX accept #12965, docs(debt): separate entries with blank lines and drop derived export totals #14212). Checked by hand: a trial merge of this branch with each of them conflicts in exactly the files thatmainwith it already conflicts in. fix(libc): leave write mode when an unbuffered update stream switches to reading on Windows #12036 changes the neighbouring update-stream line inSwitchToReadingUnlocked, fix(libc): give an unbuffered stream FreeBSD's one-byte buffer on Windows #12057 the unbuffered-stream setup, and fix(libc): translate native stream read errors #14560 translates host read errors at the same read paths (hosterrno9 stays 9); each merges cleanly with this branch, and none of them handles a write-only stream. docs(libc): name what happens on Windows when ungetc is called twice #14326 (ungetc) appends its entry todocs/dev/debt/libc.mdright after the samesetvbuf and setbufentry and changes no other file, so whichever of the two merges second has a one-line conflict there, resolved by keeping both entries.Tested
Windows 11, WinLibs GCC 15.2.0, build directory configured with Ninja, Release and
BUILD_TESTING=ON, on this branch based on e0caf13:file_streams_testsalready built from it):cmake --build build --parallel 4 --target guest_stream_purge_tests file_position_tests guest_wide_stream_scan_tests guest_standard_stream_tests file_stream_status_tests guest_append_streams_tests guest_host_stream_tests, thenctest --test-dir build -R '^(file_streams|file_position|guest_standard_streams.*|file_stream_status|guest_wide_stream_scan|guest_stream_purge|guest_append_streams|guest_host_streams)$' --output-on-failurepassed 12/12.cmake --build build --target file_streams_tests, thenctest --test-dir build -R '^file_streams$' --output-on-failurefailed withwhat(): fread on a write-only stream(exit 0xc0000409). The Windows-onlyungetcassertion already passes before the change; it makes sure the newEBADFpath does not reachungetc.cmake --buildcommand withfile_streams_testsadded, then the samectestcommand: 12/12 pass (file_streams0.34 s).libc.prxand called its exports. For"w"and"a"afterfwrite("abc"),freadreturned 0,fgetcandgetc_unlocked-1 andfgetsNULL, each withferror1 anderrno9, and the file was 3 bytes long afterfclose.ungetc('q')on a"w"stream returned -1 withferror0 anderrno0. Afterfwrite("12 ")on a"w"stream,fscanf("%d")returned -1 withferror1 anderrno9, and the file stayed 3 bytes. With thelibc.prxof 73f4f7e (none of the read paths above changed up to e0caf13;FileAndHeap.cppchanged only infopen_s/freopen_s), the same probes reportedfread4 (file 7 bytes),fgetc0 (4 bytes),fgetssuccess (10 bytes),getc_unlocked0 (4 bytes) andfscanf0 (5 bytes), all withferror0 anderrno0, andungetc-1 withferror0 anderrno0._WIN32were checked against the glibc 2.39 sources (libio/fileops.c_IO_new_file_underflow,stdio-common/vfscanf-internal.cARGCHECK) and FreeBSD's__srefill; macOS is expected from the BSD sources only.python tools/check_conventions.py --base e0caf13600 --head HEADwith this title and description passed.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)