fix: don't corrupt multi-byte runes split across a stream buffer refill - #600
Open
r0h1tb wants to merge 1 commit into
Open
fix: don't corrupt multi-byte runes split across a stream buffer refill#600r0h1tb wants to merge 1 commit into
r0h1tb wants to merge 1 commit into
Conversation
The multi-byte branch of the streaming string decoder bounded its utf8.FullRune check by len(s.buf) — the allocated capacity — instead of s.length, the number of bytes actually read. Everything between s.length and len(s.buf) is nul padding. When a rune straddles a read boundary, FullRune therefore saw a truncated sequence followed by nul and reported it complete, so the refill below was skipped and DecodeRune returned RuneError. Each byte of the sequence was then replaced with U+FFFD, leaving the decoded string 6 bytes longer than the input for a 3-byte rune. Decoding an ~8KB JSON body of Japanese text over an http.Request body reproduced this consistently, with the first U+FFFD landing at the same offset every time. Bounding the check by s.length lets the split sequence fall through to s.read() and be completed by the next chunk. Adds a hermetic regression test that feeds the decoder through a reader capped at 1024 bytes per Read, covering 2-, 3- and 4-byte runes across a range of lengths so the split lands at every offset within a rune. Fixes goccy#574
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #574
Root cause
internal/decoder/string.go, the multi-byte branch of the streaming string decoder:len(s.buf)is the allocated capacity, not the number of bytes actually read — that'ss.length. Everything between the two is nul padding.So when a rune straddles a read boundary,
FullRuneis handed a truncated sequence followed by0x00, judges it complete-but-invalid, and the refill immediately below is skipped.DecodeRunethen returnsRuneErrorand each byte of the sequence is replaced with U+FFFD — leaving the decoded string 6 bytes longer than the input for a 3-byte rune.Bounding the check by
s.lengthlets the split sequence fall through tos.read()and be completed by the next chunk.Reproduction
Decoding ~8KB of Japanese text through an
http.Requestbody reproduces it consistently. Sweeping the payload length, corruption begins at 321 repetitions of"日本語更新テスト "(JSON length 8060) and the first U+FFFD lands at output byte 8006 every time, withlen(got) == len(want) + 6:The +6 is the fingerprint: one 3-byte sequence becoming three 3-byte replacement characters.
Tests
TestDecodeStreamMultiByteAcrossBufferBoundaryfeeds the decoder through a reader capped at 1024 bytes perRead, which puts the split at a buffer boundary without needing a socket — the test stays hermetic. It covers 2-, 3- and 4-byte runes across lengths 300–340 so the split lands at every offset within a rune.All three widths fail on the current default branch:
With the change,
go test ./...is green across all packages, matching the pre-change baseline. Separately I swept the original HTTP-body reproduction over 1–1200 repetitions: 7 failures before (test stops early), none after.gofmtclean; thego vetfindings ininternal/encoder/compiler.go,internal/encoder/string.goandinternal/decoder/type.goare pre-existing and untouched here.Possibly related
#572 (SIGSEGV in
appendNormalizedHTMLStringafter a stream decode) and #571 (index out of rangeon Windows) both involve decoding from a response body, and a string whose bytes were rewritten in place is a plausible route to both. I could not reproduce either directly — #572 has no minimal case and #571 is Windows-only — so this is a lead rather than a claim, and I haven't marked them fixed.