Reject a count below one in read-n instead of crashing

[carpentry-agent]

Fixes #5.

The bug

BufReader_read_MINUS_n_ allocated CARP_MALLOC(n) without checking the
argument or the result, then looped on result.len < n. result.len is
size_t, so the comparison promotes n: a negative count becomes a huge
bound and the loop memcpys into a buffer that was never allocated. It is
reachable from one line of ordinary Carp, (BufReader.read-n &br -1).

Measured on main (265b398) before touching anything, with a mock stream
holding "hello world":

n	before
-1	[RUNTIME ERROR] exited with return value -11
0	Error "connection closed"
5	hello
100	hello world

Under -fsanitize=address,undefined the same call reports
AddressSanitizer: requested allocation size 0xffffffff at src/bufio.h:164
— size_t is 32 bits on this box, so (size_t)-1 is 0xffffffff here.

The fix

In src/bufio.h, in the shape bufio_reserve/bufio_reserve_array already
use:

• return an empty array for a non-positive n, before any allocation;
• NULL-check CARP_MALLOC and report BUFIO_ERR when it fails — the same
  defect with a rarer trigger, and the half the issue calls out separately;
• compare the loop bound against (size_t)n, now that n is known positive.

The positive path is otherwise untouched.

Why read-n grew a third error message

The issue leaves the choice between erroring and clamping open, and an empty
array would already fall through the existing wrapper to
Error "connection closed" with no Carp-side change at all. I did not take
that, because it would be an accident rather than an answer: an n below 1 is
a caller mistake, not a stream condition, and the stream is neither closed nor
even touched. This library's docs go out of their way to separate a failed
read from a closed one, so the wrapper now rejects n < 1 up front with
Error "invalid count".

n = 0 already returned an Error before this change (see the table), so
only its message moves; n < 0 moves from a crash to that same error. Adding
a status value to the C protocol PR #10 just introduced would have cost more
and bought nothing — the wrapper has n in hand and needs no help from C to
know the count is bad.

Tests

Five cases in test/bufio.carp, beside the existing read-n empty-stream
test: -1 and 0 are rejected, a rejected count leaves the stream where it
was (the next read-n 5 still yields hello), and the C entry point itself
reads nothing for -1 and still reads 5 bytes for 5. That last pair goes
through a mock_read_n_raw shim, in the same spirit as the existing
bufio_next_cap_long: the Carp guard now shadows the C guard, so without a
shim the actual subject of the issue would ship untested. Reverting only the
C hunk crashes the suite, which is what that shim is there to catch.

Controls kept byte-for-byte: read-n 5 → hello, the short read at EOF, both
read-error cases, the empty stream, and the read past the default capacity.
44 tests pass, and the whole suite is clean under -fsanitize=address,undefined.
carp-fmt --check and angler pass; docs/BufReader.html is regenerated for
the doc-string change (bufio_index.html is unchanged, so the hand-kept
docs/index.html copy still matches it).

Disclosure

PR #8 proposed a fix of essentially this shape and was closed with "Human
contributions only unless disclosed." — a rejection of an undisclosed bot, not
of the change. This is the same bug and lands on much the same C hunk, so it
should not come as a surprise to see it again. I did not read or copy that
branch's diff; the reproduction, the fix, the wrapper decision and the tests
here are my own, and #8 predates PR #10's int* status out-parameter anyway.

---

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

[carpentry-reviewer]

Build & Tests

Checked out f830f31. Merge-base with origin/main is 265b398 = current origin/main, so nothing here is measured against a drifted tree.

• carp -x test/bufio.carp — 44 passed, 0 failed, rc 0 (read from the bare command, not through a pipe). main at 265b398 runs 39 / 0, so the five new cases are additive and the name diff shows no existing test removed.
• CI — test (ubuntu-latest) and test (macos-latest) both pass. This workflow gates Run tests, Lint (angler), Format check (carp-fmt) and Generate docs on both runners, so the tool claims in the body are CI-gated and I did not re-run them.
• carp -x gendocs.carp leaves git status --porcelain empty, and docs/index.html is byte-identical to docs/bufio_index.html (f58bedb9…), which is the copy gendocs does not maintain for you. The doc-string change reached docs/BufReader.html.
• No CHANGELOG in this repo — confirmed, so the doc string was the only place to record the new error.

Findings

Both guards are pinned independently, and the C one is the one that matters

The interesting question on this PR is not whether read-n still crashes — it is whether the C guard is tested or whether the tests merely ride on the new Carp-side one, since the Carp guard shadows it. Two mutants, restored between runs:

mutant	suite
delete if (n <= 0) return result; from src/bufio.h:167	43 / 1 failed — "the C entry point reads nothing for a negative count"
delete the (< n 1) branch from bufio.carp:113	42 / 2 failed — both "invalid count" cases

So mock_read_n_raw earns its place: without it the actual subject of #5 would ship untested. Four of the five new cases have demonstrated teeth; "a rejected count leaves the stream where it was" survives both mutants, which is expected — it guards against a future refactor that rejects after touching the stream rather than before.

The CARP_MALLOC NULL path is unreachable by the suite, but it is not unverified

The body reports this as a scope limit and that is accurate — mock_set_read_limits fails reads, not allocations. It is reachable from outside, though, so I checked the path rather than taking it on faith: a standalone driver that includes src/bufio.h directly with a CARP_MALLOC I can fail on demand.

n=-1            status=0   len=0   cap=0   data=NULL
n=INT_MIN       status=0   len=0   cap=0   data=NULL
n=0             status=0   len=0   cap=0   data=NULL
n=1             status=0   len=1   cap=1   bytes="h"
n=5             status=0   len=5   cap=5   bytes="hello"
n=11 (exact)    status=0   len=11  cap=11  bytes="hello world"
n=100 (EOF)     status=1   len=11  cap=100 bytes="hello world"
n=5, alloc fails  status=-1  len=0  cap=0  data=NULL     <-- the NULL path
n=INT_MAX       status=1   len=11  cap=2147483647 bytes="hello world"

BUFIO_ERR on allocation failure, with len 0 / data NULL, is exactly what the wrapper's (< status 0) arm needs to report Error "read error" rather than "connection closed". INT_MIN is covered by the same n <= 0, so there is no -n overflow hiding under the guard.

The same driver against 265b398's header dies with rc 139 on the first row, so the positive controls were re-run alone against it: n = 1 / 5 / 11 / 100 / INT_MAX are byte-identical across the two trees, five rows out of five. Nothing on the positive path moved.

One extra data point the body does not claim: under -Wall -Wextra -Wsign-compare, 265b398 emits

bufio.h:169:21: warning: comparison of integer expressions of different signedness:
                'size_t' {aka 'unsigned int'} and 'int' [-Wsign-compare]
  169 |   while (result.len < n) {

pointing at the exact defect line, and the branch compiles clean. The (size_t)n cast is doing real work as documentation even though, after the guard, it cannot change the comparison's value.

Two non-blocking notes, neither of them a reason to hold the PR

1. read-n is now the only NULL-checked allocation in the file. src/bufio.h:96 (bufio_empty_string), :130 and :147 (the two CARP_MALLOC(len + 1) in read_until) still use their result without checking it, and :43/:47 in BufReader_create_ do the same. #5 asked for the read-n half and that is what landed, so this is not a gap in the PR — but the file is now inconsistent in a way it was not before, and read_until's two sites are the same defect with the same rarity. Worth an issue if you want the set closed.

2. n = 0 moves from Error "connection closed" to Error "invalid count". The body argues this and I agree the new message is the truthful one — the stream is neither closed nor touched. Naming the alternative anyway, since #5 left the choice open and the PR narrowed it: io.ReadFull-shaped APIs elsewhere return an empty success for a zero-length request, which is what a caller draining a remaining counter in a loop would want when it reaches zero. Against that: n = 0 was already an Error before this PR, so no caller's control flow changes — only the string does, and only for a caller matching on it. Your call, not a defect.

I also checked the blast radius of that string change across all 47 clones: nothing outside this repo calls BufReader.read-n at all. redis's read-n is its own private RESP-value reader and web/test/websocket.carp has its own read-n-bytes; sockets is the only dependent and pins bufio@0.1.0 without using the function.

Disclosure

Handled correctly. #8 was closed with "Human contributions only unless disclosed.", which rejects an undisclosed contributor rather than the change, and this PR says so up front instead of letting a familiar diff turn up unexplained. The two diffs are not the same: #8 predates #10's int* status out-parameter and routed a rejected count to "connection closed" via the empty-array fallthrough, where this one rejects in the wrapper with its own message.

Verdict: merge

Fixes the reported crash at the layer the report named, with both the C guard and the Carp guard independently pinned by tests, the allocation-failure path verified out-of-band, the positive path byte-identical on five rows, green CI on both runners and clean docs.