Read numeric header fields by their grammar, not strtol - #48
Conversation
Int.from-string is C's (int)strtol. It reads an empty string as 0, accepts a sign and leading whitespace, and on LP64 truncates values past Int (4294967296 becomes 0). A shared parse-digits reads 1*DIGIT and saturates at Int.MAX instead. - Response.parse: the status code must be 3DIGIT (RFC 9112 §4). - Set-Cookie Max-Age: an optional "-" then 1*DIGIT (RFC 6265 §5.2.2). An empty or "+"-signed value now gets the existing malformed-value error instead of expiring the cookie or being accepted. - Cache-Control max-age/s-maxage: 1*DIGIT, saturating at Int.MAX (RFC 9111 §1.2.2). Empty or signed values are now Nothing. - q values: a NaN or empty q falls back to the documented default weight of 1, instead of NaN (order-dependent negotiation) or 0.
There was a problem hiding this comment.
Build & Tests
I built and ran test/http.carp at 95256ad with a compat core that has carp-lang #1587's rename, because the local carp predates it. On armhf the branch passes 540/0 and master 516/0. I also compiled the same generated C with aarch64-linux-gnu-gcc and ran it as LP64, which matches CI's data model: 540/0 and 516/0 again. CI is green on ubuntu and macos. That job also runs angler, carp-fmt --check and gendocs.
The PR's "Probes on master" rows reproduce on both data models: -200, 2000, +200 and the doubled space, Max-Age= expiring the cookie, and on LP64 4294967296 giving an expired cookie and Just 0. The q=nan result depends on header order, and gzip;q= refuses gzip.
Mutants. I built once with env-gated mutants and ran the result on armhf and LP64. The baseline is 540/0 on both.
| mutant | failing tests, armhf | LP64 |
|---|---|---|
status site back to Int.from-string |
5 | 5 |
| status site without the length-3 check | 2 (four-digit, two-digit) | 2 |
Max-Age site back to Int.from-string |
2 | 3 |
Max-Age: no - branch |
2 | 2 |
Max-Age: - branch without Int.neg |
2 | 2 |
Max-Age: no trim |
1 | 1 |
Cache-Control site back to Int.from-string |
3 | 6 |
parse-digits: empty input is Just 0 |
2 | 2 |
parse-digits: no saturation guard |
4 | 4 |
parse-digits: >= instead of > in the guard |
survives (0) | survives (0) |
old clamp-q |
2 | 2 |
no empty-q check |
1 | 1 |
The revert rows match the PR's table (5/5, 2/3, 3/6, 2/2, 1/1). Every call-site mutant is killed.
No signed overflow in parse-digits. The guard acc > (Int.MAX - d) / 10 runs before the multiply, so acc * 10 + d cannot overflow; I checked this in the generated C. As a cross-check, UBSan on an LP64 build flags the mutant without the guard (2147483640 + 8 in parse_MINUS_digits) and reports nothing in parse-digits on the branch. Its one other report is in core's String.hash multiply, which predates this PR. A saturated Max-Age=99999999999999999999 gives a sane expiry date (October 2094) on both data models.
Findings
1. HTTP/1.1 200 OK (two spaces) now fails, though every client I measured accepts it (http.carp:586). The PR leaves this call to Veit, so here is the data. I served each status line from a raw socket server, with a unique-body control row:
| status line | master | branch | curl 7.88 | Python 3.11 http.client |
Go 1.26.1 | Chromium 154 |
|---|---|---|---|---|---|---|
HTTP/1.1 200 OK (control) |
200 | 200 | 200 | 200 | 200 | body shown |
HTTP/1.1 200 OK |
200 | error | 200 | 200 | 200 | body shown |
HTTP/1.1 , a tab (0x09), 200 OK |
200 | error | 200 | 200 | error | body shown |
HTTP/1.1 +200 OK |
200 | error | error | 200 | error | body shown |
HTTP/1.1 2000 OK |
2000 | error | error | error | error | body shown |
Chromium's --dump-dom can't show a status code, so its column only says the response wasn't refused. The doubled space is the one row where the branch is stricter than every client here. Response.parse is the response parser behind http-client. An end client has no downstream parser that could disagree with it, so rejecting this row brings no smuggling protection. Once http-client picks up this version, such a server's responses become ClientError.Parse for the whole request.
Suggested fix: skip the run of SPs after the version before reading the code, as Go does (strings.TrimLeft(status, " ") in net/http/response.go), then require exactly three digits. That keeps every other rejection in this PR, and also accepts three spaces, which master rejected only by accident. The tab row can stay rejected, since Go rejects it too. Then the a status code after a doubled space is malformed test flips to expect 200.
2. Max-Age=+60 and Max-Age= now fail the whole response, where master and every client I measured keep it (http.carp:233-242). Response.parse stops at any Cookie.parse-set error (http.carp:627-629) with Malformed response: found header 'malformed max-age value in set-cookie', and http-client returns that as ClientError.Parse. The PR body says these values give "the existing malformed max-age value error", but not that the error now ends the whole parse. The fixture was HTTP/1.1 200 OK with Set-Cookie: a=b; Max-Age=… and a 2-byte body:
| Max-Age value | master | branch | curl 7.88 cookie engine | Python 3.11 cookiejar | Go 1.26.1 Response.Cookies |
|---|---|---|---|---|---|
+60 |
60 s cookie | response fails | 60 s cookie | 60 s cookie | MaxAge=60 |
| empty | cookie expired | response fails | cookie not stored | cookie dropped | attribute ignored |
RFC 6265 §5.2.2 says to ignore the cookie-av when the value does not start with a DIGIT or -. It never calls for rejecting the cookie, let alone the response. Treating a malformed value as an error is the existing design (Max-Age=abc and a bad Expires already fail this way), so I'm not asking to change that.
+60: this is a plain regression. Master and all three clients read it as 60 seconds, and now the request fails. Accept an optional sign,["+" / "-"] 1*DIGIT, and flipa Max-Age with a plus sign is malformed.- Empty value: an error at least matches how
abcis handled. Please say in the PR body that it now fails the whole response, so Veit decides with that in view. One knock-on effect:a=b; Max-Age=; Max-Age=60now fails too, though its last Max-Age is valid. Master gave a 60 s cookie.
3. Minor and optional: the guard's boundary isn't pinned (http.carp:45). The CacheControl.max-age reads Int.MAX exactly test (test/http.carp:2586) uses the one 10-digit value where > and >= agree. With >=, the values 2147483640 to 2147483646 read as Int.MAX, and the suite stays 540/0 on both data models. A max-age=2147483646 test would pin the boundary. The real-world effect is at most 6 seconds over 68 years.
Checked and fine:
max-age="60"is still unquoted to 60 beforeparse-digits. RFC 9111 §5.2 asks recipients to accept the quoted form.- An empty,
-5or+5max-agenow givesNothing, which fails safe. Nothing in http-client, web or llm callsCacheControl.max-age. - The
qchange does what it says. A NaNqnow gives 1 in either order of the ranges, and an emptyqgives 1.-0,inf,2,-1and a quoted"0.5"give the same weights as before. - Status codes
000,099,600and999parse as they did on master.
Verdict: revise
The numeric fixes are correct and well tested: every call-site mutant fails tests, there is no overflow, and the before-values reproduce on armhf and LP64. But two of the new rejections make http-client fail whole requests that master, curl, Python and Go all handle: the doubled space before the status code, and Max-Age=+60. Each has a narrow fix.
A run of SP after the HTTP-version is skipped before the three status digits are read, as Go's ReadResponse does, so `HTTP/1.1 200 OK` parses as 200 again. A tab, a sign, or anything but three digits stays an error. Max-Age now reads `["+" / "-"] 1*DIGIT`, so `Max-Age=+60` gives a 60 s cookie again instead of failing the whole response. A max-age=2147483646 test pins the saturation guard's `>` against `>=`.
|
Follow-up to @carpentry-reviewer's review of 1. Spaces before the status code. 2. 3. Guard boundary. A 4. A malformed Max-Age fails the whole response. This supersedes the PR body on that point. The body says an invalid value gives the existing Checks. The suite passes 543/0 on armhf and as LP64: I rebuilt the generated C with
|
There was a problem hiding this comment.
Build & Tests
Round 2, on 3732688. I built test/http.carp with a compat core that has carp-lang #1587's rename, since the local carp predates it. The suite passes 543/0 on armhf. Compiling the same generated C with aarch64-linux-gnu-gcc and running it as LP64, which is CI's data model, also gives 543/0. The merge with current master c11a31d (#49) is conflict-free, as git merge-tree shows, and passes 543/0 on both data models. CI is green on ubuntu and macos for this head, and that job includes angler, carp-fmt --check and gendocs.
Prior feedback
All four points from the review of 95256ad are addressed. I re-probed each one with Response.parse / Cookie.parse-set against master and the branch, on armhf and LP64. The outputs were identical on both.
| input | master | branch |
|---|---|---|
HTTP/1.1 200 OK (2 SP) |
200 | 200 (was an error at 95256ad) |
HTTP/1.1 200 OK (3 SP) |
error | 200 |
HTTP/1.1, 300 SP, 200 OK |
error | 200 |
HTTP/1.1 , a tab (0x09), 200 OK |
200 | error |
2 SP then +200 / 2000 / 20 |
200 / 2000 / 20 | error / error / error |
HTTP/1.1 (only spaces) |
error | error |
Max-Age=+60 |
60 s cookie | 60 s cookie (live at +30 s, expired at +120 s) |
Max-Age= +60 |
60 s cookie | 60 s cookie |
Max-Age= with +, -, +-5, -+5, ++60 or + 60 |
malformed | malformed |
Max-Age=+0 / -0 |
expired | expired |
Max-Age=+99999999999999999999 |
live | live (saturates) |
- Spaces before the status code.
skip-sp(http.carp:560-567) skips only SP, and is bounded by the length. The code is then sliced fromcode-start(http.carp:584-590), which is Go'sTrimLeft(status, " ")followed by exactly three digits.index-of-fromstarts searching one pastcode-start. That is harmless here, becausecode-startalways sits on a non-space or at the end of the line. A tab stays rejected, as in Go, which is the call I said could stand. Max-Age=+60. It is back to a 60 s cookie, as on master (http.carp:233-239). Every other sign form stays malformed, the same as on master.- Guard boundary.
max-age=2147483646(test/http.carp:2593-2596) now kills the>=mutant that survived in round 1. - Empty
Max-Age. Your reply states the blast radius:a=b; Max-Age=; Max-Age=60fails the whole response. I confirmed it in both orders (Max-Age=+60; Max-Age=andMax-Age=; Max-Age=+60), where master gave an expired cookie and a 60 s cookie respectively. That is now visible for Veit to decide, which was the ask.
Mutants. I re-ran round 1's battery, because the follow-up's test edits could have unpinned a branch that round 1 showed as killed, and added the new sites. Each mutant was applied on its own and run on armhf and LP64. The baseline is 543/0 on both.
| mutant | failing tests, armhf | LP64 |
|---|---|---|
status site back to Int.from-string |
5 | 5 |
| status site without the length-3 check | 2 | 2 |
Max-Age site back to Int.from-string |
1 (round 1: 2) | 2 (round 1: 3) |
Max-Age without the - branch |
2 | 2 |
- branch without Int.neg |
2 | 2 |
Max-Age without trim |
1 | 1 |
Cache-Control site back to Int.from-string |
3 | 6 |
parse-digits reads empty as Just 0 |
2 | 2 |
parse-digits without the saturation guard |
4 | 4 |
>= in the guard |
1 (round 1: survived) | 1 |
old clamp-q |
2 | 2 |
no empty-q check |
1 | 1 |
new: no SP skip (code-start = sp1 + 1) |
2 | 2 |
new: skip-sp also skips tabs |
1 | 1 |
new: Max-Age without the + branch |
1 | 1 |
new: a + read as negative |
1 | 1 |
The four rows in your reply's table (no SP skip, tab skip, no +, >=) reproduce exactly. The status-site revert still fails five tests, but the tab test has replaced the doubled-space test among them. The Max-Age revert fails one test fewer, because strtol also reads +60 as 60, so the flipped test now agrees with master. The empty-value test still kills it.
Findings
No blocking issues. The follow-up is the narrow fix that round 1 asked for, and nothing else changed.
- Minor and optional: the PR body is now stale in three places. Your reply supersedes it, but the body is what a reader sees first.
- The table row for
Max-Agesays a+-signed value gives themalformed max-age valueerror. It now gives a cookie. - The "Calls to check" bullet still says two spaces before the status code are an error. It tells Veit to drop a test (
a status code after a doubled space is malformed) that no longer exists. - "24 new tests take the suite from 516 to 540" is now 27 tests and 543.
- The table row for
- Not this PR, for a follow-up: a status line with no SP after the code is rejected on master and on the branch.
HTTP/1.1 200followed by CRLF givesMalformed response: found first line 'HTTP/1.1 200', becausesp2is -1. Against a raw socket server, curl 7.88, Python 3.11http.client(200 '') and Go 1.26.1 (Status "200") all accept it and return the body. Go trims the SPs and then treats the SP after the code as optional, and Python falls back tosplit(None, 1). RFC 9112 §4 does require that SP, so this is interop leniency, not a bug in this PR.
Verdict: merge
Both round-1 regressions are fixed with the narrow grammar asked for, the guard boundary is pinned, every call-site mutant is killed on both data models, and the suite passes on the branch and merged with master.
Four header fields were read through core's
Int.from-string/Double.from-string, which are C's(int)strtol/strtod. They now follow their ABNF. A new private, hiddenparse-digitsreads1*DIGITand saturates atInt.MAX, in the style ofTransferEncoding.parse-hex. The three integer sites share it.Response.parsestatus code3DIGITMalformed responseerrorCookie.parse-setMax-Age["-"] 1*DIGIT+-signed gives the existingmalformed max-age valueerror; overflow saturatesCacheControl.max-age/s-maxage1*DIGITNothing; overflow saturates atInt.MAXWeightedq (Accept*)q=nanandq=fall back to the documented default weight 1Probes on master
HTTP/1.1 -200 OKparses as status -200.2000parses as 2000,20as 20, and+200as 200.HTTP/1.1 200 OK(two spaces) parses as 200, not 0:index-of-fromstarts searching afterfrom, so the code string is" 200", and strtol skips the space.Max-Age=expires the cookie at once.Max-Age=+60is accepted.Max-Age=abcalready givesmalformed max-age value in set-cookie. On LP64,(int)truncation turnsMax-Age=4294967296into 0,2147483648into -2147483648 and99999999999999999999into -1, and all three expire the cookie. This Pi is armhf: its 32-bitlongmakes strtol saturate, so it shows these as live.max-age=givesJust 0,max-age=-5givesJust -5andmax-age=+5givesJust 5. On LP64,max-age=4294967296givesJust 0.gzip;q=nangivesWeighted.q= NaN, outside the documented[0.0, 1.0].Accept: text/html;q=nan, text/html;q=0.5makestext/htmlunacceptable, but with the two ranges swapped it is chosen: no comparison can replace a NaN weight, and NaN then fails theq > 0check.Accept-Encoding: gzip;q=refuses gzip, because strtod reads""as 0.0.Calls to check
Int.MAX(2147483647), the largest value Carp's 32-bitIntcan hold, which RFC 9111 §1.2.2 allows.a status code after a doubled space is malformedtest and trimcode-str.Max-Age= 60(space after=) still parses, because the value is trimmed first. Master accepted it via strtol, and RFC 6265 §5.2 strips whitespace around attribute values.inf,-inf,0x1p-1,1e-1and+0.5. After the existing clamp, each gives a weight in[0, 1]that matches its numeric value, so none produced a wrong ordering. A strict qvalue reader would also sendq=-1(now 0) andq=0.5000(now 0.5) to weight 1, which goes against the clamp. Two new tests pin the clamp:q=2gives 1 andq=-1gives 0.Tests
24 new tests take the suite from 516 to 540, all passing. Accepted forms are pinned as well as rejected ones: leading zeros (
Max-Age=0060,max-age=007),Max-Age= 60,max-age=2147483647exactly, and the saturated overflow values.max-age=0andMax-Age=0were already covered.I reverted each fix on its own, keeping the others, and re-ran the suite. The Pi is armhf, so I also compiled the same generated C with
aarch64-linux-gnu-gccand ran it natively as LP64, like CI's ubuntu and macos runners:3DIGITMax-Ageclamp-q)The overflow tests can only fail on LP64, because 32-bit strtol already saturates.
carp-fmt -candanglerare clean on both changed files, andgendocs.carpruns.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.