Repository navigation
fix(libc): return NULL from fgets for a size below 2 as the console does - #14863
Conversation
|
Reviewed at 2b3de79. Thanks for tracking down the The check is So for a size below 2 it returns NULL. It doesn't lock the stream, write the buffer, set errno or touch the stream flags. That means
Linux and Windows then give the same result, whatever the host C library does. |
2b3de79 to
262c040
Compare
|
@oneandonlydean Thanks, that's the better evidence. I found the same entry in the I have changed the PR to follow it, and widened it slightly, since that check covers every size below 2: sizes of 0 and less ( |
|
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.
|
|
@msbutt1 I tested this on Linux (WSL Ubuntu 22.04, GCC 11.4, glibc 2.35, Release) against current Merged onto
So the rebase needs two more edits besides the conflict: Hosted Linux CI on AI-assisted: yes (Claude Code) |
BunnyFl
left a comment
There was a problem hiding this comment.
Checked against the system libc in firmware 13.42 (libSceLibcInternal.sprx, sha256 a01795ab49df639dd40abd6194956ada062244826824ce486c9acab227bf5285), where fgets (KdP-nULpuGw) is exported at 0x60090. It matches your reading of the title's libc.prx:
600a1: cmp $0x2,%esi
600a4: jge 0x600ad
600a6: xor %ebx,%ebx
600a8: jmp 0x6022e
6022e: mov %rbx,%rax (epilogue, ret)
Below 2 nothing is called and neither the buffer nor the stream is read or written, so if (size < 2) return nullptr; as the first statement is right. It also removes a null dereference on main: there, fgets(buf, 0, NULL) passes the stream && guard and then calls stream->SetError() on a null pointer. With this PR it returns NULL like the console. (For a size of 2 or more the firmware dereferences the stream with no null check, movzwl (%rdi) after the first call, so the remaining null-stream dereference there matches the console.)
Tested on Windows 11, WinLibs MinGW GCC, at 262c040: all five guest_standard_streams* tests pass. With only StandardStreams.cpp restored from the merge-base, guest_standard_streams aborts (0xc0000409), so the tests catch the old behaviour on Windows too.
For the rebase: the conflict is with main's new if (stream) stream->Orientation(-1); line in fgets_nid_postfix. Keep the size check ahead of it, because the firmware returns before touching the stream, so a size below 2 must not set the stream's orientation either. I'll re-review once the rebase is pushed.
262c040 to
4f6d9d4
Compare
|
Rebased onto
Tested on Ubuntu 22.04.5 (glibc 2.35), GCC 13.4, Release:
Overlap check for the PRs the bot couldn't compare: none implements the same behaviour.
|
|
@msbutt1 Re-tested
AI-assisted: yes (Claude Code) |
BunnyFl
left a comment
There was a problem hiding this comment.
Re-checked after the rebase: approve
Head 4f6d9d4b1f keeps the ordering the firmware needs. In 13.42's libSceLibcInternal.sprx (sha256 a01795ab49df639dd40abd6194956ada062244826824ce486c9acab227bf5285), fgets at 0x60090 returns NULL at 0x600a1-0x600a8 for a size below 2, before it reads the stream. This PR's fgets_nid_postfix now:
- returns
nullptrforsize < 2first; - then does the closed-stream check from
main; - then calls
stream->Orientation(-1).
So a size below 2 leaves the stream unoriented, as on the console.
The GuestFwide.cpp change (wide(stream, 0) == 0 after fgets with size 0) and the fwide debt entry follow from that ordering.
Tested
Windows 11, WinLibs GCC 15.2.0, Release, BUILD_TESTING=ON. Head 4f6d9d4b1f sits on current main.
| Run | ctest -R '^guest_(fwide|standard_streams)$' |
|---|---|
| This PR | 2/2 pass |
Same tree with main's core/libs/prx/libc/src/StandardStreams.cpp |
0/2; both abort (0xc0000409) |
❌ AMD GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
guest_pthread_start: Timeout (also failed when re-run alone), assertion and diagnosticsUnstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (13, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 752 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. |
What
fgetsnow returns NULL for any size below 2, as the console's does, and checks the size before anything else.The console's
fgets(NIDKdP-nULpuGw, in thelibc.prxa title ships insce_module) begins withcmp esi, 0x2/jgeto its main path; below 2 it clears the return register and jumps to the epilogue, which returns it. It does not lock the stream, write the buffer, seterrnoor change the stream's flags. The same entry is in Cult of the Lamb (PPSA06465, SDK 5.00) and Sifu (PPSA03001, SDK 3.00), read from my dumps.fgets_nid_postfixinstead:errnotoEINVAL(from FreeBSD, in 17e03d3);fgets, whose result depends on the host C library: the UCRT returns the buffer with an empty string, and on Ubuntu 22.04, where GCC defines_FORTIFY_SOURCE=2and the call is built as glibc 2.35's__fgets_chk, it returns NULL. Soguest_standard_streamsfails on Linux since test(libc): check fgets with a size of 1 stores only the null character #13529 addedCheckFgetsSizeOne, which expects the empty string.CheckFgetsSize(sizes 0, −1 andINT_MIN) now expects NULL, an untouched buffer,errnoand the error and end-of-file flags unchanged, and the position unchanged.CheckFgetsSizeOneexpects the same for a size of 1 at the start of the stream and at end of file.The size check also comes before the stream orientation that #10324 added, since the console returns before it reads the stream.
GuestFwide.cppnow expects a stream left unoriented afterfgetswith a size of 0, and the fwide entry indocs/dev/debt/libc.mdsays so. The firmware's ownfgets(BunnyFl's review) and the titles'libc.prxcopies match.Tested
Ubuntu 22.04.5 (glibc 2.35), Linux 6.8.0-138-generic, GCC 13.4, Release.
guest_fwide,guest_scan_count,guest_wide_formatting,guest_wide_stream_scan,guest_stdio_variadic, the fiveguest_standard_streams*tests,file_streams,guest_formattingandguest_filesystempass 3 of 3. With the updated tests andmain'sStandardStreams.cpp,guest_fwideandguest_standard_streamsabort.ctest -j 8on this head: 3,731 tests. Four fail, none in libc:agc_driver_htile_mask_lifetimeandguest_runtime_lifecyclefail the same way onmainhere,avplayer_http_patched_modulesfails with asymbol lookup errorwithmain'sStandardStreams.cpptoo, andagc_driver_image_table_vouched_lookupstimed out at 120 s under-j 8and passes alone (55 s).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)