Repository navigation
fix(libc): retain short wide scanf character fields - #9863
oneandonlydean merged 16 commits into
Conversation
|
@orezraey: the required overlap report identifies one documentation-line conflict with #9087. Your narrow snprintf_s/sscanf_s runtime implementation is independent of this shared wide scanner correction; I did not copy or replace it. Proposed order is #9863 first, then preserve the short-field evidence/unsupported-capacity wording together with your updated constraint wording when resolving the combined scanner debt entry. If #9087 lands first, I can apply the converse resolution. Please confirm your preferred order; I am leaving the agreement explicitly pending rather than claiming the overlap is resolved. The base-conflicted #8306 dispatch change for %S/%C is also separate from the character-field loop. If #6044's test-directory reorganization lands first, retain these cases at its final GuestWideConversion/GuestWideStreamScan paths and registrations. No foreign branches were combined for the reported runtime tests. |
|
@UnhealthyKraken Agreed: #9863 lands first. I'll then merge it into #9087 and resolve the combined scanner debt entry, keeping your short-field evidence and unsupported-capacity wording together with my constraint wording. |
|
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.
|
|
Confirmed, thank you. I have updated the PR description with the agreed order and linked your confirmation: #9863 first, then #9087 merges it and preserves both sets of debt wording. This PR remains independent with Depends on: none. The current head is mergeable and hosted checks are now clean; the full product jobs remain skipped, with the focused Windows results recorded separately. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the implementation path and the added regressions against the contributor-reported bundled-library table in #9321. The code now assigns the consumed nonempty %c/%lc prefix without a terminator, continues to later directives and %n, and leaves suppressed fields unassigned. Tests cover both string and stream scanners, secure/unsecure entry points, prior assignments, exact canary bounds, lookahead and an explicitly unsupported undersized secure-capacity case. The debt text keeps the evidence attribution and remaining limits clear. The coordinated order with #9087 is documented by both authors, and Depends on: none is consistent with the confirmed independent order.
The author reports Windows focused builds/runs and 8/8 candidate suites (9/9 including loader) against identical executables, plus 1177 test registrations. Hosted conventions, relinker and registration checks pass; hosted Linux/Windows product jobs are skipped. I did not independently run the Windows binaries here.
One freshness item: current upstream main is a0900b9d; GitHub compare reports head 26db0f2a is two commits ahead and 26 behind, with merge base d6e7ab42. CONTRIBUTING asks branches to stay current. Please rebase, preserve the agreed #9087 wording/order, and rerun the two focused scanner suites plus conventions on the final combined head before merge. No blocking behavior defect found in the reviewed diff.
|
@MarcZX10 Refreshed onto main 3bbd232 in 36496b7 using a history-preserving merge rather than a force-pushed rebase. Rebuilt libc and both requested test executables. guest_wide_conversion and guest_wide_stream_scan pass (2/2) on the combined head. Current conventions report zero errors and the same pre-existing combined debt-entry length warning; whitespace and test registration pass (1177 executables, none unregistered). The four focused files are byte-for-byte unchanged from reviewed head 26db0f2. The PR description records this fresh validation separately from the original full-library/baseline comparison. The agreement with @orezraey is preserved: #9863 first, then #9087 keeps both debt wordings; Depends on remains none. No new Linux/macOS/console or full-repository run is claimed. |
|
The agreed #9863-before-#9087 order is now documented, and the author reports the focused |
|
@MarcZX10 Refreshed again: fetched main was already at 44ce348, beyond the 7c900bb snapshot in your comment. Merged that snapshot without rewriting published history; new head a97e5e4. The four focused files are byte-for-byte unchanged from 36496b7. Reconfigured and checked the libc and both scanner test build targets (Ninja: no work required), then reran guest_wide_conversion and guest_wide_stream_scan on this combined head: 2/2 pass. Conventions: zero errors, the same pre-existing 2980-character combined debt-entry warning. Whitespace and registration pass (1177 executables, none built and unregistered). CMake's unrelated object-path-length warnings remain. The PR description records this exact tested snapshot. The #9863-before-#9087 agreement and both debt wordings are preserved. No Linux/macOS/console or full-suite runtime run is claimed; hosted product-job status remains separate from these local tests. |
|
Linux run (the description lists Linux as not tested). Docker on Windows: Ubuntu 26.04, GCC 15, glibc 2.43, WSL2 kernel 6.6, Release,
No findings. AI-assisted: yes |
|
Thanks for the independent run and the Linux baseline comparison. I linked your exact-head/environment report under Tested and attributed it to you; I have not rerun Linux here. No additional code change is indicated by that report. |
|
The current head |
|
@MarcZX10 Merged current main
The focused cloud validation checks out exact |
|
Console data for the case the description leaves open: a short secure field whose destination capacity is below the requested width. This is evidence only; I haven't touched the branch. Method. I called Results. All three titles give identical results:
What the console does:
So the behaviour where this PR "throws after consuming the prefix" for a capacity below the requested width differs from the console. I didn't check whether a capacity-0 field consumes its input; |
|
@UnhealthyKraken The new evidence in issuecomment-6357861510 contradicts the throw at ScanningWide.cpp:431-432 and the capacity 1/2 throw assertions. The contributor reports native calls into three bundled libraries: input a, width 3 and capacity 1 or 2 succeeds with one character and %n=1; input abc and capacity 2 stores and consumes two characters, with %n=2. Capacity 0 returns zero without writing; its consumption remains unverified. Please reconcile the secure character path with that evidence, including capacity-limited consumption, update the regression assertions and debt wording, and rerun both scanner suites. Keep the evidence attribution: these are contributor-reported native library calls, not firmware-wide console proof. |
|
Hey @UnhealthyKraken, I put together a small follow-up for JJenkx's positive-capacity measurements above, covering the wide-destination part of the new review request. With The attached patch limits consumption before reading, only for a nonnull, positive-capacity wide destination ( Tested Windows 11 Pro x64 26300, WinLibs GCC 15.2.0 r7, Release with
Tests check output canaries, no terminator, The native bundled-library measurements are JJenkx's; this follow-up preserves your original implementation and history. These are our own Windows regression results; shared stream coverage does not establish bundled-library stream parity. Linux, macOS, full CTest, title execution and physical PS5 behavior were not tested. The agreed #9863-before-#9087 order is unchanged. No competing PR or source push; this is a patch handoff on your existing work. AI-assisted, independently reviewed before posting. AnyPS5-9863-wide-capacity-patch-and-tests-20261023.zip ZIP SHA-256: |
|
@oneandonlydean @JJenkx @cookie42073-cmd Published the positive-capacity wide-destination correction at Secure Own cloud Linux validation (Ubuntu24.04/GCC13.3, actual bundled project, one build job, serial tests): regressions before correction fail both scanner suites. Final-head candidate/control passes10/10 focused suites; restoring only the old scanner and rebuilding libc gives2fail/8pass with SHA256-verified unchanged scanner executables; restoring the correction gives10/10 again. Both scanner suites also pass five repetitions. Conventions0errors, existing3369-character debt warning; whitespace passes. No local builds/runtime tests. The supplied Windows results remain attributed to their contributor and integration, not claimed as my final-head Windows run. The description/debt now distinguish JJenkx's reported native calls to three bundled libraries from firmware-wide or physical-console proof. This correction intentionally covers positive wide destinations. The reported narrow ASCII undersized case still differs; generalized narrow byte-capacity rules need evidence alongside main's UTF-8 changes. Short zero-capacity fields also remain explicitly unsupported: the reported return/output are known, but consumption is not. Shared stream regression coverage does not establish native bundled-library stream parity. @JJenkx If additional measurements are available, the remaining questions are narrow |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed exact head 12446569274e168eb8e14d2bc86205b7e2d02492.
One measured secure-field case is still represented as an exception, although the linked native-library results report a return value:
ScanningWide.cpp:432-433throws when%3lchas a positive-length short input and capacity zero.GuestWideConversion.cpp:259-264codifies that as an expected throw.- JJenkx's attributed report in the linked native calls says that, for three bundled
libc.prxbuilds,swscanf_s(L"a", L"%3lc%n", w, 0, &n)returns0, leaves the destination untouched, leaveserrnoat zero, and does not reach%n. The report explicitly leaves input consumption unknown.
Please make the zero-capacity short-field return and destination behavior match that evidence, and add a regression that checks those observed results without asserting stream/input consumption. Keep consumption marked unverified until measured. The current explicit throw is a known behavior mismatch, even though the cursor position is unknown.
The exact-head report records strong Linux validation (10/10 focused suites, including five repeat runs); I did not rerun those tests locally. Current hosted Linux and Windows product jobs are skipped, while conventions, eligibility, registration, relinker matrix and other applicable checks pass. The PR is also 8 commits behind current main (e94545a5, merge base 938812df); the merge is clean, and those intervening main commits do not touch the four changed files. Please refresh to current main and rerun both focused scanner suites and conventions before merge, as required by the contribution rules.
|
@oneandonlydean Reconciled the two main conflicts and retained main's public-header declarations. Removed the undersized narrow Published Updated the PR description using the current template. AI-assisted: yes. No local builds/runtime tests, games, proprietary modules or console execution; the full repository suite was not run. |
|
@UnhealthyKraken the test and declaration changes answer those parts of my ask. The merged scanner debt bullet still says “A short |
|
Merged current upstream main Validation: actual focused Release targets built in hosted Linux and Windows; both wide conversion/stream scanner suites passed on both, including repetitions. Conventions, whitespace and public-header checks pass (the scanner PR retains its existing debt-length warning). No selected cases were skipped. Latest upstream CI has no failing or pending applicable checks, and GitHub reports the PR mergeable. No local builds or native tests were run. |
|
@UnhealthyKraken a merge-order note from #12420 ( I'd suggest this PR land first. I'll rebase #12420 after it, so you have nothing to resolve. If you'd rather it go the other way, say so. AI-assisted: yes (Claude Code). |
|
@UnhealthyKraken a merge-order question: a maintainer asked the authors of #12514 ( Proposed order: this PR lands first, and #12514 rebases afterwards (#12514 will declare it in Depends on). Your PR needs no changes. Could you confirm that order works for you, or say if you'd rather #12514 went first? AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The zero-capacity short-field finding from my earlier review is resolved: the tests now pin the measured assignment count/output/errno behavior, the stale debt sentence is removed, and the author reports focused Linux and Windows runs on the current feature head. I found no new implementation defect in that fix.
Two readiness items remain. First, the #12420 author proposed #9863 first, and the #12514 author independently proposed the same order for the debt-file overlap. Please confirm that order so the follow-up authors can record their dependencies. Second, current main has advanced since this branch’s last merge; docs/dev/debt/libc.md changed on both sides, even though the merge preview is currently clean. Please reconcile the debt entry against current main and rerun the documented checks on the integrated head.
The focused scanner tests and policy checks are reported green; I did not run them locally. This request is for the unconfirmed overlap order and current-main integration, not the resolved zero-capacity behavior.
github-actions
Bot
added
needs rebase
docs/dev/debt/libc.md: keep this branch's swscanf_s changes and take main's new _WStoul sentence (a bad base returns 0, as Dinkumware's xstoul.c does).
# Conflicts: # core/libs/tests/GuestWideStreamScan.cpp
Take main's set_constraint_handler_s and wcsncat_s entries and boykopovar#9863's short-field wording, keep this branch's sscanf_s constraint wording, and use SonyRsizeMax from SonyCrt.hpp instead of a local copy of the same limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
Keep a nonempty character field when wide scanf reaches EOF before its requested width. Input
awith%3lcreturns one assignment, stores onlyawithout a terminator, and continues to%nor a following directive. Suppression consumes the prefix without adding an assignment; an empty field remains an input failure. Positive-capacity secure wide destinations consume at mostmin(width, capacity). For positive-length input, a zero-capacity wide destination adds no assignment, leaves output/errno unchanged and stops before the following%n.Short-field behavior follows pinned FreeBSD11.4 convert_char/convert_wchar, supplemented by hypermoist's report and JJenkx's native calls to three bundled libraries. These are attributed contributor measurements, not my console measurements or firmware-wide proof. Zero-capacity consumption/empty-input precedence, narrow byte-capacity rules and native stream parity remain unverified in debt. The reported undersized narrow ASCII case remains a known gap; no test pins its unsupported exception behavior.
The review refresh retains main's public declarations, UTF-8 and
%S/%Cmatching-failure behavior and one combined scanner debt bullet. Blazing Cook's positive-capacity contribution retains its author identity/history. The agreed order remains #9863 before #9087; names, NIDs and implemented ABI do not change.Tested
Review revision
5f290cecde56468bb507d4c72b9de50c03eeab87, based on main47f9cee2cd77by history-preserving merge.Hosted Linux actual project configure/focused build passed with matching source-built libraries: both scanner suites pass once plus five repetitions each. Conventions (0 errors; existing combined debt-length warning), whitespace, public-header audit and standalone libc/kernel header compilation pass. Linux job. Windows final-head actual project configure/affected-target build passed on hosted WinLibs GCC 15.2 Release with matching runtime libraries: 2/2 focused tests, followed by two repetitions each. Windows job. Git-only mergeability against later fetched main
504ea3b53b23is clean; that later combined tree was not built.Original Windows baseline/candidate and later independent contributor results remain recorded in the earlier review report and zero-capacity correction; those are historical heads, not additional final-head platform runs.
No local builds/runtime tests, games, proprietary modules or physical-console execution were performed for this revision. The full repository suite was not run.
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)