Repository navigation
fix(libc): reopen a closed stream in freopen instead of aborting - #9763
Conversation
9194191 to
a783c27
Compare
|
Rebased onto current |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 8b3e398 and FreeBSD 11.4 lib/libc/stdio/freopen.c:
- This merges cleanly, and main still throws from
Reopen→GetHandle()on a closed stream, so it isn't duplicated. The new branch matches FreeBSD's_flags == 0path: it opens on a new descriptor, and on failure the stream stays free with the open's errno. - One abort remains on the same path. When
ResolvePathError_nid_no_patchfails,freopen_nid_postfix(FileAndHeap.cpp:365-367) callsstream->Close()for anything that isn'tIsHostStandard(). A closed stream has a null handle, so it lands there, andClose()→GetHandle()throwsFileStream: closed stream. For example, afterfclose(stdout),freopen(<1024-byte path>, "w", stdout)still terminates, and so does a path under a blocked mount such as an unavailable/download0. FreeBSD returns NULL withENAMETOOLONG/ENOENTand leavesfpfree. Guarding that branch withstream->IsOpen()and adding the case toCheckClosedStreamReopenwould close it. - Optional, pre-existing: for a NULL filename, FreeBSD answers
EINVALwhenfpis closed, where main returns 45 before looking at the stream.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The closed-stream reopen path still has a failure case on the current head. I verified the control flow against this commit; the existing test covers an open failure after path resolution, but not a path-resolution error.
| const auto filename = "anyps5-closed-reopen-" + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count()); | ||
| FileStream stream(std::tmpfile()); | ||
| Require(fclose_nid_postfix(&stream) == 0); | ||
| Require(freopen_nid_postfix("missing-closed-stream-dir/out.log", "w", &stream) == nullptr && *__error_nid_postfix() == 2); |
There was a problem hiding this comment.
This failure case reaches FileStream::Reopen and tests the open failure, but it does not cover an error from ResolvePathError_nid_no_patch before Reopen is called. That path still aborts for a closed stream: freopen_nid_postfix at core/libs/prx/libc/src/FileAndHeap.cpp:365-368 calls Close() when the resolver returns an error; IsHostStandard() is false once _handle is null, and Close() calls GetHandle(), which throws. After fclose(stdout), a freopen using an overlong path or unavailable mount therefore still terminates instead of returning nullptr with the resolver error. Please add this resolver-error case for a closed standard stream and guard the caller’s close with stream->IsOpen() while retaining the standard-stream detach handling. I could not run guest_standard_streams locally because there is no configured build and less than 700 MB free; git diff --check and current-main conventions pass.
a783c27 to
4142bfa
Compare
|
Right, the resolver-error branch still called |
4142bfa to
1c2eed3
Compare
|
@harisibrar1 thanks, the resolver branch is the same one @MarcZX10 found and is fixed as above. I took the optional point too, since it is the same closed-stream path: head |
|
The hosted Linux run on |
MarcZX10
left a comment
There was a problem hiding this comment.
The latest head 1c2eed30a2d18862352c02b79f6fc7046349fb47 addresses both issues I raised: path-resolution failure only closes an open stream, and a null filename on a closed stream returns EINVAL. The new ordinary-stream and stdout tests cover resolver errors, ENAMETOOLONG, failed writes, successful subsequent reopen, and retained host stdout behavior. The author reports the focused Linux suite passing on this head; current hosted relinker/register-context/write-watch checks also pass, while product Windows is skipped. Current-main conventions, diff-check and merge-tree pass. This branch is based on 8b3e39832a, 32 commits behind current main (11f976b27c16ed91633f00039dba5dfd97dd7cc1); please refresh and rerun the stream tests before merge. I did not rerun them locally.
|
Windows run of 1c2eed3 merged onto Environment: Windows 11 Home 10.0.26200, WinLibs MinGW-w64 GCC 15.2.0 r7 (UCRT), Ninja, Release, With this PR: all four Baseline: I kept the PR's test and Full suite with this PR: 1887 of 1895 pass. The 8 failures are AGC tests that fail on this machine's AMD GPU on Code read: no findings. On Windows the closed-stream branch opens through AI-assisted: yes |
|
@Dom3005 thanks for the Windows run and the baseline comparison; the description's Tested section now lists it with your numbers. The |
1c2eed3 to
68eb26b
Compare
|
Rebased onto current |
|
Measured on a PS5: the error values for a missing, empty or overlong path match, and the reopen takes the lowest free descriptor. A null filename on a closed stream gives |
|
@MarcZX10 two loose ends here, both yours to close when you have a moment:
|
144298b to
4c28647
Compare
|
Rebased onto current |
What
freopenon a stream that had been closed aborted the process:FileStream::Reopenstarts withGetHandle(), which throwsFileStream: closed streamwhen the handle is gone. PPSA99630 (OpenMW, the Morrowind port in the homebrew catalog,v1.0.1, bring-up thread #9762) does that when/download0cannot be used: its firstfreopenofstdoutfails and leaves the stream closed, and a laterfreopenof it dies onmainbefore the title prints anything:(
0x35ddaintolibc.prx,freopen_nid_postfix.) FreeBSD'sfreopenkeeps a closedFILEand reopens it: withfp->_flags == 0it "holds on to it", opens the path on a new descriptor and makesfpthat stream (freopen.c, releng/11.4). A failed open leaves it closed, with the open'serrno.Reopennow does the same for a closed stream: it opens the path through the caller's opener and adopts the handle, or fails with the open's error (ENOENTfor an empty path, asopen("")gives there). A path the resolver rejects before any open (an overlong name, an unavailable mount) used to reachClose()on the closed stream and throw the same way;freopennow closes only a stream that is open there and returns the resolver's error. A null filename on a closed stream answersEBADF, as a PS5 does (an open stream keeps answeringENOTSUP, since mode-only reopening is not supported here). The guest state reset and descriptor bookkeeping that the open-stream path did at its end moved intoAdopt, which both paths use. A closed stream whose reopen fails keeps answeringEBADF, as before. Only the standard streams can reach this, since a dynamic stream is freed by itsfclose.The debt entry records the differences from the console: the new descriptor is whatever the host hands out, where a PS5 takes the lowest free one, and a write to the closed stream answers
EBADFhere (as onmain), where the console'sfputs/fputcleaveerrnounchanged and itsfprintfgivesEINVAL.Tested
fclose(stdout)orfclose(stderr),freopeninto a missing directory and of""givesENOENT, of a 320-byte pathENAMETOOLONG, of a null filenameEBADF; a good path returns the sameFILE*on the lowest free descriptor and reads back what was written. Verify run on144298bfca:guest_standard_streams*pass; its AGC failures aremain's on that runner.maina6dcea9279with no usable/download0dies with the lines above; with this change it runs through to its own data check and exits 0, as it does when/download0works (details on Morrowind (OpenMW) (PPSA99630) #9762). With a complete package the title doesn't reach this path, so this isUnblocks: none; the trace is the title's, the behaviour is FreeBSD's.guest_standard_streamsgainsCheckClosedStreamReopen(a closedtmpfilestream: a failed reopen into a missing directory givesENOENT, one with a 300-byte name givesENAMETOOLONGfrom the resolver, a null filename givesEINVAL, the stream staysEBADFafter each; then a reopen for writing, a write, a reopen for reading and a read, and the file holds the text) and the--reopen-closed-stdoutcase (fclose(stdout),printffails withEBADF, the overlong reopen fails withENAMETOOLONGandprintfstillEBADF,freopenreturnsstdout,printfandputsland in the file). Hosted Linux runner: the first verify run passed those tests on the diff before the resolver fix; the run on1c2eed30a2passes them too (Ubuntu 24.04, GCC, Release).ci/macos-series-2integration tree:--reopen-closed-stdout(with the overlong-path check),--closed-stdoutand--exclusive-stdoutpass; the plain case runsCheckClosedStreamReopenand then stops at that tree's pre-existingCheckFlushWhileReadingfailure (anfflushon a reading stream changes the next read there, before and after this change; noted for macOS support #1).1c2eed30a2merged ontomain11f976b27c: the fourguest_standard_streams*tests pass 5 of 5, including the new case andCheckClosedStreamReopen; withFileStream.hppandFileAndHeap.cpprestored frommainthe two closed-stream cases abort withwhat(): FileStream: closed stream. The full suite passes 1887 of 1895, the 8 failures being AGC tests that fail on that machine's AMD GPU onmain(AGC: f64 and division tests crash in amdvlk64 on RX 9060 XT (RDNA 4) since AMD driver 32.0.32015.2008 #7451, test(agc): read the sample order tests' depth through a sampled copy #9056, test(agc): align the array window test's texels at run time #8310).check_conventions.pypasses.pr_overlap.pyreports no conflicting lines; the shared files it could not compare belong to PRs that conflict withmain.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)