Update uri, time and strbuf to their latest releases - #33
Conversation
uri 0.1.0 -> 0.5.0, time 0.5.0 -> 0.5.1, strbuf 0.1.0 -> 0.2.1. base64.carp is already on its latest tag, 0.2.0. Two behaviours change with the newer uri. uri 0.1.0's URI.str prepended an unconditional slash, so a URI with no path serialised as "/" and Request.str got a valid origin-form target by accident. uri 0.5.0 emits the slash only for a path that needs one, so the same URI serialises as "" and the request line becomes "GET HTTP/1.1", which is not a valid request line and does not re-parse. Request.target now applies RFC 9112 3.2.1 -- an origin-form target whose path component is empty is sent as "/" -- instead of relying on the URI serialiser. uri 0.5.0's URI.escape emits uppercase percent-encoding as RFC 3986 2.1 requires, so Form.encode now yields %2B rather than %2b. The expectation in the test is updated to match.
There was a problem hiding this comment.
Build & Tests
Checked out claude/bump-uri-time-strbuf at 4fde0c46. carp -x test/http.carp → 388 passed,
0 failed, exit 0 (read unpiped). CI green on both OSes with head_sha = 4fde0c46…, so the green
is this commit. Base master, merge-base 31ca711 = current origin/master, no stale-branch drift.
No CHANGELOG.md in the repo — confirmed, so the PR body really is the only record this change gets.
The new guard is load-bearing, and the new tests are precisely aimed
I reverted http.carp:331 alone, back to &(URI.str (uri r)), keeping uri 0.5.0 — i.e. the exact
state the PR says the bump would have left:
GET HTTP/1.1 ← wire output, empty request-target
reparse → ERROR "Malformed request: found first line 'GET HTTP/1.1'"
and the suite goes 385 passed / 3 failed: the two new origin-form tests plus the pre-existing
request roundtrip preserves verb. The third new test (absolute-form request-target is left alone) still passes without the guard, which is right — absolute-form was never affected, so that
test pins the guard against over-triggering rather than padding the count. All five shapes I tried
re-parse cleanly on the branch.
Findings
1. The bump fixes a remotely triggerable abort in Form.parse, and the PR does not mention it
This is the most valuable thing in the change and it is missing from the body. Same driver, http
master vs this branch:
MASTER Form.parse "k=b%4" Array_unsafe_nth__Char: Assertion `n < a.len' failed → exit 134
BRANCH Form.parse "k=b%4" k => "b%4"
BRANCH Form.parse "k=b%" k => "b%"
The cause is in the dependency, at uri 0.1.0 main.carp:256-257: on seeing %, unescape reads
(unsafe-nth cs (inc i)) and (unsafe-nth cs (+ i 2)) with no bounds check at all, so a % in
the last two positions walks off the array. Isolated at the uri level, URI.unescape "a%" aborts
under 0.1.0 and returns "a%" under 0.5.0, which added the (< (+ i 2) n) check plus a hex test on
both nibbles.
Form.parse is http's documented entry point for application/x-www-form-urlencoded bodies
(http.carp:928) — that is attacker-controlled input in any server built on this library. A form
body ending in % kills the process. That turns this PR from dependency hygiene into a fix, and
it belongs in the body where a reader will see it.
2. "One user-visible output change" undercounts it substantially
URI.escape did not merely switch hex case — it moved from codepoint-indexed to byte-indexed,
so Form.encode changes for every non-ASCII value, and the old output was silently lossy:
| input | master | branch |
|---|---|---|
{k: "a+b"} |
k=a%2bb |
k=a%2Bb |
{k: "é"} |
k=%e9 |
k=%C3%A9 |
{k: "€"} |
k=%ac |
k=%E2%82%AC |
{"näme": "v"} |
n%e4me=v |
n%C3%A4me=v |
Master masked each codepoint to its low byte, so U+20AC came out as %ac — the same encoding it
would produce for U+00AC, unrecoverable. Decoding moved too: Form.parse "k=%C3%A9" yielded
[195 131 194 169] (mojibake) on master and [195 169] (a correct é) here, and Form.parse "k=%zz" silently truncated the value to empty on master where the branch keeps %zz verbatim.
All of these are improvements. The point is that "a literal + now encodes as %2B" is the
smallest of them, and anyone with stored form encodings is affected by far more than a case change.
3. The http://example.com row says "unchanged", but the request line did change
The PR's table records that shape as unchanged. That is true of URI.str under uri 0.5.0; it is not
true of what http emits. Measured Request.str:
MASTER GET http://example.com/ HTTP/1.1
BRANCH GET http://example.com HTTP/1.1
The trailing slash is gone. This is wire-visible rather than internal, because http-client's
build-and-send hands the whole parsed URI straight to Request.str (http-client.carp:272), so
absolute-form is what actually goes out.
I do not think it is a bug: RFC 3986 §4.3 permits an empty path in an absolute-URI and RFC 9112
§3.2.2 requires servers to accept absolute-form, and the output re-parses fine. But a PR whose
stated method is "treat every behaviour difference as a finding" should record it as a difference
rather than as "unchanged". If you would rather keep the slash, the fix belongs inside the URI
(set an empty path to / when an authority is present), not as a prefix on the serialised string —
prepending would produce /http://example.com.
Worth noting alongside: http-client pins http@0.3.0, so none of §1–§3 reaches it until you cut a
release and bump.
4. The guard itself is right
Prepending only when path, scheme, host, user and port are all Nothing fires exactly on
origin-form, which is what RFC 9112 §3.2.1 is about. Query-only gives /?a=1; a relative path a
gives /a (uri supplies that slash itself, since the path is Just); absolute-form passes through.
The one degenerate shape, a host with no scheme, yields //example.com — not a valid request
target, but master produced example.com/ for the same input, equally invalid, so nothing regressed.
Pre-existing and untouched, but target is now the natural place for it: Request.str still puts a
fragment in the request-target (GET /#top HTTP/1.1), which RFC 9112 §3.2.1 forbids. Identical on
both sides, so not this PR's doing.
Verdict: revise
No code change needed — I could not break it. The bump is right, the guard is load-bearing and
correctly scoped, the tests fail for the right reasons when it is removed, 388/0 reproduces, and the
ASan reasoning holds up. What needs revising is the record: this repo has no changelog, so the PR
body is the release note, and it currently omits the crash fix (§1) and most of the encoding
change (§2) while describing as "unchanged" a request line that changed (§3). Fold those in and this
is ready.
uri 0.1.0's URI.unescape read one and two chars past the array on seeing a %, so a form body ending in a truncated escape aborted the process: Form.parse "k=b%4" and "k=b%" both exit 134 under the old pin. uri 0.5.0 bounds-checks and hex-tests both nibbles. The same bump moved escaping from codepoint-indexed to byte-indexed, so non-ASCII values now survive a Form.encode/Form.parse roundtrip; uri 0.1.0 masked each codepoint to its low byte and lost U+20AC as %ac. All four fail under the old dependency: the two encoding tests as plain mismatches, the two truncated-escape tests by killing the process.
|
Worked all three review findings. Everything below was re-measured here rather than copied from the review — §1 — the §2 — "one user-visible output change" undercounted. Confirmed and expanded, with one correction to how it is tested. Measured The correction: master's roundtrip survives U+0080–U+00FF, because §3 — the New tests ( Teeth were proven, not assumed: this branch's Counts, both measured, unpiped: No code changed — as the review said, there was nothing to fix. ASan was not re-run for the four new tests; they are plain |
hellerve
left a comment
There was a problem hiding this comment.
Verified locally: 392/0 on the branch, 385/0 on master. Bumping the three pins alone on master fails exactly two tests, so Request.target is a required repair rather than scope creep. Reproduced both claimed defects on published 0.4.0 one case per process — Form.parse "k=b%4" aborts at exit 134, and € encodes to k=%ac — both correct after the bump. Repinned to uri 0.5.1 and time 0.5.2, cut just now, so the release carries the byte-consistency fixes rather than trailing them; still 392/0.
Brings the three stale intra-org pins up to their newest released tags, checked against
gh api repos/carpentry-org/<repo>/tagsat the time of writing:uritimestrbufbase64.carpTests: 385 passed / 0 failed → 392 passed / 0 failed, exit 0.
httphas noCHANGELOG.md(verified — no changelog file exists in the repo), so this body is the release note. Everything below was measured by buildingmaster'shttp.carpand this branch'shttp.carpinto the same driver, one case per process, reading each exit code directly rather than through a pipe.The bump fixes a remotely triggerable process abort in
Form.parseuri0.1.0'sURI.unescape(main.carp:256-257) reads(unsafe-nth cs (inc i))and(unsafe-nth cs (+ i 2))on seeing a%, with no bounds check, so a%in the last two bytes walks off the array.uri0.5.0 guards with(< (+ i 2) n)and hex-tests both nibbles before decoding.Form.parse(http.carp:934) is http's documented entry point forapplication/x-www-form-urlencodedbodies, so this is reachable from a request body in any server built on this library — a form body ending in%kills the process.masterForm.parse "k=b%4"Array_unsafe_nth__Char: Assertion 'n < a.len' failed, exit 134k => "b%4"Form.parse "k=b%"k => "b%"URI.unescape "a%""a%"A real bug the bump exposed: empty request-target
uri0.1.0'sURI.strconcatenated an unconditional@"/"before the path, so(URI.str (URI.zero))was"/".uri0.5.0 emits that slash only when there is a path that does not already start with one, so the same call now returns""— andRequest.strserialisedGET HTTP/1.1, an empty request-target, which is not a valid request line and does not re-parse.http had been getting its origin-form slash by accident, from a quirk of the URI serialiser.
Request.targetnow guarantees it directly: RFC 9112 §3.2.1 says a client sending origin-form MUST send/as the path when the target URI's path component is empty, so http applies that itself rather than inheriting it.The guard is structural (no string sniffing): the slash is prepended only when the URI has no path and no scheme, host, user or port — i.e. only for origin-form. Absolute-form targets are passed through untouched.
URI.strunder 0.5.0URI.zero""/?a=1)?a=1/?a=1/foo/foo/foohttp://example.comhttp://example.comhttp://example.com(see below)Behaviour differences a consumer can see
1. The absolute-form request line loses a trailing slash
An earlier revision of this body recorded
http://example.comas unchanged. That is true ofURI.strin isolation; it is not true of what http puts on the wire, becausehttp-client'sbuild-and-sendhands the whole parsed URI toRequest.str(http-client.carp:273), so absolute-form is what actually goes out. MeasuredRequest.str:masterhttp://example.comGET http://example.com/ HTTP/1.1GET http://example.com HTTP/1.1http://example.com/GET http://example.com/ HTTP/1.1/fooGET /foo HTTP/1.1This is deliberate, not a regression to fix: RFC 3986 §4.3 permits an empty path in an absolute-URI, RFC 9112 §3.2.2 requires servers to accept absolute-form, and the output re-parses cleanly. It is recorded here because it is wire-visible. Restoring the slash would belong inside the URI (empty path →
/when an authority is present), never as a prefix on the serialised string — prepending there would produce/http://example.com.2. Percent-encoding moved from codepoint-indexed to byte-indexed
uri0.5.0'sURI.escapedoes not merely switch hex case. 0.1.0 walked the string as codepoints and masked each one to its low byte ((bit-and i 240)/(bit-and i 15)), so every non-ASCII value came out wrong; 0.5.0 escapes the UTF-8 bytes.Form.encodeoutput therefore changes for every non-ASCII key or value, and the old output was lossy — U+20AC encoded as%ac, indistinguishable from U+00AC.masterForm.encode {k: "a+b"}k=a%2bbk=a%2BbForm.encode {k: "é"}k=%e9k=%C3%A9Form.encode {k: "€"}k=%ack=%E2%82%ACForm.encode {"näme": "v"}n%e4me=vn%C3%A4me=vDecoding moved with it:
masterForm.parse "k=%C3%A9"é— bytes[195 131 194 169]é— bytes[195 169]Form.parse "k=%zz""", value silently truncated%zzkept verbatimForm.parse (Form.encode {k: "€"})¬— bytes[194 172]€— bytes[226 130 172]One nuance worth recording, because it makes the old breakage easy to under-test: master's
Form.encode/Form.parseroundtrip does survive for U+0080–U+00FF, since%e9decodes back to U+00E9.étherefore roundtrips on both sides and pins nothing. U+20AC is where the low-byte mask actually destroys information, which is why the new roundtrip test uses€.+encoding as%2Brather than%2b(RFC 3986 §2.1 requires uppercase) is the smallest of these changes, not the only one. Anyone byte-comparing storedForm.encodeoutput is affected by more than a case change; anyone storing non-ASCII form encodings was storing lossy ones.Tests changed
Seven added, one updated. Nothing else in the suite moved.
request-target of an empty path is /Request.stroutput directly, not just via roundtriprequest-target of a query-only URI keeps its leading /URI.stryields no leading slashabsolute-form request-target is left aloneencode a non-ASCII value one byte at a time€as%E2%82%AC; uri 0.1.0 givesk=%acencode/parse roundtrip a non-ASCII value¬parse a value ending in a truncated percent escapek=b%4; aborts the process under uri 0.1.0parse a value ending in a bare percentk=b%; aborts the process under uri 0.1.0encode literal + as %2b→%2BForm.encodeitself is untouchedrequest roundtrip preserves verbwas the failing test that found the empty-target bug; it passes again unchanged.The four new tests were checked for teeth, not assumed to have them. This branch's
test/http.carpwas built verbatim againstmaster'shttp.carp(i.e. uri 0.1.0). The two encoding tests fail with exactly the values tabled above, and the run then dies at the truncated-escape test:Both truncated-escape shapes abort independently (
k=b%4andk=b%, exit 134 each) — the suite run only reaches the first, so they were also measured one per process.Verification
1. Pins are the newest released tags. Re-checked at run time via the tags API rather than trusting a written-down number. One caveat:
uri0.5.0 does not contain uri #33 (byte-consistent parsing).urimaster is exactly 3 commits ahead of the 0.5.0 tag and those 3 commits are #33, so a further bump to 0.5.1 is still owed once that tag is cut.2. The
use-allleak (uri #32). uri 0.1.0 did(use-all Array Int Maybe Result)at top level, which in Carp leaks into every loading file; uri 0.5.0 scopes it insidedefmodule URI. Confirmed the scoping change is in 0.5.0 and that http does not depend on the leak: outside doc strings,http.carphas no unqualifiedJust/Nothing/Success/Errorand no unqualifiedArray/Intcalls.Worth recording, because it makes the leak harder to observe than expected: I probed for it and found that a Carp file with no
loadat all already resolves bareJust,unsafe-nthandunsafe-from-success. The reason is thatcore/Statistics.carpitself does top-level(use Int),(use Double),(use Array), which leaks org-wide from core. So uri's top-leveluse-allwas largely shadowed by core's own leak, and removing it changes nothing observable for consumers of these four modules — the fix is still right as hygiene (it removes the ambiguity risk for a consumer that defines colliding names), but nobody downstream should expect a behaviour change from it.3. strbuf 0.2.1's automatic cleanup — no double-free, no leak. 0.2.x adds
(implements delete StringBuf.delete), so buffers are now freed automatically at scope exit.deletetakesStringBufby value, so http's 6 existing explicit(StringBuf.delete sb)calls consume the buffer and no second free is inserted;StringBuf.copyis a deep copy, so no aliasing either. All 6StringBuf.createsites pair with a delete on every exit path,dechunkincluded.ASan/LSan was run, not just reasoned about:
carp -b test/http.carp, then the generatedmain.chand-compiled withclang -fsanitize=address -g -O0 -I ~/carp-lang/core. All 388 tests of the previous revision pass under ASan with zero sanitizer errors (no double-free, no use-after-free, no overflow) and the only leak reported is an injected positive control — a deliberately orphaned 4321-byte allocation, which LSan did report, confirming leak detection was actually armed:Zero leaks besides the control, so this is stronger than "no new leak" — the suite leaks nothing at all under the new strbuf. The four tests added since are plain
Formstring assertions and were not re-run under ASan.4. Counts. Both sides measured, exit code read directly rather than through a pipe:
masteris 385 passed / 0 failed, exit 0; this branch is 392 passed / 0 failed, exit 0.carp-fmt -candanglerare clean on both changed files.5. Blast radius.
http-clientpinshttp@0.3.0, so none of the above reaches it until a release is cut and the pin bumped.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.