Repository navigation
fix(libc): retain short stream character scan fields - #13057
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The narrow stream %c path now treats a nonempty short field as a conversion, preserves suppressed-input counting, and leaves the next field available after exact-width reads.
What I ran
- Reviewed exact head
63af66d6c5e20c9d3755bb5d84e6970077a39724against currentmaine7d8e353362935a1f00e893d901b1debc671f5ce; merge preview was clean at99ffa586aeebd06a8c37d85c62859a47fe3586c8. - Environment: Linux 6.1.0-54-amd64, x86-64, Python 3.11.2. No compiler was used.
- The author’s test baseline is
f0c8f26eec6edd346338e7c2d7f060ece52e8f50; I checked thatmainhas no changes to either touched file since that baseline.
git merge-tree --write-tree upstream/main refs/review/pr-13057-current
git diff --check upstream/main...refs/review/pr-13057-current
python3 tools/check_conventions.py --base upstream/main --head refs/review/pr-13057-currentResults
| Check | Result |
|---|---|
| Merge preview | Clean; tree 99ffa586aeebd06a8c37d85c62859a47fe3586c8 |
git diff --check |
Exit 0; no output |
| Conventions | Passed |
| Hosted CI | Eligibility, conventions, intake, and report pass; labels and platform jobs queued; Linux product job skipped |
| Author-reported focused tests | Four libc test targets pass 4/4 each on Linux; the author reports the new short-field assertion fails on unchanged main and passes with the patch. I did not run these build-dependent tests locally. |
Command output
99ffa586aeebd06a8c37d85c62859a47fe3586c8
All conventions checks passed.
Negative control
| Assertion | Change to code under test | Result |
|---|---|---|
A one-byte stream read by %2c%n reports the converted byte and updates %n |
Author tested unchanged main scanner logic |
Author reports the new assertion failed on main; this is not an independently run result |
Sources
Not verified
- I did not build or run
FileStreamslocally. Windows and macOS runtime behavior and console behavior were not independently tested in this session.
|
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 Corrected a concurrency regression I found in the initial implementation: its separate peek/consume operations let another thread obtain the file lock midway through a character field. Unchanged main's host scan keeps that field locked. Final head The new observable try-lock/position regression fails on the initial This changes the approved source and needs re-review; it is not an optional test-only update. AI-assisted: yes. Hosted Windows follow-up on this exact head: the Windows job passes its changed host-only test-executable linking step. Its CTest execution step is skipped by workflow policy, so this supplies compile/link coverage and does not establish Windows runtime behavior. The four executed focused suites above remain Linux results. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The updated scanner holds the recursive stream lock across the whole field, addressing the interleaving window; the added regression checks lock availability during and after scanning, and current-main checks pass.
What I ran
Reviewed PR head ff25c7aba5f6a53215385cae3627b00556c4dd99, recorded PR base from the current PR metadata, against current main 91a69952c10099966378a6a9006b8ef616ae22da. The merge preview is clean (tree 342220ad34b27c990a1cf1529c4ee175a606b065). Environment: Linux x86_64, Git 2.39.5, Python 3.11.2; no compiler or project build was run.
git diff --check upstream/main...ff25c7aba5f6a53215385cae3627b00556c4dd99
python3 tools/check_conventions.py --base upstream/main --head ff25c7aba5f6a53215385cae3627b00556c4dd99
git merge-tree --write-tree upstream/main ff25c7aba5f6a53215385cae3627b00556c4dd99Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; tree 342220ad34b27c990a1cf1529c4ee175a606b065. |
git diff --check |
Passed; no output. |
| Conventions | Passed. |
| Focused Linux suites | Author reports all four suites pass on this head and 2,880 reference rows are unchanged; 141 rows are corrected with no field regressions. |
| New locking regression | The author reports the try-lock/position regression fails on the initial unlocked implementation; on unchanged main, locking passes while the earlier short-field assertion fails. |
| Hosted checks | Linux and Windows relinker jobs and Linux Debug/Release register-context checks passed; Windows passed. macOS relinker/register-context and write-watch jobs were still in progress at review time; product Linux job was skipped. |
Unedited local command output
$ git diff --check upstream/main...ff25c7aba5f6a53215385cae3627b00556c4dd99
(no output; exit 0)
$ python3 tools/check_conventions.py --base upstream/main --head ff25c7aba5f6a53215385cae3627b00556c4dd99
All conventions checks passed.
$ git merge-tree --write-tree upstream/main ff25c7aba5f6a53215385cae3627b00556c4dd99
342220ad34b27c990a1cf1529c4ee175a606b065
Negative control
The author reports the new lock/position test fails on the initial implementation and that the prior short-field behavior remains failing on unchanged main; I did not independently mutate the code or rerun these controls.
Findings
Blocking
None found in the reviewed head.
Non-blocking
The Linux test results are author-reported. Windows hosted CI linked the changed test executable, but its CTest execution step was skipped; Windows runtime and native-title behavior remain unverified.
Sources
Not verified
I did not build or run tests locally, run Windows/macOS CTest, or test SDK-thread/native-title behavior. Some macOS hosted jobs were still in progress at review time.
To get this to a good state
No further change is requested for this head. I will recheck the same commands and relevant tests if the PR head changes.
Re-checked — new head, base SHA clarifiedCorrection to my review: the PR’s recorded base 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) |
|
Checked the |
What
For a temporary stream containing a single byte,
%2c%nin the bundled libc returns1, stores that byte, and writes1 to%n. The current secure stream scanner writes the byte but reports EOF and leaves%nuntouched. Read narrow character fields directly from StreamSource so any nonempty short field counts as a conversion, suppressed fields advance the count without an assignment, and exact-width fields leave the following input available. Preserve capacity validation, UTF16 paths and all string paths.Scope avoids the contiguous-string implementation in #9087, whose bounded string path already handles part of this case. No dependency on that PR; its stream fallback is unchanged. The ordinary host-backed fscanf/vfscanf short-field gap is still outside this change.
Hold the stream's recursive file lock throughout secure scanning so another reader cannot interleave between the character-field peek and consume operations. The lock is released on return and exception paths.
Tested
Linux x86_64 Release; final head
ff25c7aba5f6a53215385cae3627b00556c4dd99, original basef0c8f26eec6edd346338e7c2d7f060ece52e8f50. All four focused suites (guest_formatting, guest_wide_formatting, guest_safe_errors, file_streams) execute and pass with zero skips. The final header-only placement adjustment leaves both the qualified production library and regression-test executable byte-identical.The new concurrency regression runs both fscanf_s and vfscanf_s over a 1MiB character field. A second thread uses ftrylockfile and ftell to detect a lock becoming available before the field ends; it also checks lock release after return, complete output,
%nand destination guards. It fails at the intended lock assertion with initial PR head63af66d6c5e. With unchanged main it passes the locking check and then fails the existing short-field assertion. Both assertions pass with this head. This is a host/HLE regression check; bundled SDK multithreaded execution is not claimed.The original two bundled libc modules agree on all 2,880 authored single-thread rows across sscanf/sscanf_s/fscanf/fscanf_s. Secure stream differences remain 165/720 →24/720, correcting 141 complete rows. After the locking correction, all2,880 outputs remain byte-for-byte identical to the previously qualified PR outputs. The other three API matrices are unchanged; the24 remaining secure stream rows concern literal-prefix exhaustion. No title speed, graphics, native game, GPU, Windows/macOS local or console validation claimed.
Conventions pass. Merge preview is clean against pinned current main
f6fddd60d826831f70f7c62e25423610ee742bcc; Formatting.cpp, FileStreams.cpp, FileStream.hpp and RuntimeSupport.cpp are unchanged on main since the tested base. The1449-PR overlap scan found one include-placement collision with #13049; moving the includes resolves the current-main pairwise merge without changing either tested binary. The9 already-conflicted shared-file peers were manually reviewed as before.Locking correction, exact pins, controls, full focused logs/JUnit and overlap resolution, SHA256
f1e7c08f61d6d91ce27a3c3a3334345750f0b1adf72b7f4b67311af00d11d39b. Original two-reference authored probe and synthetic rows, SHA2560bef305e812549fd8408a9cb41123548c9f3ae5be70f73cb7b4c5eb60b0a421a. No SDK/game binaries or captured game data included.Independent Windows validation reported by bunbeans2-arch for exact topic
ff25c7aba5f6a53215385cae3627b00556c4dd99merged into mainf7f03daf52: Windows 11 Pro 22631, i9-9900K, MSYS2 UCRT64 GCC 16.1, Release.file_streamspasses 5/5; keeping the new test while restoringFormatting.cppto main fails 5/5 at the assertion that a short character scan retains the file lock through the field. Plain main with its own source/test passes 3/3. This is the contributor's run, not a new local Windows execution.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)