Repository navigation
fix(libc): convert scanf integers as FreeBSD does on Windows - #9479
adriadam10 merged 6 commits into
Conversation
boykopovar#5070 adds its own check before main() and rewrites the one-line main(), so defining CheckIntegerScanWidths there and calling it from main() made the two pull requests conflict. It now sits before RunChecks and runs after the other scanf checks.
MarcZX10
left a comment
There was a problem hiding this comment.
The unsigned narrow-integer path has undefined behavior: the rewritten %llu/%llx/%llo formats are given a long long*, and the results are stored through signed char*, short* or int*, including for unsigned destinations. I left an inline comment at the temporary declaration with the required signedness fix and tests. The current hosted Linux/Windows build/test jobs are skipped, and the relinker matrix is still running; I have not run a local test.
A %u, %o or %x scanned through the 64-bit temporary now writes an unsigned long long and is stored through an unsigned destination, as the rewritten %llu, %llo and %llx require.
|
Follow-up to my review: the relinker matrix, register-context, write-watch, eligibility, conventions, labels, and report jobs have now completed successfully. The full Linux and Windows build/test jobs were skipped by the PR workflow. The signed/unsigned temporary and destination-pointer issue remains, so the requested code change is unchanged. |
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The latest commit fixes the signedness issue from my earlier review: %u/%o/%x now scan through an unsigned long long temporary and store through unsigned destination types, while signed conversions retain signed temporaries/destinations. The added unsigned narrow cases cover truncation, and the author reports the focused Windows/Linux formatting tests and hosted relinker/platform matrices pass; full Linux/Windows product jobs were skipped.
One repository coordination blocker remains. #9087 is open and edits the same Formatting.cpp test function/call site. The body says whichever PR merges second needs a rebase, but the required merge order/dependency is not recorded and I do not see agreement from #9087's author about this overlap. Please coordinate and state the agreed order in the Depends on checklist (or confirm the changes are order-independent and record the agreed rebase owner), then preserve both test sets. No local tests were run here.
|
The signedness correction in the latest head resolves my prior code blocker. The remaining request is merge coordination with #9087 because both change |
MarcZX10
left a comment
There was a problem hiding this comment.
The latest head fixes the earlier signedness/undefined-behavior issue: unsigned conversions use an unsigned long long temporary and unsigned destination stores, while signed conversions use signed types. The extensive %d/%i/%u/%x/%o, narrow-width, %q, %n, overflow and failed-conversion cases are covered in the reported Windows/Linux focused runs. The #9087 ordering blocker is now resolved: both authors agree #9479 lands first and #9087 rebases its test changes afterward; the PR records that order. I am requesting changes only because the tested branch remains based on a6dcea9 while current upstream main is ab347d1. Please rebase, preserve both Formatting test sets, and rerun guest_formatting, conventions and the overlap scan on current main. Hosted full Linux/Windows product jobs are skipped; I did not run local tests.
|
Thanks, @MarcZX10. The branch now has |
|
I checked head 8fab129 against titles' bundled Setup. I merged it locally onto Method. A scratch loader runs a title's decrypted
So the overflow and truncation behaviour now matches the console. On The
That may be out of scope here, which is about integer width. I mention it because the FreeBSD |
|
Thanks, @JJenkx. That's the check this PR needed. The bare |
|
Console data for this PR. I loaded the decrypted
So the first part matches the titles' copies: narrow conversions wrap (truncate the 64-bit result) without The AI-assisted: yes (Claude Code) |
The console's own sscanf, run natively from three titles' libc.prx, returns 0 for %qd and stores nothing, where FreeBSD 11.4's source reads q as ll. A q directive now ends the scan there, and the count of conversions before it is returned. The test adds the console's rows for wrapping narrow conversions, saturating 64-bit ones with ERANGE, and %qd.
|
Thanks, @JJenkx, the console's own |
|
Measured on a PS5 (a payload calling the console's |
|
@rooklz thank you for measuring the console's own |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
What
On Windows the scanf family (
sscanf,vsscanf,fscanfand their callers) translates the guest's length modifiers and hands the format to the host's scanner. Two integer cases differ from FreeBSD 11.4'svfscanf.%d,%i,%o,%u,%xor%Xwithout a length modifier, or withhorhh, goes to the host unchanged. The host converts it with its 32-bitlong, so an out-of-range value is clamped anderrnoset toERANGE. FreeBSD converts every integer withstrtoimaxorstrtoumaxand stores the result truncated to the destination type (vfscanf.cL741-L771).q, FreeBSD's quad length modifier, is not recognized, so%qd,%quand%qxthrowNotImplemented_nid_no_patch("vsscanf format conversion"). FreeBSD's source treatsqlikell(vfscanf.cL529-L531), but the console's own libc doesn't: run natively from three titles, itssscanfreturns 0 for%qdand stores nothing (@JJenkx's comment). The console's behaviour overrides FreeBSD 11.4's source here.Measured with
main's code:"2147483648"%derrno34"3000000000"%ierrno34"4294967296"%uerrno34"100000000"%xerrno34"3000000000"%qdA colour or mask written as
0xFFFFFFFFand read with%iinto aninttherefore comes back as0x7FFFFFFF.ScanWindowsArguments_nid_no_patch(WindowsScanning.hpp) now does two things:qdirective ends the scan there with nothing stored, as the console's libc does for%qd, and the count of conversions before it is returned. What the console does with aqafter other conversions isn't measured.llinto a temporary of the conversion's signedness:long longfor%dand%i,unsigned long longfor%o,%u,%xand%X. A%nafter it shows whether the conversion took place. The value is then stored truncated to the destination'sint,shortorchar, signed or unsigned to match, as FreeBSD's assignment does.A value beyond the 64-bit range still sets
ERANGE, asstrtoimaxdoes there. Linux hands the format to glibc, which already truncates this way but readsqasll; the libc debt file records that difference. No title is claimed.Overlap, merge order agreed: #9087 also adds a check function after
CheckWidePrecisionincore/libs/tests/Formatting.cppand a call at the end ofRunChecks, so the two conflict in that test file. Their library changes are in different files: #9087's inFormatting.cppandWindowsFormatting.hpp, this PR's inWindowsScanning.hpp. This PR merges first, and #9087 then rebases its test file over it, keeping both check functions and both calls; orezraey agreed to that order (comment).Tested
On GitHub's runners from my fork, merged with
mainat a6dcea9 (run):CheckIntegerScanWidthsinguest_formattingchecks:%d,%i("0xFFFFFFFF"gives -1),%u,%x,%o,%hd("5000000000"gives -3584) and%hhd("300"gives 44), witherrnoleft alone;ERANGEfor"99999999999999999999"with%d(-1);%uof4294967295,%huof40000and of70000(4464),%hxofffff,%hhuof200and of300(44), and%hhoof377(255);%qdand%qx;%nstill counts after the new conversions, and that a failed conversion leaves its destination unchanged.main'sWindowsScanning.hppguest_formattingfails. With the change it passes 10 runs in a row.guest_formattingpasses withmain's code and, 10 runs in a row, with the change; the code change does not apply there, and glibc already gives these results.core/libs/prx/libcandcore/libs/prx/libkernelpasses: 115 and 197 on Linux, 142 and 207 on Windows.guest_formattingpasses 10 runs in a row on both. The new unsigned checks also pass with the previous head, since both hosts store the same bits there. In that run's Windows libkernel suite,guest_raise_in_system_calltimed out after 30 s with and without the commit.mainat da54c65 (8fab129, no conflicts, the PR's own diff unchanged; run):guest_formattingfails on Windows withmain'sWindowsScanning.hpp; with the change,guest_formattingandguest_wide_formattingpass 10 runs in a row on both runners. The libc and libkernel suites pass on both (116 and 197 on Linux, 143 and 207 on Windows).tools/check_conventions.py --base origin/mainpasses.libc.prx, by @JJenkx (comment): head 8fab129 merged ontomainf1db096, on Windows 11 with GCC 15.2.0 UCRT, Release. A loader runs a title's decryptedlibc.prxnatively and compares itssscanfwith this build's on 60,000 random inputs per format, checking the result, the value, the%ncount anderrno. The copies bundled with three titles give the same counts.%d,%u,%hdand%hhdmatch the console on all 60,000 inputs each. Onmain16,892, 7,541, 7,455 and 7,499 of them differ.%iand%xgo from 20,669 and 14,643 differences onmainto 3,521 and 3,597.%o,%lldand%3dmatch on both trees, and%lliand%llxdiffer as much as onmain.%iand%xdifferences left, here and onmain, are one case: a0xprefix with no hexadecimal digit after it, which the console treats as a matching failure. That matching rule is outside this PR.ctest -R 'scan|format|stream|libc|wide|string'selects 44 tests, and all pass in three runs of three.sscanfon fixed inputs, by @JJenkx (comment): thelibc.prxbundled with PPSA07923, PPSA13934 and PPSA35169, loaded natively on Windows, into a 64-bit destination filled with0x55…; all three give the same results.errno:2147483648and-2147483649with%dgive0x80000000and0x7fffffff,4294967296and-1with%ugive 0 and0xffffffff,99999999999with%xgives0x99999999,300with%hhdgives0x2cand70000with%hdgives0x1170. Only the 64-bit ones saturate withERANGE:%lldand%llupast their range.%qdof5returns 0 and stores nothing, which this PR now does too (a809ad7).qchange (run):CheckIntegerScanWidthstakes those rows, and%qdof5must return 0 with the destination unchanged (Windows only, since Linux goes to glibc).WindowsScanning.hpp:guest_formattingfails. On Linux it passes before and after, as the change doesn't apply there.guest_formattingandguest_wide_formattingpass 10 runs in a row on both runners, 2 of 2 each, none skipped. The libc and libkernel suites pass on both, run withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.fscanfon a file in/data.%don99999999999returns 1 and stores0x4876e7ffwitherrno0,%hhdon300stores0x2c,%uon-1stores0xffffffff, and%lldon-9223372036854775809storesINT64_MINwitherrno34: the FreeBSD conversion this PR gives Windows, as they say. They also report two differences from glibc outside this PR:%ion0xreturns -1, and%xon0xZreturns 0 withZnext, where glibc returns 1 with 0 stored for both.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)