Repository navigation
fix(libc): store a scanned NaN as the default quiet NaN in sscanf_s - #12532
Conversation
|
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 — changes requested
On hosts where long double is 8 bytes, the new %Lf NaN normalization copies 10 bytes from an 8-byte temporary, which AddressSanitizer reports as a stack-buffer-overflow; it also does not encode the expected x87 80-bit quiet NaN.
What I ran
Reviewed head 92c6891d1cdec64888aac01dbabdcea24b97101e against its declared base e4da19aff06e25ae16b90f460dff22e8ac1eb786; the cumulative PR merges cleanly onto current main at f21ef7c99c5af648c5a41bddb18ce118b8e60c46. Environment: Debian 12 x86_64, GCC 12.2. git diff --check passed. I also compiled the new helper behavior with GCC’s -mlong-double-64 mode to model the 8-byte long double ABI noted for macOS ARM in this PR’s own documentation.
git merge-tree $(git merge-base upstream/main pr/12532) upstream/main pr/12532
git diff --check upstream/main...pr/12532
g++ -std=c++20 -mlong-double-64 -fsanitize=address -g /tmp/pr12532_ld80_probe.cpp -o /tmp/pr12532_ld80_probe
/tmp/pr12532_ld80_probeThe probe source applies the PR’s StoreDefaultNaN<T> implementation to a 16-byte guest buffer whose first eight bytes contain a negative binary64 NaN, with StoreDefaultNaN<long double>(guest, 10). The address sanitizer reports the 10-byte read from the 8-byte local value.
Results
| Check | Result |
|---|---|
Merge onto current main |
Clean |
git diff --check |
Pass; no output |
8-byte long double ASan probe |
Fails with stack-buffer-overflow at the memcpy(target, &value, bytes) operation |
| Non-ASan probe output | mantissa=7ff8000000000000 exponent=b2a0 sizeof(long double)=8; this is not the PR test’s expected x87 encoding (mantissa= c000000000000000, exponent=7fff) |
| Hosted Windows job | Pass; the selective CTest step was skipped |
| Linux and specialized checks | Linux skipped; several macOS jobs were pending when checked |
AddressSanitizer output
=================================================================
==2079376==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe5e0c2078 at pc 0x7f082884814b bp 0x7ffe5e0c2030 sp 0x7ffe5e0c17e0
READ of size 10 at 0x7ffe5e0c2078 thread T0
#0 0x7f082884814a in __interceptor_memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
#1 0x559816915685 in void StoreDefaultNaN<long double>(void*, unsigned long) /tmp/pr12532_ld80_probe.cpp:12
#2 0x559816915312 in main /tmp/pr12532_ld80_probe.cpp:19
#3 0x7f0828245249 (/lib/x86_64-linux-gnu/libc.so.6+0x27249)
#4 0x7f0828245304 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x27304)
#5 0x559816915130 in _start (/tmp/pr12532_ld80_probe+0x1130)
Address 0x7ffe5e0c2078 is located in stack of thread T0 at offset 40 in frame
#0 0x559816915523 in void StoreDefaultNaN<long double>(void*, unsigned long) /tmp/pr12532_ld80_probe.cpp:7
This frame has 1 object(s):
[32, 40) 'value' (line 8) <== Memory access at offset 40 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
SUMMARY: AddressSanitizer: stack-buffer-overflow ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy
==2079376==ABORTING
Negative control
| Assertion | Change to code under test | Did the check fail? |
|---|---|---|
| No read beyond the temporary and the guest receives the expected 80-bit NaN | No mutant/negative control run; the ASan probe reproduced the current code’s failure | Not applicable |
Findings
Blocking
StoreDefaultNaN<long double>(target, 10)assumes the host temporary contains at least 10 bytes. On macOS ARM, the PR’s own debt entry notes thatlong doubleisdouble, sosizeof(value)is 8. The 10-bytememcpyreads two bytes beyond the local object, and copying the host binary64 NaN bytes does not produce the 80-bit guest representation asserted by the test. Encode the guest’s 80-bit quiet NaN using an explicit guest-format representation (or an equivalent host-independent conversion), and add/run a test withsizeof(long double) == 8under AddressSanitizer.
Non-blocking
None identified in this check.
Sources
StoreDefaultNaNand its%Lfcall at the reviewed head- The caller that requests a 10-byte guest write
- The PR’s documentation noting
long doubleisdoubleon macOS ARM - The test assertion for the expected 80-bit NaN bytes
Not verified
I did not run the complete project test suite or execute this on a physical Apple Silicon host. The sanitizer probe models an 8-byte long double ABI and exercises the exact helper operation; hosted macOS formatting tests were not complete when checked.
To get this to a good state
Please make the NaN normalization safe when the host long double is 8 bytes and ensure it writes the guest’s 80-bit representation. Add a focused regression that runs with that ABI under AddressSanitizer. I will rerun the same probe and review the updated diff on the new head.
rooklz
left a comment
There was a problem hiding this comment.
Reviewed the top commit 92c6891d1c only, as the body asks. #12530 and #12507 are below it.
Linux build (Ubuntu 24.04 amd64 container, GCC 13, Release, BUILD_TESTING=ON, head 92c6891d1c)
guest_formatting,guest_stdio_variadic,guest_scan_count: all 3 pass.- RED check: I put the parent's (
3eca2be067)Formatting.cppunder the head tests, andguest_formattingfails. The new checks catch the old behaviour.
What glibc stores, and what the helper does with it (I copied StoreDefaultNaN into a standalone program, same container)
| input | host %f / %lf / %Lf |
after StoreDefaultNaN |
|---|---|---|
-nan, -NaN, -nan(0x5) |
ffc00000 / fff8000000000000 / ffff:c000000000000000 |
7fc00000 / 7ff8000000000000 / 7fff:c000000000000000 |
nan, NAN, +nan, nan(), nan(1), nan(0x123) |
already the default NaN | unchanged |
-inf, -0, 0x1p3 |
sign kept | unchanged (the NaN check gates correctly) |
For %Lf, bytes 10..15 stay as they were. Only the 10 value bytes are written, matching what the host stores.
Mutants against the head tests
| mutant | result |
|---|---|
drop the L branch |
killed |
drop the l branch (falls to float) |
killed |
%Lf writes sizeof(long double) (16) instead of 10 |
survives |
fix only sign-bit NaNs (!isnan || !signbit) |
survives. Equivalent on glibc, which never yields a positive non-default NaN from text (payloads are dropped, see the table) |
!suppress → target != nullptr |
survives. Equivalent, since a null target already returns EOF |
The surviving 16-byte mutant would overwrite the 6 bytes after a guest's 10-byte long double. The extended buffer in the test is uninitialised and only bytes 0..9 are checked, so nothing catches it. This test-only change kills it (head passes, mutant and parent fail):
unsigned char extended[16];
+ std::memset(extended, 0x55, sizeof(extended));
Require(ScanListS(input, "%f", &single) == 1 && std::bit_cast<std::uint32_t>(single) == 0x7fc00000u);
@@
Require(mantissa == 0xc000000000000000ull && exponent == 0x7fff);
+ Require(std::all_of(extended + 10, extended + 16, [](unsigned char byte) { return byte == 0x55; }));
}(<algorithm> is already included.)
Other checks
memcpy(target, &value, 10)from a hostlong doubleis safe, because the libc target already static-asserts an x87 80-bitlong double(Math.cpp:140), and macOS builds of the libs force x86_64.check_conventions --base upstream/mainpasses, andgit merge-treeagainstupstream/mainf21ef7c is clean.pr_overlapshows file-level sharing only ondocs/dev/debt/libc.mdand with your #12397, and a pairwise merge-tree with #12397 is clean.
No defects in the fix. The one suggestion is the padding check above.
|
Addressed the helper’s 8-byte-host overflow and the padding-test suggestion in Linux Release: The caller writes host scanf output before this helper runs; the helper is not parsing an existing guest80 value on an8-byte host. The full library’s x87 requirement remains unchanged: this does not establish a full8-byte-long-double library/ARM64/macOS port, and no new Windows or title-library comparison is claimed. AI-assisted: yes. Current-main integration: normal fast-forward merge Evidence: immutable Linux qualification bundle. Its SHA-256 is |
|
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) |
|
@MarcZX10 Please re-review unchanged current head |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
At 6d16add5fb12bcc42bb50e987a07b9ab90ee4f4c, the %Lf NaN helper reads only the host value width, writes the ten-byte guest x87 NaN explicitly, and preserves trailing padding.
What I ran
Reviewed head 6d16add5fb12bcc42bb50e987a07b9ab90ee4f4c against base 157b02c4f1803e9d2cd757677b9a805961203fc4; the dependency stack preview was clean against current main f5b82ff45302cd6748a58868611a3dbe89211cdd. Environment: Linux x86_64, Python 3.11.2. No local C++ build or test was run.
git diff --check 157b02c4f1803e9d2cd757677b9a805961203fc4...6d16add5fb12bcc42bb50e987a07b9ab90ee4f4c
python3 tools/check_conventions.py --base 157b02c4f1803e9d2cd757677b9a805961203fc4 --head 6d16add5fb12bcc42bb50e987a07b9ab90ee4f4c --title 'fix(libc): store a scanned NaN as the default quiet NaN in sscanf_s' --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-12532-6d16add5fb12/pr-body.md
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 12532Results
| Check | Result |
|---|---|
| Diff and conventions | Passed |
| Dependency integration | #12507 → #12530 → #12532 all preview cleanly against current main; declared dependencies are included in the head |
| Hosted checks | Windows, Linux relinker, macOS relinker, ASan, register-context, and write-watch jobs passed; product Linux job was skipped |
| Focused test evidence | PR participant reports 3/3 passes for the three focused Linux groups and 5/5 Windows guest_formatting runs on the current head |
| Precheck helper availability | Its optional ABI and test-quality helper copies were absent from the isolated PR checkout; no public exports change in this PR |
Unedited local convention output
All conventions checks passed.
Negative control
| Assertion | Change to code under test | Outcome |
|---|---|---|
| Ten-byte guest NaN write preserves bytes 10–15 | PR participant reports changing production output to 16 bytes while retaining the test | Test failed as expected; the reported restored source passed |
| Helper avoids an 8-byte-host overread | PR participant reports an ASan+UBSan helper harness using GCC -mlong-double-64 |
The parent reproduced the overread; the corrected helper passed the reported control. I did not run this harness locally. |
Sources
- NaN normalization helper at the reviewed head bounds the host read, detects NaN, and writes the guest x87 representation for the ten-byte destination.
- Regression assertions at the reviewed head check float, double, x87 bytes, and untouched padding.
- Linux qualification and bounded-helper report; Windows test report on the current head.
Not verified
I did not reproduce the native title comparison, Windows or Linux tests, or sanitizer harness. The PR explicitly does not establish a full libc port for hosts whose long double is eight bytes; that broader platform behavior remains outside this change. Console behavior beyond the cited title measurements is not verified.
|
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 (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)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. |
|
Refreshed to Fresh Linux CPU-only qualification: 3/3 runtime tests pass, zero skips; actual module/fixture builds, production NID patching, conventions/whitespace and MinGW syntax pass. Public-header auditing remains red on main and this head for exactly the same five missing WKSecurityOrigin declarations; this is recorded, with no header-green claim. The inherited older-main Windows allocation compile failure is fixed by already merged #15855; the fixing source is preserved here. Its successful Windows build is upstream evidence, not this branch’s Windows qualification. Final merge preview against main Immutable source/log/JUnit/control evidence, SHA-256 AI-assisted: yes (Codex). |
|
Refreshed onto Windows, WinLibs GCC 15.2 UCRT, Release:
AI-assisted: yes. |
|
Linux check of exact head
AI-assisted: yes |
❌ 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.
agc_graphics_profile_draw: Timeout (also failed when re-run alone), assertion and diagnosticsUnstable on main (6, 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)Validation IDs already present on main (4 IDs in 805 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. |
The libc.prx that titles ship (PPSA05916, PPSA07923) stores every NaN a floating conversion reads as the positive default quiet NaN, whatever its sign or payload: -nan gives 0x7fc00000 for %f, 0x7ff8000000000000 for %lf and 0x7fff:c000000000000000 for %Lf. The host scanners keep the sign. The sscanf_s scanner now rewrites a stored NaN, ten bytes for %Lf. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6b9b061 to
e471b2e
Compare
|
Linux check of exact head
AI-assisted: yes |
|
Re-checked at |
Current-main integration — 2026-11-05. Exact head
5c3cff5ecd2e5128aab2ac390649dd9da4b0b330retains previous public3e78245d86df6a16aadd1d8b96b494a6744d0fa8and tested main6d173c1a19788ff69d43b3d0ca62833e312ef939as ancestors. The bounded native-host NaN read and explicit ten-byte guest x87 NaN helper remain byte-identical to the previous public head, including padding tests. Actual current #12530 parentaf0443d37194a7000804f22c1e71c82cae572dbdis retained by a history-only merge with an unchanged source tree. No full eight-byte-host long-double library port is claimed.Linux x86-64, GCC16.2.1, Ninja Release: actual production modules and focused fixture owners rebuilt; 3/3 runtime tests pass, zero skips:
guest_formatting,guest_stdio_variadic,guest_scan_count. Matching pinned main passes all25 selected baseline groups. Production NID patching, conventions/whitespace, and MinGW syntax forFormatting.cpppass.Public-header audit: failed on both pinned main and this head with the same five missing
WKSecurityOrigin*declarations (Create,CopyToString,CopyHost,CopyProtocol,GetPort), with no additional declaration errors. The failure is retained in the logs; no all-green header/full-suite claim is made. The concrete shared-main repair is #16010, exact head545e24b2ee2e549001421f7fc9f87ff801f3423c. Its independent qualification reports a green audit of 406 libraries/30,572 exports and two standalone units compiled by each of GCC and MinGW. A maintainer still needs to merge that repair and rerun the affected checks; this unmodified qualification tree is not relabelled header-green.The older-main Windows
DepthSurface::createSampleShadowallocation compile error is already corrected by merged #15855. This head contains that fix, and its DepthSurface.cpp/ImageMemory.hpp blobs match the fixing source. The fix’s hosted Windows build passed; that is attributed upstream evidence, not a Windows build/runtime result for this head. No duplicate AGC change was added. A final merge preview against newer main7ad9749ed0311cc91d69bc7c690d09c92b9d42d0is clean; qualification remains pinned to6d173c1a19788ff69d43b3d0ca62833e312ef939.Source/parent/provider/gitlink proofs, commands, raw logs, JUnit, hashes and known baseline failure. Historical platform, module-reference and performance reports below retain their original pins; no new Windows/macOS, SDK/physical-console, title/GPU or performance run.
Earlier pinned reports and the contribution template follow.
Refreshed against main
0263899e996f26b165aed37f6b5a616dd1d4c6bb; current head3e78245d86df6a16aadd1d8b96b494a6744d0fa8retains previous public head6d16add5fb12bcc42bb50e987a07b9ab90ee4f4cand 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 #12530, which is on #12507. Current head
6d16add5fb12bcc42bb50e987a07b9ab90ee4f4cincludes the helper correction and main integration; the original subject commit92c6891d1cis historical.What
sscanf_sandvsscanf_snow store every NaN a floating conversion reads as the positive default quiet NaN, as thelibc.prxthat titles ship does. The host scanners keep the sign of-nan, somainstored a negative NaN.main-nan,-NaN,-nan(0x5)%f0x7fc000000xffc00000%lf0x7ff80000000000000xfff8000000000000%Lf0x7fff:c0000000000000000xffff:c000000000000000nan,NAN,+nan,nan(1)andnan()already gave the positive default NaN.After the host has stored its native value, the scanner classifies it within that value’s width. A
%LfNaN is written as ten explicit guest x87 quiet-NaN bytes, leaving the trailing padding untouched; other widths usestd::numeric_limits<T>::quiet_NaN(). This also covers the stream forms (fscanf_s,scanf_s), because it runs after the store.This is the same rule #12514 measured for
swscanf. Plainsscanftakes a separate path (glibc on Linux,WindowsScanning.hppon Windows) and the titles behave the same there, but it isn't changed here. It is part of the narrow-scanner work planned after #9087.Tested
Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
BUILD_TESTING=ON, head92c6891d1c.sscanf_sof PPSA07923 and PPSA05916 by NID.nan,-nan,NAN,-NaN,nan(1),-nan(0x5),nan(),+nan) at all three widths: 0 differences (9 before).%5Lfof"-naNXf") to 0.guest_formattingchecks five NaN spellings at each width. With fix(libc): fail a 0x with no hex digit and keep errno for in-range floats in sscanf_s #12530'sFormatting.cpp, it fails.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.Current Linux qualification — 2026-10-29
Addressed the helper’s 8-byte-host overflow and the padding-test suggestion in
8f74e7ad1d7. After successful host scanning, NaN classification reads the native host value width (bounded by the requested value bytes). A NaN in the%Lfpath is emitted as ten explicit guest x87 quiet-NaN bytes; other widths and finite conversions are unchanged. The regression now checks that bytes10–15 remain untouched.Linux Release:
guest_formatting,guest_stdio_variadicandguest_scan_countpass three times (9 checks), then all9 pass after restoration. A production mutant writing16bytes instead of10 fails the new padding assertion; restored libc is byte-identical. An ASan+UBSan harness extracted from the actual helper reproduces the parent’s stack over-read with GCC-mlong-double-64; the correction passes both that helper-only ABI model and native x87 checks, including exact NaN bits, finite preservation and tail guards. Conventions and whitespace pass.The caller writes host scanf output before this helper runs; the helper is not parsing an existing guest80 value on an8-byte host. The full library’s x87 requirement remains unchanged: this does not establish a full8-byte-long-double library/ARM64/macOS port, and no new Windows or title-library comparison is claimed. AI-assisted: yes.
Current-main integration: normal fast-forward merge
6d16add5fb12bcc42bb50e987a07b9ab90ee4f4cnow includesmain157b02c4f1803e9d2cd757677b9a805961203fc4. The only conflict was<bit>versusScanQuadLength.hpp; both includes and every nonblank added test line from main and this stack are retained. The NaN helper is byte-identical to the qualified correction, and the actual #12530 ancestry remains intact. All three focused groups pass three repetitions on this integrated source (9 more passes), with conventions and whitespace clean.Evidence: immutable Linux qualification bundle. Its SHA-256 is
ca6b35926bcc6d3f21a9650517894c86b65594e2e2b653c5c32cd1c1de89c835. No fresh Windows runtime execution is claimed; older measurements retain their recorded source pins.AI-assisted: yes (Codex).
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)af0443d37194a7000804f22c1e71c82cae572dbd)🤖 Generated with Claude Code