Repository navigation
fix(libc): reject a wcsncat_s copy only when its written and read ranges overlap - #9290
Conversation
|
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.
|
|
Linux data point for
AI-assisted (Claude Code). |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main a6646f8 and the console-module reading of wcsncat_s on #8269:
- The new condition is exactly the write range
[dest+used, dest+used+length)against the read range[src, src+length). I worked both new rows through it by hand, and a zero-length copy still never rejects, as on main. The head merges cleanly with main. - Could you add a debt line in
docs/dev/debt/libc.md? It would say that thewcsncat_soverlap rule follows a static reading oflibc.prx(not run on a console) and thatwcscat_sinherits it without its own code being read. The PR doesn't touch the debt file, and the existing_sentry only covers the size limits. - Main's narrow
strncat_suses a wider rule ([dest, dest+used+length+1)against[src, src+length+1), from a998987), andGuestStrings.cpp:137pinsstrncat_s(catOverlap + 3, 10, catOverlap, 3) == 22, where the written bytes [6,9) and the read bytes [0,3) don't meet. With this PR, the wide equivalent of that call copies. That's fine if the console really differs, but it may be worth noting in the debt file, or opening an issue so someone reads the narrow function's console code too.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
…ges overlap Co-Authored-By: Claude <noreply@anthropic.com>
The debt entry says the rule follows a static reading of the console's wcsncat_s in libc.prx, that wcscat_s inherits it unread, and that the narrow strncat_s keeps a wider rule, as asked in review. Co-Authored-By: Claude <noreply@anthropic.com>
edd4ae2 to
e53f782
Compare
|
@harisibrar1 Thanks for checking it against the console reading. Added in |
wcsncat_s and wcscat_s keep the overlap rule boykopovar#9290 read from the console's wcsncat_s, which leaves the terminators out of both ranges; strcat_s and strncat_s keep the measured rule that counts them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
wcsncat_srefuses a copy as overlapping when the source lies withinlength + 1units of the write position (Strings.cpp:852-858onmain), so it also refuses copies whose ranges don't meet. @oneandonlydean read the console's ownwcsncat_sinlibc.prx(the module a retail title ships; read, not run on a console): it rejects a copy only when the bytes it writes,[dest + len1, dest + len1 + len2), intersect the bytes it reads,[src, src + len2). For"abc"withsrc = dest + 1andcount = 2it writes indices 3-4 and reads 1-2, so the console copies, returns 0 and leaves"abcbc", whilemainreturnsEINVAL(22) and clearsdest[0]. This takes the console's rule. He asked for it as a separate PR on #8269 (comment).ERANGE(34) are unchanged; the same reading found they match.wcscat_scallswcsncat_swith an unbounded count, so it gets the same rule; its own console code was not read.wmemcpy_s(Strings.cpp:776) andwcsncpy_s(:817) have similar distance checks; they are left as they are, since nobody has read them on the console.docs/dev/debt/libc.mdgets an entry: the rule follows a static reading of the console'swcsncat_s(not run on a console),wcscat_sinherits it unread, andmain's narrowstrncat_skeeps a wider rule ([dest, dest + used + length + 1)against[src, src + length + 1), from a998987), so it refuses some copies whose wide equivalent now proceeds.Tested
Windows 11 26200, WinLibs GCC 15.2.0 r7, Release,
BUILD_TESTING=ON. Rebased ontomain30ecc1b7b3to add the debt entry (the only conflict was that entry's neighbour indocs/dev/debt/libc.md; the code and test are unchanged): with this change'sStrings.cppand test applied tomain9d6adeb550,guest_wide_stringspasses 3 of 3. The runs below are frommain4ec064d29.guest_wide_stringsgains @oneandonlydean's two cases: the non-overlapping copy above (returns 0, leaves"abcbc"), andu"abc\0xyz"withsrc = dest + 4andcount = 3, where the read range starts inside the write range (returns 22 withdest[0]cleared).main'sStrings.cppand the new test, the test aborts (exit0xc0000409). With the first new row removed it passes, so the non-overlapping copy is the rowmainfails.python tools/check_conventions.py --base origin/mainwith this title and body passes.maindf2569d2c5:guest_wide_stringsexits 0 with the change; withmain'sStrings.cppand the PR's tests it aborts (SIGABRT, exit 134)._sfunctions' overlap rules.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)🤖 Generated with Claude Code