Repository navigation
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.
|
|
Windows run of dea162e merged onto Built |
|
Linux run with a wider set of inputs. This is the only host where the change runs: the code is Head
Probe:
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Thanks for the probe, it found a real defect: an unsized |
|
Ready for the full run, whenever a collaborator has a moment for the manual dispatch: State:
Note for sequencing: the overlap comment lists #7386 and #9087 as touching |
|
Windows run of 9143eee merged onto Built On the Windows line in the summary above: as @wachi35676 noted, the wide-scan path is inside |
The 16-bit wide conversions decode their input as UTF-8 on Linux, but a %l[ scanset member was read byte by byte and threw for anything outside ASCII, so a scanset naming an accented or non-Latin character could not be used. Decode the scanset as UTF-8 and compare the decoded code points, which also makes a range such as a-e with multibyte endpoints a real code point range. A scanset that is not valid UTF-8 still throws. The debt entry no longer lists a scanset member outside ASCII among the throwing cases.
…he wide scanf The 16-bit wide conversions decoded UTF-8 code points and stored each as one char16_t, throwing for anything above 0xFFFF, so a guest scanf could not read an emoji or any other supplementary character. Encode the decoded code point as one or two units, count widths in characters and _s capacities in units, and treat a character that does not fit the destination as a constraint violation with an empty result, which is what the capacity path already did before it could overflow. The debt entry no longer lists a character outside the BMP among the throwing cases.
An unsized %lc field has no capacity to bound, and a character outside the BMP occupies two 16-bit units, so the scanner wrote one unit past the caller's object: a guest char16_t c filled by sscanf(s, "%lc", &c) received a second unit. The unsized field throws for such a character again, as it did before surrogate-pair support, while the _s forms keep writing both units because their capacity is checked in units. Reported with a probe by wachi35676 on the pull request.
9143eee to
5c6e9e6
Compare
|
Rebased onto Re-run after the rebase: On @ycbuzi's negative control: thanks, that is the right framing and the description already says it — this block is compiled out on Windows, so the Windows run shows the build and the absence of a regression, not that the change runs there. Linux is the host that exercises it, and @wachi35676's run is the one that covers execution. |
|
@Canzi18 Please remove the new scan tests requiring unsupported non-BMP unsized %lc/%2lc and positional forms to throw: the tests should not pin unimplemented behaviour. Retain the production safety guards, malformed-input validation, positive surrogate/scanset cases and explicit encoding limitations, then rerun the applicable formatting/scanning tests. |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 39a36bb and FreeBSD 11.4 lib/libc/stdio/vfscanf.c / vfwscanf.c:
- It still merges cleanly onto main, and main still throws for both cases, so the change is still relevant.
- Width-limited string fields in the non-
_sforms have the same problem the unsized%lcone had.wantedcounts characters (Formatting.cpp:196/205) andlimitisULONG_MAXoutside_s, but each non-BMP character appends 2 units. Reading the code,char16_t buf[3]; sscanf("😀😀", "%2ls", buf)writesd83d de00 d83d de00 0000, which is 5 units for a buffer sized width+1 (wachi35676's probe shows the same for%1ls: 3 units). On main this throws. FreeBSD'sconvert_wstring/convert_wcclstore onewchar_tper character, so width+1 is always enough there; with a 16-bitwchar_tI don't know what the console does. Until that's known, would it work to throw for a non-BMP character in a width-limited non-_s%ls/%l[field, as the%lcpath already does? The same applies to%2l[...]. - Code-point scansets and ranges like
à-êaren't what either reference does. FreeBSD 11.4vfscanf.ctests%l[through a 256-entry byte table (__sccl, thenccltab[wctob(wi)]), andvfwscanf.c'sincclis a plainwmemchrwith no ranges. Could the debt entry say that scanset members and ranges are compared as code points, unverified on the console? - Minor: a reversed range (
%l[ê-à], andz-aalready on main) matches nothing, while FreeBSD's__sccltreats it as the literal membersê,-,à. Also, a suppressed%*lcthrows on a non-BMP character even though nothing is written. Neither blocks the PR.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
|
@Canzi18 I agree with the width-limited non-_s safety finding. In addition to my existing test-contract ask, please guard non-BMP input in width-limited %ls/%l[ until a safe measured 16-bit contract is established: %2ls currently accumulates four UTF16 units and writes a fifth terminator into storage callers size as width+1. Keep safe positive/scanset tests and sentinel checks without pinning an unsupported throw. Please also state that code-point scanset members/ranges are unverified and differ from the cited FreeBSD paths. |
|
Linux re-run of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
I opened #11724, which lets CONTRIBUTING asks us to agree on an order. I suggest #9087, then this PR, then #11724. I've marked #11724 as If you'd rather go the other way, tell me and I'll swap the order. |
|
@Canzi18 measurements that bear on this PR's direction: titles' own Each byte goes to
So decoding non-ASCII scanset members and non-BMP input as UTF-8 would give titles results their own The full rules and a comparison table are on #11724, where I proposed making the wide branch follow them on both platforms, stacked on that PR. I'd suggest this PR wait for that discussion. AI-assisted: yes (Claude Code). |
|
Static review plus an independent Linux run (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja, Release, What I checked (static)
Focused test run
Test additions pin exactly the changed paths: non-ASCII scanset members ( Not an AGC/shader/VideoOut change — no paired Vulkan-validation run required. |
What
Two gaps in the guest wide
scanfon Linux, both listed in the technical debt:%l[...]read its scanset byte by byte, so a member outside ASCII threw instead of matching. The scanset is now decoded as UTF-8 and compared as code points, which also turns a range such asà-êinto a real code point range._scapacities count units, and a character that does not fit the destination leaves an empty result, as the capacity path already did.The remaining throwing cases are unchanged: input that is not UTF-8, positional arguments next to a wide conversion, and wide conversions in
fscanf. The debt entry no longer lists the two cases this fixes.Tested
-DBUILD_TESTING=ON):guest_formattingwith the new cases,guest_wide_formatting,guest_wide_stream_scanandguest_scan_countpass.core/libs/tests/Formatting.cpp:%l[é],%l[à-ê]with a multibyte range,%l[^é], a mixed ASCII/non-ASCII member,%lsof an emoji,%2lsstopping after one character, and%lsinto a 4-unit_sbuffer where one emoji fits and two do not.libcstill builds. This block is#ifndef _WIN32, so Windows CI does not exercise it; the Windows path hands%ls,%lcand%l[to the host CRT.tools/check_conventions.py --base origin/mainpasses.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)