Repository navigation
fix(libc): preserve errno on string scan input exhaustion - #13012
Conversation
rooklz
left a comment
There was a problem hiding this comment.
Host-only review of 8a7cd7d. The fix looks right. I'm leaving this as a comment rather than an approval until the windows lane finishes.
Reproduction. I wrote standalone C probes that use your row matrix: "", " ", " " × %s %d %f %*s × saved errno 0/22/97, plus the 1e99999 row.
- glibc 2.36 (docker gcc:14): raw
vsscanffails 24/36 rows. EveryEOFclears errno to 0. With your guard it fails 0/36, and the range row still ends with errno 34. - macOS libc (arm64 and x86_64): raw
vsscanfalready passes 36/36. That fits limiting the change to the__GLIBC__branch.
Guard mutants on glibc. result == EOF only and errno == 0 only both survive the new test. The no-op guard is caught (24 rows). I couldn't find an input where the two one-sided mutants behave differently from your guard: match failure returns 0, partial-then-EOF returns 1, literal-only empty input returns -1 with errno kept, %n on empty input returns 0, and invalid %ls under C.UTF-8 returns 0 with errno 84. So they look equivalent in practice. Nothing to change here.
Possible follow-up (not for this PR). glibc fscanf shows the same clearing. From errno 97, a whitespace-only fmemopen stream and an empty tmpfile() both give -1 with errno 0. HostVfscanf (Formatting.cpp:133) is still unguarded and feeds the guest stream scan exports (Formatting.cpp:479, :808). Your bundled-module probe could probably measure the stream path the same way, before anyone extends the guard there.
Windows. On Windows the new test goes through ScanWindows → UCRT vsscanf("%lld%n"). Under Wine's UCRT, whitespace-only %d/%f returns 0 rather than EOF, and %f overflow leaves errno at 0. Wine also leaves errno at 0 for the existing %d overflow row (test line 174 expects 34), and that row passes on the real windows lane. So I don't trust Wine here. The windows job on this head is the real check, and it's still pending.
Conventions and intake checks pass. No code comments, and the change is generic.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The glibc-only wrapper restores the prior errno only when scanning returns EOF with host errno left at zero; regression cases cover both exported entry points and verify that nonzero conversion errors remain intact.
What I ran
Reviewed head 8a7cd7d35a333f4500d6bdf32b07ffaa33d3a407 against current upstream main 52f212ec053f917455f2f87674ddb708dcc545d8; the merge preview is clean. The PR's base currently points to fc9240dbc5dc89a6b17a739703e00d26318c7716. No local build or tests were run.
git merge-tree --write-tree refs/remotes/upstream/main refs/review/pr-13012-head
git diff --check refs/remotes/upstream/main...refs/review/pr-13012-head
python3 tools/check_conventions.py --base upstream/main --head refs/review/pr-13012-headResults
| Check | Result |
|---|---|
Merge preview on current main |
Clean |
| Diff check | Passed (no output) |
| Repository conventions | Passed |
| Focused Linux build and tests | Author reports a Release libc build and 3/3 formatting suites passing on this exact head; I did not reproduce them locally |
| Negative control | Author reports the new fixture fails against unchanged main and the updated implementation matches all 420 rows from the bundled-module probe; not independently reproduced |
Not verified
The hosted Windows, relinker, ASan, macOS register-context and write-watch jobs were pending when I reviewed. The Linux product job was skipped. The author reports no Windows/macOS/title/console validation.
rooklz
left a comment
There was a problem hiding this comment.
Follow-up to my earlier COMMENT review. The head is unchanged at 8a7cd7d. Host-only; I didn't run anything on the console.
- The
windowsandrelinker (windows-latest, g++)lanes now pass. Under Wine the UCRT results couldn't be trusted, so this lane was what I was waiting on, and it decides it. - Checked again against current main 82e9900:
git merge-treeis clean,git diff --checkis clean, andcheck_conventionspasses. Main hasn't touched either changed file since the PR's base. pr_overlapover 1443 PRs: the only ones sharing files are #5021, #7386, #10324, #11332 and #11726, and all five already conflict with main.- The earlier evidence still holds. With the guard, glibc 2.36 goes from 24/36 failing rows to 0/36, ERANGE is kept, and the macOS libc already passes 36/36.
The stream-path sibling (HostVfscanf, which feeds the guest fscanf/vfscanf) is still an unmeasured follow-up lead. It doesn't block this PR.
AI-assisted: yes
|
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.
|
|
Measured the file-stream follow-up suggested in review and kept it separate as #13025. Both bundled libc references agree on all420 fscanf/vfscanf rows; main differs64 solely on EOF errno retention, and that follow-up matches420/420. Its four focused Linux suites pass with no skips, and the new stream regression fails against unchanged main. The authored probe, synthetic rows and exact hashes are attached there. No stream behavior is added to this PR. |
|
Checked the glibc-only guard in |
What
On glibc,
sscanf("", "%s", buffer)returnsEOFand clears an incomingerrnoof 97 to zero. Thelibc.prxmodules bundled with PPSA03766 and PPSA07923 preserve 97. The same mismatch affectsvsscanfand empty/whitespace input for other directives.Preserve the incoming value when the glibc scanner returns
EOFwitherrno == 0. A nonzero error reported by the host is retained. The change is limited to the glibc call inHostVsscanf; parsing, assignments and non-glibc calls retain their existing paths. The regression checks both exported entry points, untouched destinations, suppressed conversions, several incoming values and anERANGEconversion.Tested
Linux x86-64 (7.2.9-arch1-1), actual independent Release
libcbuild at8a7cd7d35a333f4500d6bdf32b07ffaa33d3a407.guest_formatting,guest_wide_formatting,guest_safe_errors: 3/3 pass, no skips. The new formatting fixture linked against unchanged main951f374c871fails withFormatting check failed(CTest exit 8).errno. The built change matches all 420 rows, fixing 64 with zero regressions. Range errors remain intact. Probe processes reject loaded AGC/VideoOut/Vulkan libraries.c40cef843681418b4d66df2939a92ba8ba6ba5d9. Overlap scan reports no positive source/test/export overlaps. Five same-file peers cannot merge into that base; their shared hunks concern separate stream/wide/formatting behavior rather than this EOF error-state guard.Probe source, synthetic rows, build/test and negative-control evidence, SHA-256
4abe9d7857c4d7795889dc7495e1ae5c9c8f28223b3ada53e210124e69c8423f(download verified). Attachment contains no SDK binaries or game assets.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)