Repository navigation
fix(libc): read characters outside the BMP as surrogate pairs in fgetwc - #10929
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Found one blocking stream-ordering regression in the pending-surrogate path. fgetwc stores the low surrogate after returning the high half, and the next fgetwc returns that queued unit before reading the underlying stream. If a caller then successfully calls ungetwc(X, stream), ungetwc_nid_postfix pushes X into the host stream, but the next fgetwc still returns the queued low surrogate first. That violates the pushback guarantee that the next wide-character read returns the successfully pushed-back character. Please add a regression case that reads the high half of a supplementary character, pushes back a BMP character, and verifies the order of the next reads; preserve the queued low half after the pushed-back character as appropriate.
|
The current hosted Conventions run also fails on the PR description: |
|
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.
|
|
Runtime confirmation of MarcZX10's
The pushed-back AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Tested
Corrected: an earlier version of this comment listed Test suite only; no title was run. AI-assisted: yes (Claude Code) |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the decoding and the new CheckSupplementaryWideCharacters cases look right (they fail on main). Two things block this:
StandardStreams.cpp:104: afterfgetwcreturns the high surrogate, the queued low half is returned by the nextfgetwcbefore anything the caller pushed back.ungetwc_nid_postfix(StandardStreams.cpp:130) only callsungetcon the host stream and ignoreslowSurrogate, sogetwc(0xd83d),ungetwc(L'X'), thengetwcreturns 0xde00 instead ofX(reproduced by @wachi35676, same point as @MarcZX10's review; C11 7.29.3.10). Fix it so the pushed-back character is read next and the low half still follows it, and add that sequence as a regression case next to line 302 ofGuestStandardStreams.cpp.- The PR body is empty:
What,Tested,Depends on,Fixes,Unblocksand the checklist andAI-assistedlines are unfilled, so Conventions fails. Please fill the existing template.
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the other open libc PRs #11260, #11262, #11266, #11632, #11634, #11278, #11270, #11283, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
fputws and wprintf write a UTF-16 surrogate pair as one four-byte UTF-8 sequence, but fgetwc decoded that sequence into a single code point above 0xFFFF, which is not a 16-bit guest wchar_t value, and fgetws stored it truncated: fputws(u"A\U0001F600\n") read back as A, U+F600, newline. fgetwc now returns the high surrogate and keeps the low one in the stream for the next read, so fgetws stores the pair, also when the buffer fills between the two halves. A successful seek discards it, as FreeBSD's fseeko resets the stream's mbstate. Sequences that decode to an overlong form or above U+10FFFF, and lead bytes F8 to FF, fail with EILSEQ instead of returning made-up characters; the three-byte surrogates that fputwc writes for a single unit are still read back as that unit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QegaHM5QeCynUQ2ZLWHE4b
16a21bb to
84aec8b
Compare
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — approved
The pending-surrogate pushback issue is fixed on the current head 84aec8b0ac56f402e4072c59495abf56211be41c. FileStream now tracks successful pushbacks while a low surrogate is queued; TakeLowSurrogate() defers that low half until the pushed-back byte has been consumed, and NoteWideRead() decrements the pending count. The regression covers getwc returning the high half, ungetwc(L'X'), then reading X, the low half, and the following newline. I also checked that reposition/reopen/reset paths clear the new state. This resolves my earlier functional finding.
The author documents the required template fields and tests; Linux and Windows focused runs are reported on the PR thread, and hosted Windows/conventions/policy checks pass. The current-main merge preview is clean; source/test paths have no later main-side edits, though the shared debt entry was updated and the preview integrates it cleanly. I did not rerun the stream tests locally.
|
Thanks for the fix. Both points of the earlier request are now done on your branch:
|
What
fputws and wprintf write a UTF-16 surrogate pair as one four-byte UTF-8 sequence, but fgetwc decoded that sequence into a single code point above 0xFFFF, which is not a 16-bit guest wchar_t value, and fgetws stored it truncated: fputws(u"A\U0001F600\n") read back as A, U+F600, newline.
fgetwc now returns the high surrogate and keeps the low one in the stream for the next read, so fgetws stores the pair, also when the buffer fills between the two halves. A successful seek discards it, as FreeBSD's fseeko resets the stream's mbstate. Sequences that decode to an overlong form or above U+10FFFF, and lead bytes F8 to FF, fail with EILSEQ instead of returning made-up characters; the three-byte surrogates that fputwc writes for a single unit are still read back as that unit.
Claude-Session: https://claude.ai/code/session_01QegaHM5QeCynUQ2ZLWHE4b
ungetwcafter a high surrogate: the pushed-back character is now read before the queued low half, which follows it.Tested
Linux and Windows 11 (WinLibs GCC 15.2.0) runs of
guest_standard_streamsby other testers on the PR thread; the newCheckSupplementaryWideCharacterscases, includinggetwc,ungetwc(L'X'),getwc, fail onmain. No title tested.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)