Reject malformed UTF-8 in Parser.UTF8.decode

[carpentry-agent]

Problem

UTF8.decode is documented to return Nothing "on EOF or malformed input", but its multi-byte branches only checked that enough bytes were present (truncation). They never validated the trailing bytes, so UTF8.any-char, char, and codepoint-satisfy — all layered on decode — silently accepted malformed UTF-8:

• Bad continuation byte — C3 28 decoded to U+00E8 and consumed the (, so the next parser never saw it.
• Overlong encodings — C0 80 (overlong NUL) and C0 AF (overlong /, the classic directory-traversal exploit) decoded to U+0000 / /.
• UTF-16 surrogates — ED A0 80 (U+D800) was accepted.
• Above U+10FFFF — F4 90 80 80 (U+110000) was accepted.

This is a contract/robustness bug rather than a new feature — the docstring already promises Nothing on malformed input.

Fix

decode is the only function changed. After confirming enough bytes are present, each multi-byte branch now:

1. verifies every trailing byte is a continuation byte (byte & 0xC0 == 0x80, i.e. 0x80–0xBF);
2. rejects overlong encodings by requiring the codepoint to reach the width's minimum (≥ 0x80 for 2-byte, ≥ 0x800 for 3-byte, ≥ 0x10000 for 4-byte);
3. rejects surrogates U+D800–U+DFFF (3-byte branch);
4. rejects codepoints above U+10FFFF (4-byte branch).

Any failure returns Nothing. any-char already maps Nothing to a non-consuming Reply.ErrEmpty at the lead byte, so the correct backtrack-friendly semantics — fail without advancing, leaving the offending byte for the next parser — come for free. (Leads C0/C1 are always overlong and caught by the ≥ 0x80 check; leads F5–FF yield a codepoint above U+10FFFF or fall through.)

On routing this through utf8.carp instead: I implemented that (@hellerve's ask, once 0.2.0 shipped) and it does not currently compile. utf8.carp defines a top-level (deftype UTF8 …), parsec exposes a public Parser.UTF8 module, and loading both makes carp emit struct UTF8 twice — error: redefinition of 'UTF8' on both ubuntu and macos. It needs a rename on one side or a carp fix, so it can't land here. The work is parked on claude/utf8-decode-at-rewire and written up in the comments below; this PR stays the self-contained fix.

Tests

test/parsec.carp gains a byte-based matrix, built with String.from-bytes &[(Byte.from-int N) …] to match the existing malformed-input tests (no non-ASCII char literals):

• Malformed, each rejected: bad continuation (C3 28), bad second continuation (E0 A0 28), overlong NUL (C0 80), overlong / (C0 AF), overlong 3-byte (E0 80 80, E0 9F BF), overlong 4-byte (F0 80 80 80), low/high surrogate (ED A0 80, ED BF BF), above-max (F4 90 80 80), 5-wide lead (F5 80 80 80), plus the two pre-existing truncated / lone-continuation cases.
• Boundary-valid, each still decodes: U+007F, U+0080, U+07FF, U+0800, U+D7FF, U+E000, U+FFFF, U+10000, U+10FFFF (and é as a sanity anchor).
• Non-consumption: the previously err?-only assertions were strengthened to pin that a malformed lead advances nothing. err? is satisfied by both ErrEmpty and ErrConsumed, so a wrongly-consuming decoder would have passed; the new probe runs (alt (UTF8.any-char) (any-byte)), which reaches any-byte only if any-char rejected the lead without consuming, and checks the surviving remainder (e.g. C3 28 leaves ( unconsumed).

Reverting only the decode change makes exactly the 12 malformed-input assertions fail, confirming the tests pin the bug rather than passing vacuously.

Local carp -x test/parsec.carp → 318/0 (294 baseline + 24 new). carp-fmt --check and angler are clean on parsec.carp; gendocs.carp runs unchanged (decode is hidden, so the public API is untouched).

Separately, an exhaustive old-vs-new differential over 3,174,482 byte sequences confirms this decoder and utf8.carp's decode-at agree on every input — see the comment below.

---

Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.

[carpentry-reviewer]

Build & Tests

Built and ran the suite on Linux/armhf (carp -x test/parsec.carp): 318 passed, 0 failed, matching your number (294 baseline + 24 new). CI is green on both ubuntu-latest and macos-latest.

I verified the tests aren't vacuous rather than taking it on trust — restored parsec.carp from main (old decode) while keeping the new test file, and got 306 passed, 12 failed. Exactly your 12 malformed-input assertions fail against the old decoder (the C3 28 codepoint and its "leaves ( unconsumed" tail probe, E0 A0 28, C0 80, C0 AF, E0 80 80, E0 9F BF, F0 80 80 80, ED A0 80, ED BF BF, F4 90 80 80, F5 80 80 80). The two already-handled cases (truncated 195, lone-continuation 128) correctly pass on both, so they're not among the 12. The tests pin the bug.

Findings

None. I traced every branch of decode against the committed code:

• Dispatch is complete and safe. b0 < 192 catches lone continuation bytes → Nothing; the final Nothing catches b0 >= 248 (F8–FF). Every byte-at is guarded by the preceding (Int.>= (Int.+ pos N) len) truncation check, so b1/b2/b3 can't index past the end — important given Carp's unchecked String.char-at.
• The value-range checks are the correct equivalent of Unicode Table 3-7. Overlong is rejected by cp < 0x80 / 0x800 / 0x10000; surrogates by 55296 <= cp <= 57343; max by cp > 1114111. That folds in the byte-range restrictions on the first continuation byte (E0/ED/F0/F4) exactly. C0/C1 leads fall out via cp < 128; F5–F7 via cp > 1114111.
• No over-rejection at the boundaries. The valid matrix straddling the surrogate hole (U+D7FF and U+E000 both decode, D800/DFFF rejected) and the exact per-width min/max (U+0080, U+07FF, U+0800, U+FFFF, U+10000, U+10FFFF) all decode — confirmed in the run, and the boundary codepoints round-trip through Char correctly (U+10FFFF → 1114111), so there's no width truncation.
• No overflow. The widest cp is 0x1FFFFF, well inside Int; byte-at's (bit-and 255 …) neutralizes any sign-extension from Char.to-int.

The fix is at the right layer — the single decode chokepoint — so any-char, char and codepoint-satisfy all inherit it. The CHANGELOG entry is warranted (the public parsers' behavior genuinely changes) and correctly filed under ## Unreleased. Branch is on current main (merge-base == origin/main), so no changelog-misfile risk.

Verdict: merge

Correct, comprehensive, bounds-safe, and backed by a test set I confirmed is non-vacuous. Nothing to change.

[hellerve]

Shouldn’t we let the utf8 library handle these things instead?

[carpentry-agent]

Good question — I dug into whether Parser.UTF8.decode could defer to utf8.carp, and the honest answer is that the two solve different problems, so it isn't a clean drop-in. Sharing what I found so you can steer:

What utf8.carp exposes today is whole-buffer validation: valid? ((Ref (Array Byte)) → Bool, RFC 3629, rejecting overlongs / surrogates / > U+10FFFF) and from-bytes (… → (Maybe UTF8)). to-runes splits via String.chars, which already assumes well-formed input.

What Parser.UTF8.decode needs is incremental: decode exactly one codepoint at a byte cursor and report its width (decode : src len pos → (Maybe (Pair Char Int))), so any-char / char / codepoint-satisfy can advance one column at a time and fail empty (un-consuming) on the first malformed byte. utf8.carp has no single-codepoint "decode next + how many bytes" primitive, and parsec currently depends only on strbuf, not utf8.carp.

So there are two paths:

• Keep this PR as is — it's the minimal, self-contained fix to the contract decode already advertised ("Nothing on malformed input") but didn't honor, and it adds no dependency. The cost is that the Unicode Table 3-7 checks now live in two places (here and utf8.carp.valid?).
• Centralize in utf8.carp — add an incremental primitive there (decode the codepoint at a byte offset, returning it plus its width, validating once) and route both valid? and parsec's decode through it, removing the duplication at the source. That's the cleaner end state, but it's cross-repo: parsec takes a new utf8.carp dependency, and it can't land atomically (parsec can only load a tagged utf8.carp), so it'd be a follow-up sequenced after a utf8.carp release.

This PR's fix is correct and independently reviewable either way, so my suggestion is to merge it as the immediate correctness fix, and — if you want the single-source-of-truth version — I'm happy to do the utf8.carp incremental-decode primitive plus the parsec rewire as a separate follow-up. Which direction do you prefer?

[hellerve]

We’re in no hurry to get this in, so I’d do the more principled fix right away.

[carpentry-agent]

Per your ask to do the principled fix right away, I've built its foundation in utf8.carp: carpentry-org/utf8.carp#9.

That PR adds a public incremental decoder, UTF8.decode-at — decode one code point at a byte offset, returning (Just (Pair codepoint width)) or Nothing — carrying the full Table 3-7 checks (overlong / surrogate / > U+10FFFF / continuation ranges), and re-expresses valid? on top of it (proven byte-for-byte identical by an exhaustive old-vs-new differential). So the validation logic that was about to be duplicated between utf8.carp's valid? and this PR's decode now has a single home. It takes (Ref (Array Byte)) and returns the code point as an Int specifically so parsec can route through it — that dodges the NUL-truncation and Char-is-a-byte-vs-code-point mismatches a String-based shared decoder would hit.

The catch is sequencing: parsec can only load a tagged utf8.carp, so routing Parser.UTF8.decode through decode-at (and deleting the inline validation here) can't land atomically with #9 — it has to follow once utf8.carp cuts a release. So the choice is yours:

• hold Reject malformed UTF-8 in Parser.UTF8.decode #24 as the interim, dependency-free correctness fix until utf8.carp is released, then do the rewire as a follow-up; or
• close Reject malformed UTF-8 in Parser.UTF8.decode #24 in favor of the rewire, accepting decode stays as-is on main until the release lands.

Happy to take either path — just say which you prefer. I haven't touched this branch or parsec's code; this is only a heads-up.

[hellerve]

0.2.0 for utf8.carp was published, so the discussion is moot. we can just depend on that.

[carpentry-agent]

Thanks for the go-ahead, and for clearing the sequencing — done. Parser.UTF8.decode now routes through UTF8.decode-at from utf8.carp 0.2.0.

What went: the ~45 lines of inline dispatch, continuation-byte checking and overlong/surrogate/range logic that the first commit added. decode is now a thin adapter: it builds a min(4, len - pos)-byte window at the cursor, calls (decode-at &window 0), and wraps the Int code point back into a Char. Net −50/+21 in parsec.carp. Its signature, its private/hidden markers and the three call sites (any-char, char, codepoint-satisfy) are untouched, so they inherit the shared validation as-is. Truncation-at-end-of-input needed no special handling — a short window makes decode-at return Nothing for exactly the inputs the old (pos + N >= len) guards rejected.

What the differential proved: I ran the old decoder and the new one side by side over 3,174,482 cases — every 1-byte and every 2-byte input exhaustively, every lead byte crossed with continuation-boundary bytes at 3 and 4 bytes wide, and the code point edges around each width's min/max and the surrogate hole; each re-run at every truncated length and at a shifted pos. Zero disagreements in constructor, code point or width. Suite is unchanged at 318/0.

That was worth building rather than asserting: the first bridge I wrote used String.byte-slice + String.to-bytes, which reads equivalent but isn't — to-bytes is strlen-based, so a NUL lead byte gives an empty window and Nothing where the old decoder returned U+0000 width 1. 12,402 cases disagreed. The shipped version reads bytes directly.

The one trade-off, for you to weigh: decode-at takes a (Ref (Array Byte)), so the window is a malloc/free per code point that the inline version didn't pay. It's real — many(any-char) over mixed-width text went 2.5× slower. I mitigated it by short-circuiting ASCII leads (b0 < 128) before building any window, which brings it to 1.57× on that mixed-width corpus and 1.04× on ASCII-heavy input. That b0 < 128 test is the only decode logic left in parsec, and it duplicates one comparison rather than any of the Table 3-7 checks — but it is a deliberate deviation from "nothing here at all", so say the word if you'd rather drop it and eat the full 2.5×; it's a one-line change. The way to get both would be a &String-based overload in utf8.carp, which needs another release there — happy to do that as a follow-up if you want it.

CHANGELOG notes the new dependency; the strictness entry stays. carp-fmt --check and angler are clean, and gendocs regenerates docs/ byte-identical. Leaving CI to confirm and not merging.

[carpentry-agent]

Correction to my comment above: I pushed the utf8.carp rewire, CI rejected it, and I've rolled this branch back to the reviewed self-contained fix (7c374d4, green, 318/0). The rewire is parked on claude/utf8-decode-at-rewire. Sorry for the churn — here's what happened, because the blocker is one you'll want to know about regardless of this PR.

The rewire itself works. decode becomes a thin adapter: decode-at takes a (Ref (Array Byte)) while decode has a &String and a byte cursor, so it builds a min(4, len - pos)-byte window at the cursor and calls (decode-at &window 0). Truncation falls out for free — a short window makes decode-at return Nothing for exactly the inputs the old (pos + N >= len) guards rejected. ~45 lines of inline dispatch/continuation/range logic deleted, net −50/+21, signature and the three call sites untouched. It passes 318/0 locally.

I proved it behaviour-preserving before pushing: old vs. new over 3,174,482 cases — every 1-byte and every 2-byte input exhaustively, every lead byte crossed with continuation-boundary bytes at 3 and 4 bytes wide, and the code point edges around each width's min/max and the surrogate hole; each re-run at every truncated length and at a shifted pos. Zero disagreements in constructor, code point or width. So decode-at is a correct drop-in for this decoder — that part is settled, and it's worth knowing independently of how the packaging shakes out.

(That differential earned its keep: my first bridge used String.byte-slice + String.to-bytes, which reads equivalent but isn't — to-bytes is strlen-based, so a NUL lead byte gives an empty window and Nothing where the old decoder returns U+0000 width 1. 12,402 cases disagreed. The parked version reads bytes directly.)

What blocks it — a name collision, not a logic problem. utf8.carp defines a top-level (deftype UTF8 [runes (Array Rune)]); parsec exposes a public (defmodule UTF8 …) under Parser. With both loaded, carp emits struct UTF8 twice:

out/main.c:2365:8: error: redefinition of 'UTF8'
 2365 | struct UTF8 {
out/main.c:2360:8: note: previous definition is here

Deterministic on both ubuntu-latest and macos-latest. It does not reproduce on the carp I have here (905f9457), which is why it got past me locally — my apologies for finding this in your CI rather than before pushing.

I checked whether it was the reference site by hoisting the UTF8.decode-at call into a shim outside Parser.UTF8 entirely. Same duplicate struct. So it's the coexistence of the type and the module name, not how the symbol is reached — no reference-site trick fixes it. (That probe also turned up a second, smaller thing: CI's carp enforces private across submodules, so a private binding in Parser isn't reachable from Parser.UTF8, where the local one allows it.)

That leaves three ways out, and all three are your call rather than mine:

1. utf8.carp renames the type (UTF8.T, or fold it into Rune) — the type is the newer, less-established name of the two.
2. parsec renames Parser.UTF8 — breaking public API for any-char / char / codepoint-satisfy, which I'd rather not do for this.
3. carp scopes the emitted struct name so a module and a type can share a name — the real fix, but the biggest.

Happy to take any of them once you've picked; option 1 looks cheapest to me. In the meantime this PR is back to the version your reviewer signed off on, so it's mergeable as-is and the dedup can follow whenever the naming is sorted.

[carpentry-reviewer]

Build & Tests

This branch is back at 7c374d4 — byte-for-byte the commit I reviewed on 07-21, not a revert commit layered on top. carp -x test/parsec.carp → 318/0 on armhf, and CI re-ran against this exact SHA at 22:57 (both ubuntu-latest and macos-latest green), so the rolled-back state is independently verified rather than inheriting July's run.

The rollback is clean: git diff main..7c374d4 touches only CHANGELOG.md, parsec.carp and test/parsec.carp, with no residue of the rewire — no decode-at reference, no utf8.carp dependency line in the changelog. Merge-base is 2e63d75, current main.

Prior feedback

My 07-21 review of this code stands unchanged — the review's commit SHA is the current head, so there is no new code here to re-review. The decoder, the Table 3-7 equivalence and the non-vacuity of the 12 malformed-input assertions were all checked then and none of it moved.

Findings

None in the code. What I did check is the blocker report, since it's the part of this PR that's new and the part you'll act on.

It is accurate, verbatim. Pulling the failed job logs directly:

30b6ccd (rewire)       out/main.c:2365:8: error: redefinition of 'UTF8'
                       out/main.c:2360:8: note: previous definition is here
13e0f7d (retry)        same, same lines

on both ubuntu-latest and macos-latest. The secondary observation checks out too — the middle attempt (d28bd21, the shim probe) failed differently and for the stated reason:

The binding: Parser.codepoint-at is private; it may only be used
within the module that defines it.  (parsec.carp:1253)

So CI's carp enforces private across submodules where the local one doesn't. Both are worth knowing independently of this PR. The parked branch is where it says it is (claude/utf8-decode-at-rewire at 30b6ccd, −50/+21 in parsec.carp).

One thing that may make the naming decision cheaper than it looks

The write-up frames option 1 as "rename utf8.carp's type", which reads like a breaking change to a published library. Two facts narrow that:

The two bindings anyone actually calls don't mention the type. UTF8.valid? (utf8.carp:185) and UTF8.decode-at (:166) both take (Ref (Array Byte)) and neither names UTF8 in its signature — they only live in that module because the deftype created it. They could sit in a plain (defmodule UTF8 …) next to a renamed type, which keeps UTF8.valid? and UTF8.decode-at source-compatible for every caller. Only the bindings that genuinely operate on the type (from-string, init, append, reverse, slice, …) would move.

The org-wide blast radius of the type itself is one file. Grepping every carpentry clone, the only consumer of utf8.carp outside its own tree is web/web.carp, and it calls exactly one binding — UTF8.valid?, at :2418 and :2454. Nothing outside utf8.carp names the UTF8 type.

Caveat I can't remove from here: whether a top-level (defmodule UTF8 …) in utf8.carp coexists with parsec's (defmodule UTF8 …) under Parser is exactly the thing the local carp can't tell us — 905f9457 doesn't reproduce the collision at all. It's a one-commit throwaway branch to find out on CI, and worth doing before committing to a rename, because if two same-named modules also collide then option 1 is no cheaper than option 2 and the real answer is option 3.

Verdict: merge

Unchanged from 07-21 and now re-confirmed at this SHA: the self-contained fix is correct, bounds-safe, and its tests demonstrably pin the bug. The rewire is a separate question that this PR correctly stopped waiting on, and decode is a single chokepoint, so swapping its body later is a small diff against whatever naming you pick.

---

Reviewed by the carpentry-org review agent (Claude).

[hellerve]

i’ll have to think on this, let’s park it.

[hellerve]

Rebased onto main (0.6.0); only CHANGELOG conflicted, source and tests auto-merged. Verified after the rebase: 356/0, and a byte-array probe shows main accepting all six malformed sequences (C3 28, C0 80, C0 AF, ED A0 80, F4 90 80 80, E0 80 80) while this branch rejects exactly those and still accepts valid two- and four-byte codepoints. Lead-byte space is fully covered: stray continuation bytes fall out at the b0 < 192 arm, C0/C1 at the overlong test.