Harden buffer growth against overflow and failed reallocation

[carpentry-agent]

Every buffer-growth site in src/bufio.h computed the new capacity in int and stored the
CARP_REALLOC result unconditionally. bufio exists to wrap sockets, so the amount of data driving
that growth is remote input.

bufreader_fill is the clearest case — two defects in four lines:

  if (br->rbuf_len >= br->rbuf_cap) {
    br->rbuf_cap *= 2;
    br->rbuf = CARP_REALLOC(br->rbuf, br->rbuf_cap);
  }
  int space = br->rbuf_cap - br->rbuf_len;
  int n = br->read_fn(br->inner, br->rbuf + br->rbuf_len, space);

rbuf_cap *= 2 is signed overflow at 2^30 (UB, negative in practice), and on a failed realloc
rbuf becomes NULL, the old block leaks, and the next line hands NULL + rbuf_len plus a
possibly negative space to read_fn — for a socket that is recv(fd, garbage, (size_t)negative).

Sites changed

site	before
bufreader_fill	rbuf_cap *= 2, unchecked realloc
BufReader_read_MINUS_append_ (both branches)	int new_cap = (buf->len + avail) * 2 with buf->len a size_t, narrowed into an int, then widened back into buf->capacity; unchecked realloc
BufReader_write_	wbuf_cap = (wbuf_len + len) * 2, where wbuf_len + len can overflow before the * 2; unchecked realloc
BufReader_write_MINUS_bytes_	same, plus data->len (Array's size_t) mixed into int arithmetic, so the multiply happens unsigned and the result is narrowed into wbuf_cap

What replaces them

Three static helpers, following the shape of carpentry-org/sockets#10 ("safe CARP_REALLOC and
size_t capacity in read-append functions", merged), which made the same change to that library's
read-append functions:

• bufio_next_cap(have, need) — pure size_t arithmetic, doubles have, meets need when that
  is bigger, clamps at BUFIO_MAX_CAP (INT_MAX, the width of the int cap fields), and returns
  0 when need is past the maximum. No expression in it can wrap.
• bufio_reserve(&buf, &cap, used, extra) for the two char* buffers and
  bufio_reserve_array(buf, extra) for the Carp Array. Both assign the CARP_REALLOC result to
  a temporary and commit it only when it is non-NULL, so a failed allocation leaves the BufReader
  (or the caller's Array) exactly as it was rather than NULL-pointered.

Failure reporting. No API change. bufreader_fill returns -1, which is the <= 0 every
caller already handles as "no progress". For the two write functions I chose -1 as well: it is
the same sentinel BufReader_read_MINUS_append_ and BufReader_flush_ already use, and the Carp
wrappers already translate -1 to Result.Error for both of those. write/write-bytes still
return the raw Int, so their doc strings now say that -1 means nothing was buffered.

Tests — what is live and what is not

carp -x test/bufio.carp: 23 passed, 0 failed (17 on main; no existing assertion was
edited).

Live, and new coverage the change makes possible — bufio_next_cap is a pure function, so the
boundary can be checked directly without allocating anything:

• doubles (8192 → 16384), and meets a request larger than double
• clamps at INT_MAX instead of overflowing, for have = 2^30 — the exact step the old
  rbuf_cap *= 2 wrapped through
• refuses a request past the maximum, returning 0

Those last two have teeth: with the clamp and the max check removed the helper returns
2147483648 for both, and both assertions fail. That value is precisely what used to land in the
int cap field as a negative number.

Live, and closing a real gap — main has no assertion with a payload above BUFIO_DEFAULT_CAP
(8192), so the growth path itself was never executed by the suite:

• a 30000-byte delimiter-free read returns every byte in order (three growth doublings)
• a 30000-byte write is buffered and flushed whole

Both would also pass on main; they are regression guards for the rewritten arithmetic rather
than proof of a fixed bug.

Not tested, and argued for rather than demonstrated: the overflow itself needs a 2 GB buffer
and the realloc-failure branch needs an allocator hook. Neither is reachable from the suite, and I
did not write an assertion that pretends otherwise.

CI

Lint is red on main for two pre-existing unused-let-binding findings in test/bufio.carp
that #2 fixes, and its failure skips the two steps after it. This branch does not touch them. Run
locally so the skipped steps are known clean:

• angler on all .carp files — the same two pre-existing findings, nothing new
• carp-fmt --check on all .carp files — clean
• carp -x gendocs.carp — clean; the regenerated docs/BufReader.html is included

Overlap with the other open PRs

Checked with git merge-tree --write-tree against both:

• Keep the discarded read-line calls, drop the unused names (angler unused-let-binding) #2 (claude/lint-unused-let-binding) — merges clean.
• Resume a failed flush instead of re-sending the accepted prefix #3 (claude/flush-partial-write) — src/bufio.h, bufio.carp and docs/BufReader.html
  all auto-merge (Resume a failed flush instead of re-sending the accepted prefix #3's only C hunk is in BufReader_flush_, which this branch does not touch).
  test/bufio.carp conflicts: both branches append assertions to the end of the same
  deftest. It is a textual conflict at the tail of the file, not a semantic one — whichever
  lands second keeps both blocks.

BufReader_copy is untouched, as it is already listed as a follow-up on #3.

Follow-up, not implemented here

The growth policy is still unbounded: a stream that never sends the delimiter makes
read-until grow the read buffer until it hits BUFIO_MAX_CAP (now a clean read failure instead
of UB, but still 2 GB of remote-driven allocation first). Capping that is an API decision — whether
read-line gains an error, whether there is a configurable maximum — so it belongs to you.

---

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

[carpentry-reviewer]

Build & Tests

carp -x test/bufio.carp on this armhf Pi — 23 passed, 0 failed, exit 0 (was 17/0). In CI the Run tests step passes on both runners with the same 23/0; the job is red only at the Lint step.

I checked the known-red disclosure rather than taking it on trust, and it holds:

• main's test/bufio.carp carries both bindings itself — first-line at :27 and :145. This branch adds one line above them, which is the whole of the :28 / :146 shift; it does not touch either binding.
• main's last CI run was 2026-06-19 and it was green. angler's unused-let-binding rule postdates it. So main is stale-green, not clean — it would be red today for the same two findings.
• PR #2 changes exactly those two lines (first-line → _first-line) and is green on both runners.

So there is no code work owed on this branch for the red gate.

ASan. Rebuilt the suite's generated C with -fsanitize=address and ran with detect_leaks=1: 23/0 and no ASan report at all. The 30000-byte delimiter-free read (three doublings) and the 30000-byte write drive the new bufio_reserve / bufio_reserve_array through real allocations, so this is coverage of the paths that are live, not just of the pure helper.

Mutation-tested the new boundary assertions rather than accepting "teeth-checked":

mutation to src/bufio.h	result
drop the BUFIO_MAX_CAP clamp (next = have * 2)	the next capacity clamps at the maximum fails — 22/1
drop if (need > BUFIO_MAX_CAP) return 0;	a request past the maximum is refused fails — 22/1
drop if (!grown) return -1; in bufio_reserve	suite still 23/0

The third one is the useful one: it confirms your own statement that the failed-realloc guard is not reachable from the suite. Saying so outright, and calling the two 30000-byte assertions regression guards rather than proof, is the right call — the PR would have been weaker if it had implied coverage it does not have.

Findings

1. bufio_reserve does not range-check used, but bufio_reserve_array does check buf->len (src/bufio.h:68, :80) — hardening, not reachable today

static int bufio_reserve(char** buf, int* cap, size_t used, size_t extra) {
  if (extra > BUFIO_MAX_CAP - used) return -1;

static int bufio_reserve_array(Array* buf, size_t extra) {
  if (buf->len > BUFIO_MAX_CAP || extra > BUFIO_MAX_CAP - buf->len) return -1;

BUFIO_MAX_CAP - used wraps if used > BUFIO_MAX_CAP, and the guard then passes for any extra. It is safe today because both call sites pass (size_t) of an int field that this PR keeps in [0, INT_MAX] — which is exactly the property the array version declines to assume about its own input. Since the two helpers sit four lines apart and guard asymmetrically, the reader has to reconstruct that invariant to tell whether the first one is right. if (used > BUFIO_MAX_CAP) return -1; closes it and makes the pair read the same way.

2. bufio_next_cap overloads 0 as "refused", and both callers read it as success (src/bufio.h:59)

bufio_next_cap returns 0 for a request past the maximum, but both wrappers then do if (next <= *cap) return 0; — i.e. 0 is indistinguishable from "already big enough, nothing to do", and the caller proceeds to memcpy into the ungrown buffer. Again not reachable today, because both wrappers pre-check the range before calling. It is worth tightening now rather than later because this PR promotes bufio_next_cap from an internal detail to something registered and directly asserted on from the suite — the sentinel is a contract now, and 0 is a legal capacity.

3. bufreader_fill's new -1 is folded into EOF by every caller (src/bufio.h:98)

Both readers test if (r <= 0): BufReader_read_MINUS_until_ returns whatever is buffered (or an empty string), BufReader_read_MINUS_n_ breaks out of its loop. So a growth failure reaches Carp either as Result.Error "connection closed" or — when data is already buffered — as a Result.Success holding a partial, delimiter-less string, indistinguishable from a genuine short read at EOF.

That is a large improvement on the old behaviour (NULL + rbuf_len handed to read_fn), and read-until already conflates EOF and error, so I would not restructure for it. But the PR's framing is that the buffer can now fail cleanly, and on this path it fails silently — worth a sentence in the docstring at least.

4. write / write-bytes expose -1 as a bare Int while every sibling wraps failure in a Result (bufio.carp:107, :113)

read-append maps -1 to Result.Error, flush maps -1 to Result.Error, but write and write-bytes hand the sentinel to the caller. The suite's own new assertion shows the shape this invites:

(ignore (BufReader.write &br &payload))
(ignore (BufReader.flush &br))

A caller written that way now loses data silently: the write is refused, nothing is buffered, and flush returns Success having sent a short payload. Wrapping these two in Result like their siblings is a breaking change, so this is a maintainer call rather than something I would ask you to change — but the docstring is currently the only thing standing between a failed write and a silent truncation, and the library's own test does not read it.

5. Adjacent and out of scope: read-n segfaults on a negative length (src/bufio.h:146, pre-existing)

Not this PR's — BufReader_read_MINUS_n_ is a malloc site, not a growth site, and the diff does not touch it. Flagging it because it is the same class of defect in the same file and it reproduces:

A: n = 0             -> clean Result.Error
B: n = -1            -> SIGSEGV (rc 139)
C: n = 2000000000    -> Result.Success  (the 2 GB malloc succeeded here; Linux overcommit)

n goes unvalidated into CARP_MALLOC(n) — plain malloc with no NULL check in the default build (carp_memory.h:60) — and is then compared against result.len, a size_t, so a negative n promotes to a huge unsigned value, the loop runs, and take goes negative into memcpy. read-n's length is precisely the kind of value that comes off the wire (a parsed Content-Length), which is the same argument the PR makes for the growth sites. A one-line if (n <= 0) guard plus a NULL check would be a clean small follow-up.

Verdict: revise

The C is correct on everything I could reach: clean under ASan on the live growth paths, and the two boundary assertions genuinely fail when I remove the code they pin. The reason for revise is the gate, not the code — CI is red on Lint on both runners, and I cannot recommend merging a red PR even though I verified the two findings belong to main and not to this branch. The clean unblock is to land #2 first (two characters, green, touches nothing else) and re-run this one; folding #2's change in here instead would only collide with it. Findings 1 and 2 are cheap hardening worth taking while the branch is open; 3 and 4 are a docstring sentence and a maintainer call; 5 is a follow-up, not yours.