From 245f38d37093fd03d9990be6d90bf098bfbe57cd Mon Sep 17 00:00:00 2001 From: "carpentry-heartbeat[bot]" Date: Sat, 22 Aug 2026 00:49:54 +0200 Subject: [PATCH 1/2] Check every fixed-size allocation instead of dereferencing NULL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two growth paths already return an error when an allocation fails, but the seven fixed-size CARP_MALLOC calls next to them each dereferenced their result immediately. Three of the failing sites have no error channel to report through, so each degrades instead: - BufReader_create_ returns a BufReader by value. A buffer it could not allocate is now recorded as absent — NULL with capacity 0 — rather than as an 8192-byte one that is not there. bufio_reserve treats NULL as a fresh allocation, so the buffer appears on first use and the reader works. Capacity 0 is unreachable on the normal path, so bufio_next_cap and the growth schedule are untouched. - bufio_empty_string returns a String the Carp side frees, so it cannot return a static one. It now returns NULL and downgrades *status to BUFIO_ERR, which the Carp wrapper checks before it looks at the string; the NULL is freed, and free(NULL) is a no-op. - BufReader_copy implements Carp's copy, which has no error channel. It now falls back to a buffer sized to the bytes actually held, so a copy made under memory pressure keeps its data at a tighter capacity, and only drops it if even that cannot be had. The two read-until slice allocations report BUFIO_ERR and consume nothing, so a retry resumes from the same place, matching what a failed read already did. --- src/bufio.h | 53 ++++++++++++++++++++++++++++++++++------------ test/bufio.carp | 53 +++++++++++++++++++++++++++++++++++++++++++++- test/mock_stream.h | 14 ++++++++++++ 3 files changed, 105 insertions(+), 15 deletions(-) diff --git a/src/bufio.h b/src/bufio.h index cdba842..de57661 100644 --- a/src/bufio.h +++ b/src/bufio.h @@ -33,6 +33,8 @@ typedef struct { /* --- Construction / destruction --- */ +/* A buffer that could not be allocated is NULL with capacity 0, holding + nothing; bufio_reserve allocates it on first use. */ BufReader BufReader_create_(void* inner, bufio_read_fn rfn, bufio_write_fn wfn, bufio_close_fn cfn) { BufReader br; @@ -43,10 +45,10 @@ BufReader BufReader_create_(void* inner, bufio_read_fn rfn, br.rbuf = CARP_MALLOC(BUFIO_DEFAULT_CAP); br.rbuf_len = 0; br.rbuf_pos = 0; - br.rbuf_cap = BUFIO_DEFAULT_CAP; + br.rbuf_cap = br.rbuf ? BUFIO_DEFAULT_CAP : 0; br.wbuf = CARP_MALLOC(BUFIO_DEFAULT_CAP); br.wbuf_len = 0; - br.wbuf_cap = BUFIO_DEFAULT_CAP; + br.wbuf_cap = br.wbuf ? BUFIO_DEFAULT_CAP : 0; return br; } @@ -92,8 +94,27 @@ static int bufio_reserve_array(Array* buf, size_t extra) { return 0; } -static String bufio_empty_string(void) { +/* Copy of the `used` bytes of `src` with capacity `cap`, falling back to a + tight buffer and then to none; `*cap_out` is the capacity obtained. */ +static char* bufio_dup_buf(const char* src, int used, int cap, int* cap_out) { + char* buf = cap > 0 ? CARP_MALLOC((size_t)cap) : NULL; + *cap_out = buf ? cap : 0; + if (!buf && used > 0) { + buf = CARP_MALLOC((size_t)used); + *cap_out = buf ? used : 0; + } + if (buf && used > 0) memcpy(buf, src, (size_t)used); + return buf; +} + +/* Empty string for a read that produced nothing; NULL with `*status` set to + BUFIO_ERR if even one byte cannot be had. */ +static String bufio_empty_string(int* status) { String s = CARP_MALLOC(1); + if (!s) { + *status = BUFIO_ERR; + return s; + } s[0] = '\0'; return s; } @@ -128,6 +149,10 @@ String BufReader_read_MINUS_until_(BufReader* br, char delim, int* status) { if (br->rbuf[i] == delim) { int len = i - br->rbuf_pos + 1; String s = CARP_MALLOC(len + 1); + if (!s) { + *status = BUFIO_ERR; + return bufio_empty_string(status); + } memcpy(s, br->rbuf + br->rbuf_pos, len); s[len] = '\0'; br->rbuf_pos += len; @@ -138,19 +163,23 @@ String BufReader_read_MINUS_until_(BufReader* br, char delim, int* status) { int r = bufreader_fill(br); if (r < 0) { *status = BUFIO_ERR; - return bufio_empty_string(); + return bufio_empty_string(status); } if (r == 0) { int avail = bufreader_available(br); *status = BUFIO_EOF; if (avail > 0) { String s = CARP_MALLOC(avail + 1); + if (!s) { + *status = BUFIO_ERR; + return bufio_empty_string(status); + } memcpy(s, br->rbuf + br->rbuf_pos, avail); s[avail] = '\0'; br->rbuf_pos += avail; return s; } - return bufio_empty_string(); + return bufio_empty_string(status); } } } @@ -261,15 +290,11 @@ BufReader BufReader_copy(BufReader* br) { c.read_fn = br->read_fn; c.write_fn = br->write_fn; c.close_fn = br->close_fn; - c.rbuf_cap = br->rbuf_cap; - c.rbuf_len = br->rbuf_len; - c.rbuf_pos = br->rbuf_pos; - c.rbuf = CARP_MALLOC(c.rbuf_cap); - memcpy(c.rbuf, br->rbuf, c.rbuf_len); - c.wbuf_cap = br->wbuf_cap; - c.wbuf_len = br->wbuf_len; - c.wbuf = CARP_MALLOC(c.wbuf_cap); - memcpy(c.wbuf, br->wbuf, c.wbuf_len); + c.rbuf = bufio_dup_buf(br->rbuf, br->rbuf_len, br->rbuf_cap, &c.rbuf_cap); + c.rbuf_len = c.rbuf ? br->rbuf_len : 0; + c.rbuf_pos = c.rbuf ? br->rbuf_pos : 0; + c.wbuf = bufio_dup_buf(br->wbuf, br->wbuf_len, br->wbuf_cap, &c.wbuf_cap); + c.wbuf_len = c.wbuf ? br->wbuf_len : 0; return c; } diff --git a/test/bufio.carp b/test/bufio.carp index edef523..583383d 100644 --- a/test/bufio.carp +++ b/test/bufio.carp @@ -11,6 +11,7 @@ (Fn [&BufReader] Int) "mock_buffered_write_len") (register mock-read-n-raw (Fn [&BufReader Int] Int) "mock_read_n_raw") +(register mock-starve-buffers (Fn [&BufReader] ()) "mock_starve_buffers") (register bufio-next-cap-long (Fn [Long Long] Long) "bufio_next_cap_long") (register mock-get-output (Fn [] String) "mock_get_output") (register mock-is-closed? (Fn [] Bool) "mock_is_closed") @@ -442,4 +443,54 @@ world (assert-equal test -1l (bufio-next-cap-long 1073741824l -1l) - "a request that is not a size is refused")) + "a request that is not a size is refused") + + (assert-equal test "hello +" &(let-do [br (mock-bufreader-create "hello +world +" 0)] (mock-starve-buffers &br) (let-do [r (BufReader.read-line &br)] + (BufReader.delete br) + (mock-cleanup) + (match r (Result.Success s) s (Result.Error _) @"ERROR"))) "a reader whose read buffer could not be allocated still reads a line") + + (assert-equal test + "hello world" + &(let-do [br (mock-bufreader-create "" 0)] + (mock-starve-buffers &br) + (ignore (BufReader.write &br "hello world")) + (ignore (BufReader.flush &br)) + (let-do [output (mock-get-output)] + (BufReader.delete br) + (mock-cleanup) + output)) + "a reader whose write buffer could not be allocated still writes") + + (assert-true test + (let-do [payload (String.repeat 3000 "0123456789") + br (mock-bufreader-create &payload 0)] + (mock-starve-buffers &br) + (let-do [r (BufReader.read-until &br \;)] + (BufReader.delete br) + (mock-cleanup) + (match r (Result.Success s) (= &s &payload) (Result.Error _) false))) + "a reader with no read buffer grows back past the default capacity") + + (assert-equal test "world +" &(let-do [br (mock-bufreader-create "hello +world +" 0) + _first-line (BufReader.read-line &br) + c (BufReader.copy &br) + r (BufReader.read-line &c)] (BufReader.delete c) (BufReader.delete br) (mock-cleanup) (match r + (Result.Success s) s + (Result.Error _) @"ERROR")) "a copy keeps the bytes the original had buffered") + + (assert-equal test "hello +" &(let-do [br (mock-bufreader-create "hello +world +" 0)] (mock-starve-buffers &br) (let-do [c (BufReader.copy &br) + r (BufReader.read-line &c)] + (BufReader.delete c) + (BufReader.delete br) + (mock-cleanup) + (match r (Result.Success s) s (Result.Error _) @"ERROR"))) "a copy of a reader with no buffers allocates on first use")) diff --git a/test/mock_stream.h b/test/mock_stream.h index 4e09d53..ca90317 100644 --- a/test/mock_stream.h +++ b/test/mock_stream.h @@ -104,6 +104,20 @@ static void mock_set_read_limits(int budget, int fail_code) { static int mock_buffered_write_len(BufReader* br) { return br->wbuf_len; } +/* Puts a reader in the state BufReader_create_ leaves behind when neither + buffer could be allocated. */ +static void mock_starve_buffers(BufReader* br) { + if (br->rbuf) CARP_FREE(br->rbuf); + if (br->wbuf) CARP_FREE(br->wbuf); + br->rbuf = NULL; + br->rbuf_cap = 0; + br->rbuf_len = 0; + br->rbuf_pos = 0; + br->wbuf = NULL; + br->wbuf_cap = 0; + br->wbuf_len = 0; +} + /* read-n's C entry point unguarded by the Carp wrapper; -1 on error status. */ static int mock_read_n_raw(BufReader* br, int n) { int status = BUFIO_OK; From 512cdbf61547b7c6c217c1fa93e18ccd80c09ce1 Mon Sep 17 00:00:00 2001 From: "carpentry-heartbeat[bot]" Date: Sat, 22 Aug 2026 06:21:00 +0200 Subject: [PATCH 2/2] Refill a starved reader at the default capacity, skip empty writes bufreader_fill asked for one byte past what was buffered, which read-n never grows past: it drains the buffer every iteration, so bufio_reserve(cap, used=0, extra=1) finds need <= have and the capacity stays at 1. A reader that lost its buffer to a failed allocation then made one read_fn call per byte, permanently -- 4000 calls for read-n(4000), 2000 for 200 x read-n(10) -- rather than the "O(log n) extra reads once per reader" the design note claimed. read-until is unaffected (13 calls for a 4001-byte line) because it fills before it drains. The fill request is now floored at BUFIO_DEFAULT_CAP while the capacity is below it, falling back to the one-byte request when that allocation fails, so a reader under sustained memory pressure still makes progress byte by byte and warms back up as soon as memory is available. The write path reached memcpy(br->wbuf + br->wbuf_len, ..., 0) with a NULL wbuf for a zero-length write: UB of the same class this branch exists to close, and reachable now that a NULL wbuf is a supported steady state rather than a doomed one. Guarding both memcpys on a non-zero length matches how the rest of the header already guards them. --- src/bufio.h | 12 +++++++++--- test/bufio.carp | 29 ++++++++++++++++++++++++++++- test/mock_stream.h | 5 +++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/bufio.h b/src/bufio.h index de57661..23f90d5 100644 --- a/src/bufio.h +++ b/src/bufio.h @@ -129,7 +129,13 @@ static int bufreader_fill(BufReader* br) { br->rbuf_len = remaining; br->rbuf_pos = 0; } - if (bufio_reserve(&br->rbuf, &br->rbuf_cap, (size_t)br->rbuf_len, 1) != 0) + /* A capacity below the default belongs to a reader recovering from a failed + allocation. */ + size_t extra = br->rbuf_cap < BUFIO_DEFAULT_CAP + ? (size_t)(BUFIO_DEFAULT_CAP - br->rbuf_len) + : 1; + if (bufio_reserve(&br->rbuf, &br->rbuf_cap, (size_t)br->rbuf_len, extra) != 0 + && bufio_reserve(&br->rbuf, &br->rbuf_cap, (size_t)br->rbuf_len, 1) != 0) return -1; int space = br->rbuf_cap - br->rbuf_len; int n = br->read_fn(br->inner, br->rbuf + br->rbuf_len, space); @@ -247,7 +253,7 @@ int BufReader_write_(BufReader* br, String* data) { size_t len = strlen(*data); if (bufio_reserve(&br->wbuf, &br->wbuf_cap, (size_t)br->wbuf_len, len) != 0) return -1; - memcpy(br->wbuf + br->wbuf_len, *data, len); + if (len > 0) memcpy(br->wbuf + br->wbuf_len, *data, len); br->wbuf_len += (int)len; return (int)len; } @@ -256,7 +262,7 @@ int BufReader_write_MINUS_bytes_(BufReader* br, Array* data) { size_t len = data->len; if (bufio_reserve(&br->wbuf, &br->wbuf_cap, (size_t)br->wbuf_len, len) != 0) return -1; - memcpy(br->wbuf + br->wbuf_len, data->data, len); + if (len > 0) memcpy(br->wbuf + br->wbuf_len, data->data, len); br->wbuf_len += (int)len; return (int)len; } diff --git a/test/bufio.carp b/test/bufio.carp index 583383d..845e026 100644 --- a/test/bufio.carp +++ b/test/bufio.carp @@ -12,6 +12,7 @@ "mock_buffered_write_len") (register mock-read-n-raw (Fn [&BufReader Int] Int) "mock_read_n_raw") (register mock-starve-buffers (Fn [&BufReader] ()) "mock_starve_buffers") +(register mock-read-calls (Fn [] Int) "mock_read_calls") (register bufio-next-cap-long (Fn [Long Long] Long) "bufio_next_cap_long") (register mock-get-output (Fn [] String) "mock_get_output") (register mock-is-closed? (Fn [] Bool) "mock_is_closed") @@ -493,4 +494,30 @@ world (BufReader.delete c) (BufReader.delete br) (mock-cleanup) - (match r (Result.Success s) s (Result.Error _) @"ERROR"))) "a copy of a reader with no buffers allocates on first use")) + (match r (Result.Success s) s (Result.Error _) @"ERROR"))) "a copy of a reader with no buffers allocates on first use") + + (assert-equal test + 1 + (let-do [payload (String.repeat 400 "0123456789") + br (mock-bufreader-create &payload 0)] + (mock-starve-buffers &br) + (ignore (BufReader.read-n &br 4000)) + (let-do [calls (mock-read-calls)] + (BufReader.delete br) + (mock-cleanup) + calls)) + "a reader with no read buffer refills at the default capacity, not one byte per call") + + (assert-equal test + "hi" + &(let-do [br (mock-bufreader-create "" 0)] + (mock-starve-buffers &br) + (ignore (BufReader.write &br "")) + (ignore (BufReader.write-bytes &br &(the (Array Byte) []))) + (ignore (BufReader.write &br "hi")) + (ignore (BufReader.flush &br)) + (let-do [output (mock-get-output)] + (BufReader.delete br) + (mock-cleanup) + output)) + "an empty write to a reader with no write buffer buffers nothing and leaves it usable")) diff --git a/test/mock_stream.h b/test/mock_stream.h index ca90317..d995c98 100644 --- a/test/mock_stream.h +++ b/test/mock_stream.h @@ -20,12 +20,14 @@ typedef struct { int write_fail_code; int read_budget; int read_fail_code; + int read_calls; } MockStream; static MockStream* g_mock = NULL; static int mock_stream_read(void* inner, char* buf, int len) { MockStream* ms = (MockStream*)inner; + ms->read_calls++; if (ms->read_budget >= 0) { if (ms->read_budget == 0) return ms->read_fail_code; if (len > ms->read_budget) len = ms->read_budget; @@ -79,6 +81,7 @@ static BufReader mock_bufreader_create(String* data, int chunk_size) { ms->write_fail_code = -1; ms->read_budget = -1; ms->read_fail_code = -1; + ms->read_calls = 0; g_mock = ms; return BufReader_create_( (void*)ms, mock_stream_read, mock_stream_write, mock_stream_close); @@ -104,6 +107,8 @@ static void mock_set_read_limits(int budget, int fail_code) { static int mock_buffered_write_len(BufReader* br) { return br->wbuf_len; } +static int mock_read_calls(void) { return g_mock ? g_mock->read_calls : -1; } + /* Puts a reader in the state BufReader_create_ leaves behind when neither buffer could be allocated. */ static void mock_starve_buffers(BufReader* br) {