Repository navigation
fix(libc): fail a 0x with no hex digit and keep errno for in-range floats in sscanf_s - #12530
Conversation
…oats in sscanf_s The libc.prx that titles ship (PPSA05916, PPSA07923) does not take a 0x prefix with no hex digit after it as 0. A %x, %X, %i or %p field fails as an input failure when the input or width ends after the x and no sign precedes it, and as a matching failure otherwise; a floating field needs a hex digit, or a dot and a hex digit. Floating fields set ERANGE only when a decimal field is out of long double range, so %e of 1e40 and %a of 0x1p200 leave errno alone. sscanf_s and vsscanf_s now check the prefix before handing the field to the host scanner and decide errno from the scanned text. The stream forms cannot look three characters ahead and are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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.
Reviewed — looks good
The second stacked commit’s prefix checks and string-scan errno handling match the measured behavior described in the PR, and I found no blocking issue in that incremental diff.
What I ran
Reviewed commit 3eca2be06731a86e6dc6a7cf07259dc1378eb149 on parent PR #12507 head 0b3eb0b3e53a8faa1f01fd01a6de4061247ab308. The cumulative head merges cleanly onto current main f21ef7c99c5af648c5a41bddb18ce118b8e60c46. Environment: Debian 12 x86_64. I checked the incremental diff, merge tree, and whitespace; I did not build or execute the formatting tests locally.
git diff pr/12507...pr/12530 -- core/libs/prx/libc/src/Formatting.cpp core/libs/tests/Formatting.cpp docs/dev/debt/libc.md
git diff --check pr/12507...pr/12530
git merge-tree $(git merge-base upstream/main pr/12530) upstream/main pr/12530
gh pr checks 12530 -R boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Incremental diff from #12507 | Reviewed; limited to prefix parsing, floating range errno, tests, and debt documentation |
Merge onto current main |
Clean |
git diff --check |
Pass; no output |
| Conventions, eligibility, intake, labels | Pass |
| Hosted Windows job | Pass; selected-test, build, and CTest steps skipped |
| Linux | Skipped |
| Focused differential and regression tests | Author reports targeted comparisons matched both title libc builds and the regression set passed 3/3; not independently rerun by me |
| macOS jobs | Pending when checked |
Command output
$ git diff --check pr/12507...pr/12530
(no output)
$ git merge-tree $(git merge-base upstream/main pr/12530) upstream/main pr/12530 | head -3
merged
result 100644 464536c25d2f7b531cd37b16a664125621c0933f core/libs/prx/libc/src/Formatting.cpp
our 100644 ac280da5abaf8689e7817e9507167dfd17e18ad1 core/libs/prx/libc/src/Formatting.cpp
$ gh pr checks 12530 -R boykopovar/AnyPS5
conventions pass
eligibility pass
intake pass
labels pass
register-context (ubuntu-latest, Debug) pass
register-context (ubuntu-latest, Release) pass
relinker (ubuntu-latest, g++) pass
relinker (windows-latest, g++) pass
windows pass
relinker-asan pass
report pass
register-context (macos-15-intel, Debug) pending
register-context (macos-15-intel, Release) pending
relinker (macos-latest, clang++) pending
relinker (macos-latest, clang++, x86_64) pending
write-watch (macos-15) pending
write-watch (macos-15-intel) pending
linux skipping
Negative control
| Assertion | Change under test | Did the new test fail? |
|---|---|---|
| Prefix failures and in-range floating conversions retain the measured behavior | Not independently run by me; author reports guest_formatting fails with #12507’s prior implementation |
Author reports yes; not independently reproduced |
Findings
Blocking
None found in the incremental change.
Non-blocking
The focused tests and title comparisons are author-reported. The hosted Windows job skipped CTest; Linux was skipped and macOS checks were pending when checked.
Sources
- Prefix and range handling at the reviewed commit
- Focused tests at the reviewed commit
- Documented behavior and platform limitation at the reviewed commit
Not verified
I did not independently build/run the tests, use the title comparison loader, test on macOS or a console, or rerun the author’s random/targeted probes.
To get this to a good state
No follow-up is needed for this incremental review. If the commit changes, I will re-review its diff and rerun the merge and whitespace checks.
|
@JJenkx the content of this PR looks right to me; one thing to sequence before it and #12468 can both land. #12468 deletes Whichever of #12468 and this PR lands second needs |
|
A merge-order note from a local integration run. I merged this stack (#12507, this PR, #12532) with my #12428 and #12468 on
With both handled, the combined tree passes Against the
AI-assisted: yes (Claude Code). |
|
Linux run at this PR's exact head Arch Linux, kernel 7.2.9, Ryzen 9 7950X, g++ (GCC) 16.2.1 20260810, glibc 2.44, x86-64. Release,
|
|
For my own PRs, the landing order is #12507 → #12530 → #12468. #12468 now retains The final #12468 head Raw qualification bundle, SHA-256 AI-assisted: yes (Codex). |
boykopovar#12530 reads source.Remaining() in the contiguous branch of ScanSource, so dropping the accessor here would break the build whichever of the two lands second, without a textual conflict. AI-assisted: yes
|
A Windows run with GCC 16: Windows 11 Pro 10.0.22631, i9-9900K, MSYS2 UCRT64 GCC 16.1.0, Release, this PR (
AI-assisted: yes (Claude Code) |
|
Windows check for the scheduled cross-platform review, at head Setup: Windows 11 Enterprise 22621, WinLibs GCC 15.2 UCRT, Release. The hosted Windows check does not run these tests on pull requests.
AI-assisted: yes. |
✅ AMD GPU test: passedPR head
Unstable on main (5, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (14, 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)Validation IDs already present on main (4 IDs in 807 tests)Skipped (61)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. |
…hex-prefix # Conflicts: # core/libs/prx/libc/src/Formatting.cpp
|
Refreshed onto Windows, WinLibs GCC 15.2 UCRT, Release:
AI-assisted: yes. |
|
Linux check of exact head
AI-assisted: yes |
✅ AMD GPU test: passedPR head
Unstable on main (7, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (10, 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 (67, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 808 tests)Skipped (71)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. |
|
Re-checked against current main |
Refreshed against main
0263899e996f26b165aed37f6b5a616dd1d4c6bb; current headaf0443d37194a7000804f22c1e71c82cae572dbdretains previous public head3eca2be06731a86e6dc6a7cf07259dc1378eb149and main as ancestors. The conflict resolution preserves the topic behavior and current-main ordinary-scan/input-failure handling and fixtures.The same five fixtures (
guest_formatting,guest_scan_count,guest_wide_stream_scan,guest_wide_formatting,guest_crt) pass three times on main and on this head: 15 executions per arm, zero failures/skips. Public-header audit, conventions and whitespace checks pass. These are actual scoped libc/kernel/Ampr owners and selected fixtures with existing frozen ZArchive/zstd providers, not a whole-project build. No new Windows, title-unblock or performance claim.Raw build/test/JUnit logs, commands, source patches, conflict resolutions and binary hashes, SHA-256
fd8ddb6a38a010916d71698df9cb60011e9800fe14db63baa9f4eca21e6b8e4f(download verified). The batch has 87 successful fixture executions in total; setup failures and corrected final qualification are retained in the archive.Previous report and contribution template, with historical pins preserved:
Stacked on #12507. It builds on #12507's change to the same lines of
ScanSource; review the second commit,3eca2be067.What
sscanf_sandvsscanf_snow handle two cases the way thelibc.prxthat titles ship does.1. A
0xprefix with no hex digit after it.mainhanded these fields to the host scanner, which takes0xas 0 and counts a successful conversion. The titles fail the conversion and store nothing:%x,%X,%ior%pwhere the input or the width ends right after thex, with no sign ("0x"," 0X","0x1"with%2x)EOF, or the count so far after an earlier conversionx("0xg"), or with a sign ("-0x","-0x1"with%3x)%a,%e,%f,%g).and a hex digit, within the width ("0x.8"is 0.5); anything else is a matching failure, neverEOF%d,%oand%uread the0and are unchanged. The check runs before the field reaches the host scanner.2.
errnofor floating fields. UCRT setsERANGEwhen%eof1e40overflows afloat, and when%aof0x1p200does. The titles set it only when a decimal field overflows, or underflows to 0, as along double, whatever the destination width; otherwiseerrnois left alone. This is the same rule #12514 measured forswscanf. Examples:%Leof1e5000and%Lgof1e-5000setERANGE;%leof1e400and%Laof0x1p20000don't.errnois now decided from the scanned text withstrtold.Limits (new debt entry):
fscanf_s,scanf_sand theirvforms) can only look one character ahead, so they keep the old behaviour.long doubleisdouble(macOS on ARM), a decimal field beyonddoublerange setsERANGE.Tested
Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
BUILD_TESTING=ON, head3eca2be067(on #12507, base6793a72475).libc.prxof PPSA07923 and PPSA05916 and calls theirsscanf_sby NID.%nanderrno.main(470a562903) has 136 differences in PPSA07923: 780xfailures, 37 floaterrnocases, fix(libc): wrap narrow integers in sscanf_s as titles' libc.prx does #12507's 20 wrap cases and the NaN case below. This PR has 1 per title: a%5Lfof"-naNXf"that differs past the 8 bytes the probe prints, most likely the NaN's sign. That is a separate issue and isn't touched here.%i,%p, suppressed fields, a failure after an earlier conversion) and 15 range cases (%e,%le,%Le,%a,%La,%Lg, 64-bit integer overflow). 0 differences in both titles.guest_formattinggains these cases. With fix(libc): wrap narrow integers in sscanf_s as titles' libc.prx does #12507'sFormatting.cppand the new tests,guest_formattingfails.guest_formatting,guest_stdio_variadic,guest_scan_count,guest_wide_stream_scan,guest_wide_formatting,file_streamsand the fiveguest_standard_streams*tests pass 3/3. Conventions pass.git merge-treemerges this head cleanly with fix(libc): end sscanf and vsscanf at a q length on Linux and macOS #11852, fix(libc): match bundled narrow scansets and scan directive parsing #12428 and fix(libc): read wide scanf fields byte by byte, as titles' libc.prx does #12468, the other open PRs that touchFormatting.cpp.Integration and landing order — 2026-10-29
For my own PRs, the landing order is #12507 → #12530 → #12468. #12468 now retains
StringSource::Remaining(), which #12530 uses twice. A private tree exactly matching the merge of #12468 accessor fixa84100962ceb61a1646c04aa62bdcc254c8cec01and this PR at3eca2be06731a86e6dc6a7cf07259dc1378eb149builds and passes five focused Linux suites three times (15 checks). Removing the accessor produces the expected compile errors at those two calls; restoration passes all15 checks and restores identical libc bytes. The private combined source was not published.The final #12468 head
068725777be07527eda51e086d8850500b7b8707additionally integrates main157b and passes the same15 checks, with both formatting translation units passing MinGW cross syntax checks. This does not constitute new Windows runtime testing or external #11724 author agreement.Raw qualification bundle, SHA-256
ca6b35926bcc6d3f21a9650517894c86b65594e2e2b653c5c32cd1c1de89c835.AI-assisted: yes (Codex).
Independent reports on this head (their runs, summarised here):
3eca2be067merged intomainfae9e483bb:guest_formattingpasses 5/5; withFormatting.cppfrommainit fails 5/5.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