Slice headers, redirect targets and cookie attributes by byte - #16
Conversation
Core's index-of, index-of-string, index-of-from, char-at and length are byte-based while prefix, suffix and slice count characters, and starts-with? / ends-with? guard on byte length and then slice characters. Every one of those pairings in this package sits on a string the server chose. Measured on unmodified main, end to end through Client.get against a server writing raw response bytes, one case per process: Server: caf<c3><a9> body HELLO-BODY arrived as ELLO-BODY X-Weird: <80 x5> body arrived as 15 bytes of mojibake Set-Cookie Domain=<80> SIGABRT in CookieJar.domain-matches? Isolating the header/body split: a 32-byte response with four continuation bytes in a header value made String.prefix return 44 bytes and String.suffix return 0, because chars/from-chars does not round-trip bytes that are not valid UTF-8; one more continuation byte pushed the byte offset past the character count, which aborts. The RFC 3986 redirect resolver has the same shape throughout, on the Location header: every index it derives comes from a byte-based primitive and every span it cut counted characters. last-index-of-char already returned a byte index by construction and keeps that contract. byte-slice is an unchecked memcpy, so each new call is dominated by a guard that proves its offsets: index-of results are tested for -1 before use, last-index-of-char scans 0..<length, split-pos + 4 is inside the accumulator because the needle matched there, and each drop-bytes sits under the byte-starts-with? that just matched those bytes. The two helpers slice only after their own short-circuiting length comparison. Redirects whose Location is not ASCII still abort, inside URI.parse in the uri that http@0.3.0 carries. That is fixed upstream but not in a release this package can pin, which is also why the byte helpers here are local rather than http's String.byte-starts-with?.
There was a problem hiding this comment.
Build & Tests
Checked out claude/byte-safe-slicing at 4d275b1 and ran the real suite locally on armhf:
bash test/run.sh (two local Python origins) → 89 passed, 0 failed, run.sh exit 0. CI is green
on both OSes and the ubuntu raw log confirms Passed: 89 Failed: 0. Worth stating explicitly since
it has bitten this org before: this repo's Run tests step is not continue-on-error any more —
c707fd6 re-gated it — so the green check really is the tests.
Merge-base is 1a49948, which is current origin/main, so no stale-branch drift.
Two pre-existing No 'prn' function found with type (Fn [TlsStream] String) warnings still print on
every build. Unrelated to this PR; already on the standing list.
Note this is still a draft, so it cannot merge as-is regardless of verdict.
Findings
The guards hold — I checked every one, not just the summary
String.byte-slice is memcpy(ptr, *s + a, b - a) with no bounds check (core/carp_string.h:378),
so the PR body's per-call-site proofs are the load-bearing part of this change. I walked all nine
drop-bytes sites and every byte-slice site against them and they hold. byte-starts-with? and
byte-ends-with? mirror core's starts-with?/ends-with? exactly (core/String.carp:136,141) with
byte-slice swapped for prefix/suffix, so they are identical for ASCII and correct beyond it,
and their and guards short-circuit (core/Macros.carp:118). last-index-of-char does keep its
byte-index contract — it scans 0 ..< String.length with char-at, both byte-based. header-end = split-pos + 4 is in range because index-of-string is strstr. ascii-to-lower is
to-bytes/from-bytes, so the cookie jar's normalisation was already byte-safe and does not undo
the fix.
The sweep is complete: on this branch there is no remaining String.prefix, suffix, slice,
starts-with? or ends-with? anywhere in http-client.carp, src/cookie-jar.carp or
src/multipart.carp. The only hit is a comment.
Mutation battery: both coverage claims are exactly true
A — reverted all of http-client.carp to main, keeping only the three helpers so cookie-jar
still builds: 87 passed, 2 failed. The two failures are precisely the two new header tests. Nothing
else moved. So the entire RFC 3986 resolver — all eleven functions — can regress to character
slicing without a single test noticing. The PR says so ("The redirect resolver has no new end-to-end
test"); this confirms it is exactly, not approximately, true.
B — reverted only src/cookie-jar.carp: exit 134, Array_unsafe_nth__Char: Assertion 'n < a.len' failed, aborting partway through the earlier cookie tests and taking the rest of the run
with it. That is the abort the PR describes, and it is why the five cookie assertions had to be
measured one per process. Confirmed.
The resolver fix is real — here is the evidence the PR is missing
Since no test can distinguish it, I measured it directly instead. I reopened defmodule Client in a
probe to reach the private remove-dot-segments, put a verbatim copy of main's char-sliced
resolver (old-remove-dot-segments, plus main's first-segment-end and remove-last-segment) in
the same binary, and ran one case per process so an abort on one side could not hide the other.
| path | branch (byte) | main (char) |
|---|---|---|
/a/b/../c |
/a/c |
/a/c |
/a/./b |
/a/b |
/a/b |
/x/../y/./z |
/y/z |
/y/z |
/caf<c3><a9>/../b |
/b |
/caf<c3><a9>/b — .. silently fails to remove the segment |
/a/./caf<c3><a9> |
/a/caf<c3><a9> |
SIGABRT |
/<80><80><80>/../b |
/b |
SIGABRT |
/a/<80>/./c |
/a/<80>/c |
SIGABRT |
../caf<c3><a9>/x |
caf<c3><a9>/x |
caf<c3><a9>/x |
(<c3><a9> and <80> are literal bytes in the input, written as hex so nothing here is a raw byte.)
So the resolver change turns three aborts and one silently wrong URL into correct answers, and
leaves all four ASCII cases byte-identical. The A/B is not vacuous — row 4 differs without aborting,
so it can report a difference. This belongs in the PR body: it is the strongest evidence for the
largest part of the diff, and right now that part rests on argument alone.
It does not change the end-to-end story — URI.parse still aborts first on the pinned uri, so
none of this is reachable through Client.get until that pin moves — but it does move the resolver
from "unverified" to "verified correct, untestable in CI for now".
The cookie assertions do reach the code they claim to
Worth confirming rather than assuming, since four of the five expect 0 and would pass for free if
they short-circuited earlier. CookieJar.matching reaches domain-matches? only after the expiry,
secure and Maybe.Nothing-domain guards; the fixtures pass all three, and the positive control
(jar-with-domain "example.com" → 1) proves the whole path can yield a non-zero. So the zeros are
real zeros. Their value is the absence of the abort, which mutation B independently demonstrates.
1. Three unqualified names enter the global namespace
byte-starts-with?, byte-ends-with? and drop-bytes are the only top-level non-module defns
in http-client.carp — everything else lives in Connection, RequestConfig, ResponseStream or
Client, and every module-internal helper carries both (hidden x) and (private x). These carry
only (hidden x), which hides them from docs but not from consumers, so anyone who loads
http-client now gets three new unqualified globals. Given web already silently shadows three of
http's Response functions, this is the same class of hazard.
I am not asking you to move them here — where these live is the cross-repo call already sitting
on your list, and four private copies across web, http, uri and this repo is the actual
problem. Flagging it only so the choice is deliberate.
2. drop-bytes is the one helper that does not guard itself
(defn drop-bytes [s n] (String.byte-slice s n (String.length s)))
n > String.length s gives byte-slice a negative length, i.e. CARP_MALLOC(negative + 1) followed
by a memcpy of the same. Its two siblings guard themselves; this one relies entirely on its callers.
All nine in-repo callers are correctly guarded, so there is no live bug — but it is exported
unqualified (see above), so a library user can reach it. If you want it airtight for one line:
(defn drop-bytes [s n]
(let [l (String.length s)] (String.byte-slice s (Int.min n l) l)))
Optional, and it costs nothing at the call sites that are already proven.
Verdict: merge
The change is correct, the guards are sound, the sweep is complete, and the suite is a genuine 89/0
locally as well as in CI. I could not break it: mutation confirms both of the PR's own coverage
claims verbatim, and the resolver — the one part no test covers — I verified directly, where it
fixes three aborts and one silently wrong URL with zero change to ASCII behaviour. The PR is unusually
honest about what it could not close, and that assessment stands up.
Two caveats that are yours, not defects: it is still a draft, and the three helpers land in the
global namespace. Adding the resolver A/B above to the PR body would be worth doing before it lands.
Every string this library slices comes off the wire. Core's
String.index-of,index-of-string,index-of-from,char-atandlengthare byte-based, whileString.prefix,suffixandslicecount characters, andstarts-with?/ends-with?guard on byte length and then slice characters. Wherever a byteoffset met a character-indexed cut, the client either lost body bytes or died.
This is the same fix shape as http #31, uri #33, time #23 and web #52.
Measured on unmodified main
End-to-end through
Client.getagainst a local server that writes raw responsebytes, one case per process (an abort would otherwise mask the rest):
HELLO-BODY(10 bytes)Server: caf\xc3\xa9ELLO-BODY(9 bytes)HELLO-BODY(10)X-Weird: \x80\x80\x80\x80\x80HELLO-BODY(10)Domain=\x80Domain=example.comPath=\x80So a single two-byte UTF-8 sequence anywhere in the headers silently ate the
first byte of every body; five continuation bytes replaced the body entirely.
Isolating the split itself (
String.index-of-string+String.prefix/suffixon the raw accumulator, no client involved):
split-pos30,header-end34, header text 35 bytes, body 9 bytes — one body byte crossedinto the header text.
String.prefixreturned 44bytes and
String.suffixreturned 0 — the whole body gone. The growthis
chars/from-charsfailing to round-trip bytes that are not valid UTF-8,not a read past the buffer.
String.prefixaborts —Array_unsafe_nth__Char: Assertion 'n < a.len' failed, byte offset 29 against28 characters.
Five hostile cases that still abort, and why
A redirect whose
Locationis not ASCII, and any request URL that is not ASCII,still abort on this branch. They do not abort in this repo's code. Measured
against
http@0.3.0alone, one case per process:URI.parse "http://127.0.0.1:8799/x"— fineURI.parsewithcaf\xc3\xa9in the path, in the host, or a lone\x80inthe path — SIGABRT, all three
Response.parseof a 302 carryingLocation: /caf\xc3\xa9/xorLocation: \x80— fine, both parse as 302That is uri's own byte/character mismatch, fixed upstream in "Extract URI
components with byte indices, not character indices" but not in any release this
package can pin yet (
http@0.3.0and@0.4.0both carry the olduri, andneither has
String.byte-starts-with?, which is why the helpers below arelocal). The resolver fix here is necessary but not sufficient for those cases
until the pins move; I left it in rather than shipping a half-byte-safe
resolver.
Tally over the 11-case corpus: 3 fixed, 3 unchanged and correct, 5 unchanged and
broken upstream, 0 newly differing.
What changed
Three local helpers over
String.byte-slice(byte-starts-with?,byte-ends-with?,drop-bytes), and then:Client.read-headerscuts the header/body boundary withbyte-slice.split-query,strip-fragment,path-after-authority,remove-last-segment,first-segment-end,remove-dot-segments,merge-paths,resolve-authority-ref,resolve-path-ref,resolve-location— takes every span by byte offset andtests every prefix bytewise.
last-index-of-charalready returned a byteindex by construction and keeps that contract.
CookieJar.domain-matches?/path-matches?/matchingcompare theserver's
Domain=andPath=bytewise. The.test on line 31 was aone-character needle, which a single continuation byte defeats.
String.byte-sliceis an unchecked memcpy, so each call needs its offsetsproven in range:
and-guarded length comparison,which short-circuits
read-headers:split-pos >= 0means the four needle bytes are insideacc,so
header-end = split-pos + 4 <= length accsplit-query,strip-fragment,path-after-authority,resolve-authority-ref: every index comes fromindex-of/index-of-from,and the
< 0case takes the other branch, so a used index is a real positionin the string
remove-last-segment,merge-paths:last-index-of-charscans0 ..< length s, so a non-negative result is< length sandidx + 1 <= length sremove-dot-segments: eachdrop-bytessits under thebyte-starts-with?that just proved those bytes are present;
first-segment-endreturns eitherlength inputor anindex-of-fromhit inside itresolve-authority-ref'sdrop-bytes r 2is reached only through(byte-starts-with? location "//")drop-bytes cd 1is reached only through(byte-starts-with? cd ".")Tests
bash test/run.sh: 89 passed, 0 failed (82 on main, 7 new). Two newendpoints in
test/server.pywrite raw bytes so the exact header encodingreaches the client.
Teeth, with the new tests in place and both source files reverted to main. Of
the seven new assertions, three fail on main and four are regression guards that
already held:
a multi-byte UTF-8 header value does not eat the front of the body— fails,got
ELLO-BODYa header value that is not valid UTF-8 leaves the body intact— fails, got15 bytes of mojibake
a Domain that is not valid UTF-8 does not match— aborts the whole runwith
Array_unsafe_nth__Char: Assertion 'n < a.len' failed, so the suitenever reaches the assertions after it
Because that abort masks the rest, the five cookie assertions were also measured
one per process against verbatim copies of main's
domain-matches?andpath-matches?: the plain-domain control matches,Domain=\x80aborts, andDomain=.\x80\x80\x80,Path=\x80andPath=/\x80\x80\x80/already answeredfalse. Those last three are guards, not evidence.
The redirect resolver has no new end-to-end test: every input that would
distinguish it has to travel through
URI.parse, which aborts first on thepinned
uri. It is covered by the existing redirect tests for the ASCII case.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.