Fix two byte-index-into-character-slice bugs in Path - #13
Conversation
`String.starts-with?` in core guards on `(length s)`, which is `strlen` and
therefore counts bytes, but then compares with `prefix`, which slices
characters via `String.chars`. `chars` sizes the array with `utf8len`, which
counts only non-continuation bytes, so a one-byte string whose byte is in
0x80-0xBF has a byte length of 1 and zero characters, and the slice runs off
the end:
(Path.absolute? &(String.from-bytes &[187b]))
; Assertion `n < a.len' failed, exit -6
POSIX filenames are arbitrary byte strings, so a bare 0xBB — a Latin-1 »
— is a legal name on disk, and `absolute?` is the predicate every other
function in the module routes through: `relative?`, `absolute`, `normalize`,
and `relative` all abort on it.
A leading `/` is always a whole character, so reading the first byte and
comparing it to \/ keeps the answer identical for every input that did not
crash; a differential over ASCII, multi-byte and truncated-UTF-8 inputs
reports no mismatches. The empty-string guard is explicit rather than relying
on the NUL terminator.
The windows-only branch matches a drive-letter pattern and is untouched.
There was a problem hiding this comment.
Build & Tests
carp -x test/path.carp on ca69b0c → 120 passed, 0 failed. CI green on both runners.
The change itself is correct. The new assertion has teeth — calling master's predicate (String.starts-with? p "/") on a lone <0xBB> aborts with Array_unsafe_nth__Char: Assertion 'n < a.len' failed, exit -6, while Path.absolute? at head returns false and /<0xBB> returns true. A differential of the old and new predicate over 21 inputs — "", "/", "//", "a", "/a", "a/", ".", "./a", "..", "ä", "ä/b", "/ä", "grüße/x", "C:\x", " /", "/ ", a leading tab, "日本/x", a bare lead byte <0xC3>, "/<0xBB>", "<0x80>/" — reports 0 mismatches. relative?, normalize, join and filename all survive continuation-byte input and preserve the byte.
Findings
split-extension has the identical bug, and it is still live
path.carp:165-168:
(let [i (Pattern.find extension-pat p)]
(if (= -1 i)
(Maybe.Nothing)
(Maybe.Just (Pair.init (prefix p i) (suffix p (inc i))))))
Pattern.find returns MatchResult.start from the C matcher — a byte index. String.prefix/String.suffix slice characters. That is the same byte-guard-versus-character-slice mismatch the PR exists to remove, one screen further down the same file, and it fails both ways.
It aborts. Same assertion, same exit code, on the branch as it stands:
(split-extension "<0xBB><0xBB>.") -> Array_unsafe_nth__Char assertion, exit -6
(split-extension "<0xBB><0xBB><0xBB>.") -> same
(split-extension "<0xBB><0xBB><0xBB>.x") -> same
Two continuation bytes are one byte-index of . past the character count, so Array.prefix runs off the end exactly as absolute? did. By the PR's own argument — POSIX filenames are arbitrary byte strings — »». is as legal a name as ».
And it is silently wrong on input that is not exotic at all. Any non-ASCII in the basename shifts the byte index past the character index:
(split-extension "a.txt") -> (Just (Pair @"a" @"txt")) ok
(split-extension "ä.txt") -> (Just (Pair @"ä." @"xt")) wrong
(split-extension "ää.txt") -> (Just (Pair @"ää.t" @"t")) wrong
(split-extension "grüße.c") -> (Just (Pair @"grüße.c" @"")) extension lost entirely
which propagates to everything built on it:
(extension "ä.txt") -> (Just @"xt")
(is-extension? "ä.txt" "txt") -> false
(drop-extension "ä.txt") -> "ä."
A false is-extension? on an accented filename is a more likely thing to hit in practice than the crash this PR fixes.
Why the PR's verification missed it. The description says a probe over the reachable callers "— relative?, normalize, relative, absolute, filename, basename, matches?, split-extension — no longer aborts on "\xBB" or on "/\xBB"". That is true, but neither probe input contains a ., so Pattern.find returns -1 for both and the prefix/suffix branch is never entered. The probe could not have reached the bug it was aimed at.
The fix is the same one-line shape already used elsewhere in the module. i is a byte index, so cut bytes:
(Maybe.Just (Pair.init (String.byte-slice p 0 i)
(String.byte-slice p (inc i) (String.length p))))
byte-slice is a raw memcpy, and i here comes from a successful match so it is in [0, len) — no extra guard needed. Worth an assertion on "ä.txt" and one built with (String.from-bytes &[187b 187b 46b]).
Not worth acting on
matches? (path.carp:425) goes through String.chars on both arguments, so a lone continuation byte matches * but not ?. It does not abort, and glob semantics over invalid UTF-8 are genuinely arguable — noting it only so it is not mistaken for an oversight later.
Verdict: revise
The one-line change to absolute? is correct, minimal and well tested, and I would take it over master as-is. But the PR's stated verification does not hold — the module still aborts on the same class of legal filename it was opened to protect against, two functions down, and that same function is quietly returning wrong extensions for every accented filename today. Both are one byte-slice away, in the style this PR already establishes; folding them in here is a better outcome than shipping the description's claim as written.
Pattern.find returns MatchResult.start from the C matcher, a byte
offset, while String.prefix and String.suffix slice characters through
String.chars. Any multi-byte character before the dot shifts the byte
index past the character index, so the split lands in the wrong place:
"ä.txt" split as ("ä." . "xt") and "grüße.c" lost its extension
entirely, which propagated to extension, is-extension? and
drop-extension. When the byte index runs past the character count
outright — a name of UTF-8 continuation bytes such as <BB><BB>. — the
character slice ran off the end of the array and aborted, the same
assertion this branch removes from absolute?.
Cutting with String.byte-slice matches the index to the slice.
core's String.split-by already splits this way.
|
@carpentry-reviewer — you were right on both counts, and the reproduction came out exactly as you described. Fixed in 81cb3a3. What I reproduced first, on ca69b0c, before touching anything. The abort, on And the silent wrong answers, character for character what you posted: Your diagnosis of why the original probe missed it is also correct: neither What changed. Your suggested shape, verbatim: (Maybe.Just
(Pair.init (String.byte-slice p 0 i)
(String.byte-slice p (inc i) (String.length p))))Checking the rest of the module for the same class, as you asked. Teeth. Four new assertions, and I checked each of them fails on the old code:
With the fix: 124 passed / 0 failed. Beyond the assertions, since the failure is a silent wrong answer and not just a crash:
On #12: still clean. Merging
|
There was a problem hiding this comment.
Build & Tests
carp -x test/path.carp on 81cb3a3 → 124 passed, 0 failed. CI green on both runners.
Prior feedback
Both points from the previous round are addressed.
split-extensioncut on a byte index — fixed in81cb3a3, in the shape suggested (String.byte-sliceon both halves). Confirmed below against an independent oracle.matches?(path.carp:425) and thewindows-onlyabsolute?— left alone, which is what I asked for.
The teeth claim reproduces exactly. Reverting path.carp to ca69b0c and dropping only the continuation-byte assertion gives 120 passed / 3 failed, and the three are precisely the accented ones:
Test 'split-extension splits at the dot in a multi-byte filename' failed
Test 'split-extension keeps the extension of a multi-byte filename' failed
Test 'is-extension? works on a multi-byte filename' failed
With that assertion back in place the suite dies at exit 134 before it can report. Reverting path.carp all the way to master kills the suite even earlier, on ca69b0c's own absolute? assertion — so both commits' tests have teeth.
Findings
None. I went looking specifically for a case the new byte slicing gets wrong, and there isn't one.
split-extension, 2356 inputs, byte-exact. A corpus of every 1–3 element combination of a, ä, 日, 😀, a lone \xBB, a lone \x80, a bare lead byte \xC3, ., .., /, x.y, - and space, plus the documented examples and the accented/invalid-UTF-8 names from the PR. Each input run through Path.split-extension on master and on the branch, dumped as hex, and compared against re.search(rb'\.[^/.]*$', p) on the raw bytes:
| count | |
|---|---|
| fixed by this PR | 738 (of which 173 were SIGABRT on master, 565 silently wrong) |
| both already correct | 1618 |
| broken by this PR | 0 |
| both wrong | 0 |
The branch agrees with the byte oracle on all 2356. Every input master handled correctly, the branch still handles correctly.
absolute?, same 2356 inputs. master aborts on 14 (all strings made only of continuation bytes); on the other 2342 the two agree. The branch matches the oracle absolute? ⟺ first byte is 0x2F on all 2356, and relative? is exactly its negation on all 2356. (absolute? "") returns false without touching the NUL terminator, so the written-out guard does its job.
Downstream, spot-checked on the branch: replace-extension "ä.txt" "md" → ä.md, is-extension? "grüße.c" "c" → true, drop-extension on <BB><BB>. → the bytes bbbb. carp -x gendocs.carp produces no diff.
One transcription slip, for the record only. The follow-up comment renders master's pre-fix result as grüße.c -> Just(grüße | ). My measurement — and the PR description's own table — both give Just("grüße.c" | ""). The conclusion you drew from it (the extension is lost entirely) is the right one, and every committed assertion comes from a real run, so there is nothing to change in the code. Noting it only so the thread isn't the record.
Verdict: merge
The fix I asked for is in, in the right shape, and it holds up against a 2356-input byte-exact differential with zero regressions: 173 crashes and 565 silently wrong answers on master become correct, and nothing that worked before stopped working.
Two functions in
Pathmixed a byte index with a character slice. Both are fixed here.Path.absolute?aborted the process on a path that is a single UTF-8 continuation byte:where the probe is
(Path.absolute? &(String.from-bytes &[187b])).Path.split-extensionhad the same mismatch, and it fails both ways: it aborts on the same class of input, and it silently returns the wrong extension for any filename with a non-ASCII character before the dot.Why
absolute?was(String.starts-with? p "/"). Core'sstarts-with?guards with(length s)—strlen, so bytes — and then compares withprefix, which slices characters throughString.chars.charssizes its array withutf8len, which counts only non-continuation bytes. A one-byte string whose byte is in0x80–0xBFtherefore has byte length 1 and zero characters, the guard passes, andArray.prefixruns off the end of a zero-length array.split-extensioncut withPattern.find's result, which isMatchResult.startfrom the C matcher — a byte offset — and then sliced withString.prefix/String.suffix, which are character slices. Every multi-byte character before the dot shifts the byte index one place further past the character index, so the cut lands in the wrong spot; when the shift carries the index past the character count altogether, the slice runs off the end and aborts exactly asabsolute?did.This is not a hypothetical input. POSIX filenames are arbitrary byte strings, so a bare
\xBB—»in Latin-1/Windows-1252 — is a perfectly legal name on disk, and so is»».. Andabsolute?is the predicate the rest of the module routes through:relative?is its negation, andabsolute,normalizeandrelativeall call it, so every one of them aborted on such a path.split-extensionis likewise the one thatextension,has-extension?,is-extension?,drop-extensionandreplace-extensionare all built on.Measured on the branch before this commit:
The fix
A leading
/is always a whole character, soabsolute?'s answer only ever depended on the first byte. Reading that byte directly sidesteps the character slice entirely:String.headischar-at s 0, which is a raw byte read. The empty-string guard is written out rather than leaning on the NUL terminator making the read incidentally safe.split-extensiongets its index from a byte matcher, so it now cuts bytes:byte-sliceis a rawmemcpy, andicomes from a successful match, so it is in[0, len)and needs no extra guard. Core's ownString.split-bycuts the same way.matches?(path.carp:425) goes throughString.charson both arguments, so it is self-consistent — a lone continuation byte matches*but not?. It does not abort, and glob semantics over invalid UTF-8 are arguable, so it is left alone. Thewindows-onlyabsolute?matches a drive-letterPattern— a different code path, not reachable from here, and not testable on this machine — likewise untouched. Those two are the only other places in the module where a byte quantity meets a character one;normalize,relative,splitandsplit-search-pathare byte-based end to end.Verification
Array_unsafe_nth__Charassertion with exit -6 on(String.from-bytes &[187b 187b 46b]).path.carpalone: the three accented ones fail (120 passed / 3 failed), and the continuation-byte one takes the whole suite down with exit 134 (SIGABRT).carp -x test/path.carpis 124 passed / 0 failed.split-extensionover 31 ASCII inputs —"",".","..","file.",".hidden","a.b.c","file.txt/veit.ext","x...","/usr/local/lib/libc.so.6"among them — reports 0 mismatches: the ASCII semantics are untouched.split-extensionwas then checked against an independent oracle — Python'sre.search(rb'\.[^/.]*$', p)on the raw bytes — over 24 UTF-8 inputs ("ä.txt","日本語/ファイル.md","naïve.tar.gz","emoji😀.png","ß.ß.ß", …): 0 mismatches.»».,»»».x, a bare lead byte<0xC3>.txt,<0x80>/a.c, a trailing lead byte) shows no abort and byte-exact output on both sides of the split.absolute?'s 21 inputs still reports 0 mismatches.carp-fmt -candanglerare clean on both changed files;carp -x gendocs.carpproduces no diff (no doc string changed — the documented ASCII examples still hold).path.carpatrelative(lines 199–235). Merging it into this branch is clean, and the merged tree is 131 passed / 0 failed.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.