From f830f310d7a07ec6831c85bfa5554c814b7423ac Mon Sep 17 00:00:00 2001 From: "carpentry-heartbeat[bot]" Date: Fri, 21 Aug 2026 11:45:49 +0200 Subject: [PATCH] Reject a count below one in read-n instead of crashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BufReader_read_MINUS_n_` allocated `CARP_MALLOC(n)` unchecked and looped on `result.len < n`. `result.len` is `size_t`, so a negative `n` converted to a huge bound and the loop memcpy'd into a buffer that was never allocated: `(BufReader.read-n &br -1)` segfaulted. A failed allocation for a legitimate `n` copied into NULL just the same. The C entry point now returns an empty array for a non-positive `n`, NULL-checks the allocation and reports BUFIO_ERR when it fails, and compares the loop bound against `(size_t)n`. The positive path is otherwise unchanged. An `n` below one is a caller mistake, not a stream condition, so the Carp wrapper rejects it up front with `Error "invalid count"` rather than letting the empty array fall through to `Error "connection closed"` — this library's docs go out of their way to distinguish a failed read from a closed one, and saying the connection closed when nothing was read would be a wrong diagnosis. `n = 0` already returned an error; only its message changes. Fixes #5. --- bufio.carp | 18 ++++++++++------- docs/BufReader.html | 1 + src/bufio.h | 16 +++++++++++---- test/bufio.carp | 49 +++++++++++++++++++++++++++++++++++++++++++++ test/mock_stream.h | 8 ++++++++ 5 files changed, 81 insertions(+), 11 deletions(-) diff --git a/bufio.carp b/bufio.carp index 78e1d76..eda35ce 100644 --- a/bufio.carp +++ b/bufio.carp @@ -106,14 +106,18 @@ Ends and fails exactly like `read-line`, which is this function with `\\n`.") A stream that ends early yields the bytes it did produce; if that is none the result is `Error \"connection closed\"`. A stream that fails yields `Error \"read error\"` and the bytes already copied out are gone — unlike -`read-line`, a partial `read-n` cannot be resumed.") +`read-line`, a partial `read-n` cannot be resumed. + +An `n` below `1` is `Error \"invalid count\"` and reads nothing.") (defn read-n [br n] - (let [status 0 - arr (read-n- br n &status)] - (cond - (< status 0) (Result.Error @"read error") - (Array.empty? &arr) (Result.Error @"connection closed") - (Result.Success arr)))) + (if (< n 1) + (Result.Error @"invalid count") + (let [status 0 + arr (read-n- br n &status)] + (cond + (< status 0) (Result.Error @"read error") + (Array.empty? &arr) (Result.Error @"connection closed") + (Result.Success arr))))) (doc read-append "reads available data and appends to the byte buffer. Returns `(Result Int String)` with the number of bytes read; `0` means the diff --git a/docs/BufReader.html b/docs/BufReader.html index 5933ad1..c4c7530 100644 --- a/docs/BufReader.html +++ b/docs/BufReader.html @@ -218,6 +218,7 @@

result is Error "connection closed". A stream that fails yields Error "read error" and the bytes already copied out are gone — unlike read-line, a partial read-n cannot be resumed.

+

An n below 1 is Error "invalid count" and reads nothing.

diff --git a/src/bufio.h b/src/bufio.h index d4bb111..cdba842 100644 --- a/src/bufio.h +++ b/src/bufio.h @@ -161,15 +161,23 @@ String BufReader_read_MINUS_line_(BufReader* br, int* status) { Array BufReader_read_MINUS_n_(BufReader* br, int n, int* status) { Array result; - result.data = CARP_MALLOC(n); - result.capacity = n; + result.data = NULL; + result.capacity = 0; result.len = 0; *status = BUFIO_OK; + if (n <= 0) return result; - while (result.len < n) { + result.data = CARP_MALLOC((size_t)n); + if (!result.data) { + *status = BUFIO_ERR; + return result; + } + result.capacity = (size_t)n; + + while (result.len < (size_t)n) { int avail = bufreader_available(br); if (avail > 0) { - int want = n - result.len; + int want = n - (int)result.len; int take = avail < want ? avail : want; memcpy((char*)result.data + result.len, br->rbuf + br->rbuf_pos, take); result.len += take; diff --git a/test/bufio.carp b/test/bufio.carp index 031e00d..edef523 100644 --- a/test/bufio.carp +++ b/test/bufio.carp @@ -10,6 +10,7 @@ (register mock-buffered-write-len (Fn [&BufReader] Int) "mock_buffered_write_len") +(register mock-read-n-raw (Fn [&BufReader Int] Int) "mock_read_n_raw") (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") @@ -167,6 +168,54 @@ world (match r (Result.Success _) @"UNEXPECTED" (Result.Error e) e)) "read-n returns error on empty stream") + (assert-equal test + "invalid count" + &(let-do [br (mock-bufreader-create "hello world" 0) + r (BufReader.read-n &br -1)] + (BufReader.delete br) + (mock-cleanup) + (match r (Result.Success _) @"UNEXPECTED" (Result.Error e) e)) + "read-n rejects a negative count instead of crashing") + + (assert-equal test + "invalid count" + &(let-do [br (mock-bufreader-create "hello world" 0) + r (BufReader.read-n &br 0)] + (BufReader.delete br) + (mock-cleanup) + (match r (Result.Success _) @"UNEXPECTED" (Result.Error e) e)) + "read-n rejects a count of zero") + + (assert-equal test + "hello" + &(let-do [br (mock-bufreader-create "hello world" 0) + _rejected (BufReader.read-n &br -1) + r (BufReader.read-n &br 5)] + (BufReader.delete br) + (mock-cleanup) + (match r + (Result.Success arr) (mock-bytes-to-string &arr) + (Result.Error _) @"ERROR")) + "a rejected count leaves the stream where it was") + + (assert-equal test + 0 + (let-do [br (mock-bufreader-create "hello world" 0) + n (mock-read-n-raw &br -1)] + (BufReader.delete br) + (mock-cleanup) + n) + "the C entry point reads nothing for a negative count") + + (assert-equal test + 5 + (let-do [br (mock-bufreader-create "hello world" 0) + n (mock-read-n-raw &br 5)] + (BufReader.delete br) + (mock-cleanup) + n) + "the C entry point still reads a positive count") + (assert-equal test 5 (let-do [br (mock-bufreader-create "hello" 0) diff --git a/test/mock_stream.h b/test/mock_stream.h index 7d251fe..4e09d53 100644 --- a/test/mock_stream.h +++ b/test/mock_stream.h @@ -104,6 +104,14 @@ static void mock_set_read_limits(int budget, int fail_code) { static int mock_buffered_write_len(BufReader* br) { return br->wbuf_len; } +/* 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; + Array a = BufReader_read_MINUS_n_(br, n, &status); + if (a.data) CARP_FREE(a.data); + return status < 0 ? -1 : (int)a.len; +} + /* Long-typed view of bufio_next_cap, which takes and returns size_t; -1 for an argument that not every size_t can represent. */ static int64_t bufio_next_cap_long(int64_t have, int64_t need) {