Fix/utf8 validation prototype - #3
Merged
Merged
Conversation
Signed-off-by: Frédéric BIDON <fredbi@yahoo.com>
Signed-off-by: Frédéric BIDON <fredbi@yahoo.com>
Neither lexer validated UTF-8: the string scanners stop only on '"', '\' and control chars, so every byte >= 0x80 was waved through and aliased straight into the token value. Ten i_string_* JSONTestSuite documents carrying ill-formed UTF-8 were silently ACCEPTED, and the corruption then became invisible rather than absent (Go yields U+FFFD when such a value is iterated as a string). RFC 8259 §8.1 requires JSON text to be UTF-8. Supersedes the prototype in fb066a1. Plan: .claude/plans/utf8-validation.md. API --- One knob, WithUTF8Policy: UTF8Strict (default) invalid UTF-8 -> ErrInvalidUTF8, a broken \u surrogate pair -> ErrSurrogateEscape UTF8Replace substitute U+FFFD: one per invalid byte, one per broken escape UTF8Passthrough no raw-byte validation (unsafe; the pre-fix behavior) The prototype's fused/naive modes are gone: those were implementation strategies, not policies, and are now chosen internally from the CPU. The same policy governs \u escapes that do not denote a scalar value, which removes a live L/VL divergence: L laundered a broken pair into a silent U+FFFD (utf16.DecodeRune returns RuneError, and utf8.ValidRune(U+FFFD) is true) while VL errored. L also used to swallow the second escape, so "\uD800\uD800" yielded one replacement; it now yields two, matching token.Unescape. Performance ----------- Detection is fused into the scan already being performed: every SWAR word and every AVX2 block is OR-accumulated, so "did this value hold a byte >= 0x80" is answered with no second read and no call, and only values that actually do reach the validator. The AVX2 string-stop kernel gained a second result for this (VPMOVMSKB of the block already in the register, masked below the stop lane) -- without it every string past guessLong was force-validated, which was most of the prototype's cost. Validation itself is a port of the simdutf/Keiser-Lemire lookup4 algorithm to avo AVX2 (json/internal/utf8x), 32 B/iter, 5-12x the stdlib once engaged. vs. no validation, benchstat 10x300ms: the four ASCII-dominated corpus workloads are statistically indistinguishable; twitter_status (30% non-ASCII by string bytes) costs 6.3% (L) / 1.7% (VL). Geomean -0.36%. Zero-copy is preserved: validation only reads, so no policy adds a copy to a well-formed document. Only an offending value is rewritten under UTF8Replace. Also ---- - A leading UTF-8 BOM is now consumed instead of rejected (RFC 8259 §8.1 allows ignoring one on input). It is checked once on the buffer's opening bytes, so no scan loop is touched. It is NOT re-emitted by VL: the mark is consumed before any token exists, so it belongs to no token's leading space -- the one exception to VL's byte-exact round-trip on valid input, and the direction §8.1 asks for. A UTF-16 BOM now reports ErrNotUTF8 instead of a baffling "invalid JSON token". - VL's mangling contract is spelled out: U+FFFD is three bytes replacing one, so a rewritten value's length is not its source width and an index into it cannot be mapped back to a source offset. Positions (Offset/Line/Column/LeadingSpace) stay exact under every policy -- they come from the scan cursor, never from the value. - Shared primitives live in json/internal/utf8x, the one location reachable from both the lexers and the writers, which have the mirror-image bug. Tests ----- - Every policy x every one of the eight string paths (whole/stream x clean/ escaped x L/VL), plus keys, long values and WithoutAVX2. - Conformance: the ten cases flip to reject; added the VL/reader mode (VL's streaming string scanners had no conformance coverage) and an L/replace mode that pins replacement as relaxing nothing but ill-formed UTF-8. - The implementation-defined (i_) verdict table is now snapshotted in testdata/conformance_i_behavior.golden. Nothing asserted it before, which is exactly how this bug survived from the day the zero-copy fast path landed. - FuzzLexer gained the invariant that an emitted value is always utf8.Valid, across four lanes x two policies. - The AVX2 kernel is checked against the stdlib exhaustively over all 65536 byte pairs and over every lead byte C0-FF x all 2nd x all 3rd bytes, at several alignments, plus seam straddles, every length, and ~325M fuzzed validations. Mutation-tested: three single-bit table corruptions are each caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Two statements in the default-lexer docs were written while BOM trimming was still deferred and never updated when it landed: - "A leading UTF-8 BOM is currently rejected as an invalid token rather than skipped" (README.md and doc.go) — it is now consumed before the first token. Both places now also say what follows from that: the mark belongs to no token's leading space, so VL does not reproduce it, which RFC 8259 §8.1 asks for anyway. - The measured cost still quoted round 1's ~10% on twitter_status. With the AVX2 validator that is ~6% (L) / ~2% (VL). Also: - DESIGN.md §10.2 documented a single avo kernel; there are now two. Points at json/internal/utf8x, its separate CPUID detection, and its differential suite — a hand-written validator that wrongly ACCEPTS fails silently, so that suite is the safety net. - The plan's §9 conformance table still claimed the BOM would be carried in VL.LeadingSpace(); it deliberately is not (see the round-2 outcome). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
The writers had the mirror image of the lexer bug, plus an inconsistency of their own. Probed, not assumed: input to String* before Raw before invalid byte mid-string silently -> U+FFFD passed through overlong / surrogate silently -> U+FFFD passed through truncated rune at end ERRORED passed through So a trailing incomplete rune errored while every other ill-formed sequence was silently rewritten, and there was no way to ask for an error at all. RFC 8259 §8.1 requires JSON text to be UTF-8, so silently corrupting a caller's payload is the worse half of that. API --- WithUTF8Policy(UTF8Strict|UTF8Replace|UTF8Passthrough), defaulting to Strict. It is the SAME type the lexers use: UTF8Policy moved to json/internal/utf8x and both packages alias it, so a document refused on the way out and one rejected on the way in mean the same thing. Unbuffered takes WithUnbufferedUTF8Policy -- two names only because Go options are typed per writer; Indented and YAML get it through their existing buffered-option wrappers. UTF8Replace is exactly the old behavior, now explicit and complete: it is the policy under which the output is always valid UTF-8. Note Go strings may legally hold ill-formed UTF-8, so String()/StringBytes() can trip the new default on data from a non-Unicode source; that is the intended change. What is checked --------------- Validated: String, StringBytes, StringRunes, StringCopy, Raw, RawCopy, and VerbatimValue (which renders through the ordinary string path). Trusted: Token and VerbatimToken. The lexers already validate string values as they scan, so re-checking them would cost the token-copy path its reason for existing. The loophole is deliberate and documented: a document lexed with UTF8Passthrough can carry ill-formed bytes in, and then Token silently substitutes U+FFFD (the escaper decodes as it goes, so the output stays valid while differing from the input with no error) and VerbatimToken emits them byte-for-byte, as verbatim requires. Unchecked: NumberBytes/NumberCopy, which already document themselves so; a number holding non-ASCII is a grammar problem, not a UTF-8 one. Two bugs found by the new tests, both fixed ------------------------------------------- 1. The streaming paths validated each read independently, so a lead byte at the end of one chunk followed by a non-continuation at the start of the next was tolerated by both halves and emitted. RawCopy now holds the incomplete tail back and re-joins it with the next read -- reads land at an offset in the buffer that leaves room for the carry, so no second buffer and no allocation -- and StringCopy now validates the sequence its stitcher assembles, which had never passed through the chunk check. 2. Under UTF8Replace a truncated tail still errored instead of substituting, on every escaped path. The remainder is now policy-driven like everything else. Cost ---- Escaped paths are within noise: the escaper already decodes every multi-byte rune, so validation adds a pass the AVX2 kernel makes negligible (ascii 1767 vs 1841 MB/s, cjk 279 vs 291). Raw pays 5.9 vs 8.8 GB/s (-33%), being otherwise a memcpy -- the honest price of Strict meaning what it says; callers who have already validated can select UTF8Passthrough. The token-driven path is untouched by construction. Tests ----- Three policies x five writer configurations (including a minimum-size buffer that forces flushes mid-escape) x every entry point; split-sequence sweeps at read sizes 1..8 for both streaming paths; the trust loophole pinned explicitly; StringRunes rejecting surrogates and out-of-range values. And TestWriterSubstitutionMatchesLexer cross-checks the writers' substitution against utf8x.Sanitize, so "the lexer and the writer share one rule" is enforced rather than asserted -- the two escapers still carry their escaping logic twice (they differ in how they emit), so the rule is what is shared, not the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
fredbi
force-pushed
the
fix/utf8-validation-prototype
branch
from
July 30, 2026 18:46
2677c00 to
16224b8
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (65.62%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #3 +/- ##
==========================================
+ Coverage 62.10% 63.10% +1.00%
==========================================
Files 82 90 +8
Lines 8908 9581 +673
==========================================
+ Hits 5532 6046 +514
- Misses 2877 3036 +159
Partials 499 499 ☔ View full report in Codecov by Harness. |
A mapping whose own "<<" merge key aliases itself
e: &b
<<: *b
made resolveMapping recurse until the stack overflowed. That is a FATAL
runtime error, not a Go panic: safeParse cannot recover it, and neither
WithMaxTokens nor WithMaxContainerStack applies, because the recursion happens
while flattening merge entries — before any token is emitted and outside the
container walk. The process simply dies, which is what CI reported as "fuzzing
process hung or terminated unexpectedly while minimizing".
mergeEntries did have an alias-cycle guard, but its scope was wrong. It spanned
only the resolution of the alias to a node list:
l.expanding[name] = true
entries := l.mergeEntries(target) // a MappingNode just returns its values
delete(l.expanding, name) // released here...
and the expansion of those entries happens afterwards, back in resolveMapping's
add(), which walks them and meets the same "<<" entry with the guard already
released. So the guard covered the step that cannot recurse and left the one
that does unprotected.
The guard now spans the expansion, keyed on the merge SOURCE node rather than
the anchor name. Node identity also catches a cycle formed through a chain of
aliases or a merge sequence, and still lets the same anchor be merged in sibling
positions, which is redundant but legal.
Note on the reported input: the crasher CI recorded,
[]byte("e: &b\n <<: *b\n c ")
does NOT reproduce, because goccy rejects it at parse time ("non-map value is
specified") over the trailing "c ". The kill happened during minimization, on a
mutation that does parse — Go records the pre-minimization input, so the file
points next to the culprit rather than at it. Both are seeded now.
A multi-node merge cycle turns out to be unreachable: closing one needs a
forward reference, and an anchor must be defined before it is aliased, so it
fails earlier as ErrUnknownAnchor. Self-reference is the only shape that gets
there. TestMergeCycleNeedsSelfReference records that, so the narrow fix does not
look like an oversight later.
Tests: cycle shapes rejected with ErrAliasCycle; legitimate merges (repeated
anchors, sequences, 200-deep acyclic chains) still resolved, since a guard that
fails to unmark on the way out would break them; the pre-existing walkAlias
guard pinned as untouched. Each runs on a watchdog so a regression fails the
test rather than taking the suite down. 1M fuzz executions clean, where it
previously died in under 3 seconds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
…d asm Conformance fixtures are DATA: their exact bytes are the test. Without a .gitattributes, a checkout with core.autocrlf=true — the Windows default — rewrites every LF to CRLF and silently changes what is being measured. A JSON fixture that must be REJECTED can start being accepted; a golden file mismatches; and a YAML fixture that hinges on a trailing space or a hard tab becomes a different document, so the failure reads as a lexer bug rather than a checkout artefact. This is why the JSON conformance harness fails on Windows. Marks testdata/**, *.golden and generated *.s as -text. The assembly matters for the same reason: it is checked in and has to match what `go generate` produces byte for byte. No working-tree churn on a LF checkout — the files are already LF, so this only stops them from ever being translated. Verified with git check-attr over the JSON suite fixtures, the i_ behavior golden and both avo-generated kernels. Identical to the copy on the YAML conformance branch, so the two resolve trivially when that work lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
A trailing blank line left by the UTF-8 round-1 edit to this file. The project linter skips _-prefixed directories, so only plain gofmt notices it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Belt-and-braces to the .gitattributes fix: that stops git translating the fixtures, this makes the harness right even when a suite arrives some other way — an unmarked clone, a zip, a copy through a Windows tool. A conformance fixture's exact bytes ARE the test, so a translated checkout does not just fail noisily: a document that must be REJECTED can start being accepted, and the failure reads as a lexer bug rather than a checkout artefact. Fixtures and the i_ behavior golden are now normalised on read. A lone CR is deliberately left alone — it is a meaningful byte inside a JSON document (an unescaped control character a conforming parser must reject), so collapsing it would change the very thing several n_ cases test. Only CRLF collapses. What licenses that: TestConformanceFixturesAreStoredWithLF asserts no vendored fixture carries a raw CR, so every CR that could turn up has been introduced by translation. If upstream ever ships one, the test fails and the normalisation gets revisited instead of silently eating it. The property is tested rather than assumed: every fixture is converted to CRLF and each lexer mode must return the same accept/reject verdict as for the LF original. Mutation-checked — stubbing normalizeLineEndings to the identity fails it, so it has teeth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
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
1 participant
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.
Change type
Please select: 🆕 New feature or enhancement|🔧 Bug fix'|📃 Documentation update
Short description
Fixes
Full description
Checklist