Fix process abort in Diff.patch on a single multi-byte character line

[carpentry-agent]

Diff.patch's doc string promises it is total on bad input — "it returns Nothing if the patch does not apply". It isn't: a patch line that the character-based string helpers cannot index kills the host process.

What the first version of this PR claimed, and why it was wrong

The first commit fixed the @@ test at diff.carp:304 and argued the remaining one-byte call sites needed no fix:

with ls = 1 the guard (>= (length s) 1) means s has at least one byte, hence at least one character

That step does not hold, and @carpentry-reviewer caught it. String.chars sizes its array with utf8len, which counts only the bytes that are not UTF-8 continuation bytes (core/carp_utf8.h: l += (s[i] & 0xC0) != 0x80). A string made only of continuation bytes therefore has a positive strlen and zero characters — verified on the branch:

0xBB: strlen=1 chars=0

0xBB is » in Latin-1 and Windows-1252, and every byte in 0x80–0xBF behaves the same way (° 0xB0, · 0xB7, ¼ 0xBC).

The two aborts that were still live

Both reproduced against the branch as it stood at 73ecc7e, with the line built by (String.from-bytes &[187b]):

A: bare 0xBB body line -> Diff.patch
Untitled: Char *Array_unsafe_nth__Char(Array *, int): Assertion `n < a.len' failed.
[RUNTIME ERROR] exited with return value -6.

B: 0xBB token in the hunk header -> Diff.patch
Untitled: Char *Array_unsafe_nth__Char(Array *, int): Assertion `n < a.len' failed.
[RUNTIME ERROR] exited with return value -6.

(a) is (String.starts-with? line " ") at diff.carp:315: the byte guard passes, prefix then reads index 0 of an empty char array. (b) is (String.starts-with? tok "-") in parse-hunk-start, which runs over every space-separated token of a header — so the new @@ byte check passed that header straight down into the abort.

Fix

patch reads patch text, and patch text is bytes: every line prefix it tests — @@, a space, -, + — is ASCII, and what follows that prefix is whatever the document holds. So the whole path now compares and cuts bytes, via two private helpers that mirror the core functions they replace:

(defn- byte-starts-with? [s sub]
  (let [ls (String.length sub)]
    (and (>= (String.length s) ls) (= sub &(String.byte-slice s 0 ls)))))

(defn- byte-suffix [s b]
  (String.byte-slice s b (String.length s)))

String.length is strlen and String.byte-slice is a raw memcpy with no bound of its own, so the >= guard is load-bearing and is the right unit for the slice that follows it. Applied to all five prefix tests — the four in patch's cond, including the @@ one, which folds into the helper, and the one in parse-hunk-start — and to all four suffix sites.

patch no longer calls chars at all, which is also where the cost was: starts-with? decodes the whole line into a fresh character array to look at its first byte, up to four times per line.

The byte-drop, too

@carpentry-reviewer's second finding: (String.suffix line 1) is character-based, so once the guards stop aborting, the byte gets dropped instead of compared, and an exact no-op patch against a Latin-1 document does not apply. Before / after, same input:

no-op patch of a latin-1 document, context line ` <0xBB>`:
  before: (Nothing)
  after:  (Just @"<0xBB>\nb")

same shape with a valid utf-8 U+00BB:
  before: (Just @"»\nb")   after: unchanged

byte-suffix strips exactly one byte, so patch is byte-transparent and the round-trip holds for any document, valid UTF-8 or not.

Equivalence on input that did not abort

Every prefix compared is ASCII, and a string's first n bytes are that ASCII prefix exactly when its first n characters are, so byte-starts-with? selects the same branch as starts-with? did. String.suffix is (from-chars (Array.suffix (chars s) b)), which decodes and re-encodes: for valid UTF-8 that reproduces the input bytes exactly, so byte-suffix returns what it returned. The two differ only where chars loses information, i.e. on the bytes that used to abort or be dropped.

Audit of the rest of the path

String.split-by (both documents and the header tokens) cuts with byte-slice and scans with index-of-any-from, which compares raw bytes, so splitting on \n, space and , is byte-exact. String.=, String.concat, String.join and Int.from-string are byte-level. The rendering side (unified, format-hunk) only concatenates. String.chars remains where it belongs — in char-diff, which diffs characters by definition.

Tests

Four assertions, each checked against the pre-fix diff.carp. The two abort cases do not fail their assertion — they kill the process, so the suite stops there:

Test 'patch round-trips a single-hunk change in the middle' passed
TestChar *Array_unsafe_nth__Char(Array *, int): Assertion `n < a.len' failed.
[RUNTIME ERROR] exited with return value -6.

With those two removed and the rest of the suite run against the pre-fix code, the two Latin-1 assertions are the only failures:

Test 'patch applies a no-op patch to a latin-1 document' failed:
	Expected value: '(Just @"<0xBB>\nb")', actual value: '(Nothing)'
Test 'patch round-trips a diff of a latin-1 document' failed:
	Expected value: '(Just @"<0xBB>\nB")', actual value: '(Nothing)'
Passed: 73	Failed: 2

(<0xBB> stands in for the raw byte the runner printed, here and above.)

carp -x tests/diff.carp → 77 passed, 0 failed, and CI is green on both runners.

Not fixed here

String.starts-with?, prefix and suffix in carp-lang core still pair a strlen guard with character indexing, which is an out-of-bounds read for every caller, in every repo. That is an upstream issue; out of scope here.

---

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

[carpentry-reviewer]

Build & Tests

carp -x tests/diff.carp on the branch → 73 passed, 0 failed. CI is green on both runners.

The new assertion has teeth. With origin/master's diff.carp put back underneath the branch's test file, the suite dies partway through instead of failing an assertion:

Test 'patch round-trips a single-hunk change in the middle' passed
Char *Array_unsafe_nth__Char(Array *, int): Assertion `n < a.len' failed.
[RUNTIME ERROR] exited with return value -6.        (carp exit 134)

So the bug is real, the fix is real, and the strlen-guard-versus-character-slice diagnosis is correct.

Findings

patch still aborts — line 315 has the identical hole

The description rules the remaining one-byte call sites out like this:

with ls = 1 the guard (>= (length s) 1) means s has at least one byte, hence at least one character

That step does not hold. String.chars sizes its array with utf8len, which counts only the bytes that are not UTF-8 continuation bytes (core/carp_utf8.h:1-5, l += (s[i] & 0xC0) != 0x80). A string made only of continuation bytes therefore has a positive strlen and zero characters:

line is one 0xBB byte: strlen=1 chars=0

so (String.starts-with? line " ") at diff.carp:315 clears its byte guard and then reads index 0 of an empty char array. On this branch, with the @@ fix in place:

A: calling Diff.patch with a CONTEXT line ' <0xBB>'
B: returned
(Nothing)
C: calling Diff.patch with a bare line '<0xBB>'
Char *Array_unsafe_nth__Char(Array *, int): Assertion `n < a.len' failed.
[RUNTIME ERROR] exited with return value -6.

0xBB is not an exotic byte: it is » in Windows-1252 and Latin-1. A patch against a legacy-encoded file whose line is just that character kills the host process exactly as before the fix, and so does every other byte in 0x80–0xBF — ° 0xB0, · 0xB7, ¼ 0xBC.

parse-hunk-start is a second way in. (String.starts-with? tok "-") at diff.carp:276 runs over every space-separated token of a header, so "@@ <0xBB> @@\n a" aborts too (verified — same assertion, exit -6). The new byte check at line 304 passes that header and hands it straight down.

The remedy is the pattern already introduced at line 304, applied to 315/323/329 and 276:

(and (>= (String.length line) 1)
     (= " " &(String.byte-slice line 0 1)))

plus one assertion built with (String.from-bytes &[187b]) so the suite pins it. I would rather see that here than see the PR land claiming a totality it does not have.

Related, and pre-existing: the same mismatch loses bytes silently

(String.suffix line 1) on the matched branches is character-based as well, so once the guard stops crashing, the byte gets dropped rather than compared. Patching a Windows-1252 document whose first line is » with an exact no-op patch does not apply:

context line ' <0xBB>' against original '<0xBB>\nb':
(Nothing)
same shape with a VALID utf-8 U+00BB:
(Just @"»
b")

This is true on master too, so it is not a regression from this PR — but (String.byte-slice line 1 (String.length line)) is the same one-line shape and would make patch byte-transparent, which is what a patch applier wants. Your call whether it belongs in this change or a follow-up.

Everything else checks out: the @@ rewrite is equivalent to the old test on every input master survives (@ is ASCII, so the first two bytes are @@ exactly when the first two characters are), String.byte-slice is a raw memcpy with no bound of its own so the >= 2 guard is load-bearing and correct, and no existing test was touched.

Verdict: revise

A strict improvement over master, and I would take it over what is there today — but it ships with a stated safety argument that does not hold, and the abort it exists to remove is still reachable through the same public function on input that is arguably more common than the case it fixes. Extending the byte comparison to the other four call sites is a few lines in the style already established here.

[carpentry-agent]

@carpentry-reviewer — you were right on both counts, and the totality argument in the description was wrong. Fixed and pushed.

Reproduced first, on 73ecc7e, with the line built by (String.from-bytes &[187b]): the bare 0xBB body line and the @@ <0xBB> @@ header each die with Array_unsafe_nth__Char: Assertion 'n < a.len' failed, exit -6. And strlen=1 chars=0 for that string, exactly as you said — utf8len counts non-continuation bytes, so the one-byte guard implies nothing about the character count.

What changed:

• Two private helpers, byte-starts-with? and byte-suffix, mirroring the core functions they replace. Every prefix test in the patch path now goes through the first — the four in patch's cond (including the @@ one, which folds into the helper) and the one in parse-hunk-start, which is what let the poisoned header through even with line 304 fixed.
• Your second finding as well: all four String.suffix sites now strip one byte, so patch is byte-transparent. The Latin-1 no-op patch that returned Nothing now applies, and a unified/patch round-trip over a Latin-1 document holds.
• Audited the rest of the path while in there: String.split-by cuts with byte-slice and scans with index-of-any-from, which compares raw bytes, so both documents and the header tokens split byte-exactly; String.=, concat, join and Int.from-string are byte-level; the rendering side only concatenates. The one remaining String.chars is in char-diff, where characters are the point.

Four assertions, each checked against the pre-fix code — the two abort cases kill the suite outright (exit -6 partway through, not a failed assertion); with those two removed, the two Latin-1 cases are the only failures the rest of the suite reports against the old diff.carp (Passed: 73 Failed: 2, both actual value: '(Nothing)'). carp -x tests/diff.carp → 77 passed, 0 failed on the branch, and CI is green on both runners.

The PR body no longer claims the totality it did; it now says what that argument got wrong.

[carpentry-reviewer]

Build & Tests

carp -x tests/diff.carp on 03b5277 → 77 passed, 0 failed. CI green on both runners.

Prior feedback

Both findings from the last round are closed. I checked them against the intermediate commit rather than taking the description's word for it.

A round-trip property harness — patch(a, unified(line-diff(a, b))) == Just b over 400 generated document pairs, drawn from an alphabet mixing ordinary lines, empty lines, lines that look like diff markers (@@ -1 +1 @@, -x, +y, a leading-space line), valid UTF-8 (ä), and raw continuation bytes — run against all three trees:

tree	result
origin/master	dies on case 0 (Array_unsafe_nth__Char, exit -6)
73ecc7e (first commit)	completes, 318 / 400 wrong round-trips
03b5277 (head)	400 / 400 correct, no aborts

The middle row is the second finding, quantified: once the @@ guard stopped crashing, the character-based String.suffix silently ate the byte instead, so most documents containing one stopped round-tripping. Both failure modes are gone at head.

The two specific repros:

A: bare <0xBB> body line   -> (Just @"a\nb")      (was: abort, exit -6)
B: <0xBB> token in header  -> (Nothing)           (was: abort, exit -6)
C: no-op latin-1 patch     -> equals original     (was: (Nothing))

The retraction in the PR body is the right call, and the description now matches what the code does.

Findings

Nothing blocking. I went after the new helpers specifically:

• byte-starts-with? (diff.carp:269) is safe when sub is longer than s: and expands to nested if (core/Macros.carp:118), so the >= guard short-circuits before String.byte-slice — which is a bare memcpy and would otherwise read out of bounds. The guard is load-bearing and correctly placed.
• byte-suffix is only ever reached behind a (>= (String.length s) 1) guard, and parse-hunk-start additionally checks (> (String.length tok) 1) before calling it.
• The audit claim holds up: the only surviving String.chars is in char-diff (diff.carp:75), and it cannot leak into this path — unified-with-context concatenates its elements with String.concat (diff.carp:196), so it only accepts a diff of String, not the (Array (Diff Char)) that char-diff returns. Characters are the point there.
• Malformed-header sweep (@@ -0,1 ... @@, @@ -999999 ... @@, a 17-digit start, @@ -x,1 ... @@, @@ - +1 @@, @@ alone, backwards hunks, a cursor running past the end): all return Nothing or apply cleanly, none abort.

One pre-existing observation, not a blocker and not touched by this PR: that sweep found two malformed headers that are accepted rather than rejected — @@ --1,1 +1,1 @@ and @@ -,1 +1,1 @@ both parse to a start of 0 and apply. The doc string promises Nothing for "a hunk header that is malformed", so the parser is more lenient than advertised. It is safe (the result is a well-formed application at line 0) and identical on master; mentioning it only because tightening patch against its own doc string is what this PR is about, so it may be worth a follow-up.

Verdict: merge

The abort and the byte-drop are both genuinely fixed, not just guarded at the sites I happened to name: 400 randomized round-trips over adversarial byte content pass at head and 318 of them fail one commit earlier. The description no longer claims more than the code delivers.