fix: safe CARP_REALLOC and size_t capacity in read-append functions

[carpentry-agent]

Summary

• Check CARP_REALLOC return value before overwriting buf->data in all read-append functions (TcpStream, UnixStream, UdpSocket). Previously a failed realloc lost the original pointer (memory leak) and the next read crashed via NULL deref.
• Change capacity arithmetic from int to size_t to prevent integer overflow on buffers approaching 2GB.
• Matches the fixes already applied in the TLS library (TlsStream_read_MINUS_append_ and TlsStream_read_MINUS_append_MINUS_nb_).

Affected functions

• TcpStream_read_MINUS_append_
• TcpStream_read_MINUS_append_MINUS_nb_
• UnixStream_read_MINUS_append_
• UnixStream_read_MINUS_append_MINUS_nb_
• UdpSocket_recv_

Test plan

• All existing tests pass (tcp_test, unix_test, poll_test — 17 tests total)

---

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

[carpentry-reviewer]

Build & Tests

Build: clean.
Tests: 16/16 pass (tcp_test 6/6, poll_test 5/5, poll_test_poll2 5/5). unix_test fails to build due to a pre-existing clang-14 segfault on armhf (Array_copy__Uint32), unrelated to this PR.
CI: passing on both ubuntu-latest and macos-latest.

Findings

None. The fix is correct and complete across all 5 CARP_REALLOC sites:

• NULL-check pattern: old code overwrote buf->data directly with the realloc result, losing the original pointer on failure (memory leak + NULL deref on next read). New code stores in grown, checks for NULL, returns -1 on failure. Correct.
• size_t arithmetic: old int new_cap could overflow at ~1GB. New size_t new_cap is strictly better. The expression (buf->len + (size_t)SOCK_BUF_SIZE) * 2 cannot practically overflow on 64-bit; on 32-bit, realloc would fail before size_t wraps (~2GB). Not a concern.
• UdpSocket_recv_ capacity check: old (int)buf->capacity < SOCK_BUF_SIZE vs new buf->capacity < (size_t)SOCK_BUF_SIZE — semantically equivalent for positive SOCK_BUF_SIZE (4096). The new version is actually safer for capacities exceeding INT_MAX.
• Caller handling: all 5 Carp-side wrappers check for -1 and convert to Result.Error with System.error-text. On realloc failure, errno is ENOMEM, so the error message is meaningful.
• Scope: the PR correctly limits itself to the realloc sites. Unchecked CARP_MALLOC calls elsewhere are a separate concern and consistent with Carp's general approach.

Verdict: merge

Correct, well-scoped fix. All realloc sites covered, callers handle errors properly.

[hellerve]

let’s reduce duplication here.

[carpentry-agent]

Addressed @hellerve's feedback to reduce duplication:

• Extracted buf_grow_for_read() in common.h — ensures at least SOCK_BUF_SIZE bytes free for appending, with the same doubling strategy and NULL check. Replaces the 4 identical blocks in TcpStream_read_append_, TcpStream_read_append_nb_, UnixStream_read_append_, and UnixStream_read_append_nb_.
• Extracted buf_ensure() in common.h — ensures a minimum total capacity. Replaces the capacity check in UdpSocket_recv_.

All 22 tests pass (tcp: 6, unix: 6, poll: 5, poll2: 5).

[carpentry-reviewer]

Build & Tests

CI: pass (ubuntu-latest, macos-latest)
Local build: no Carp compiler available on this machine — relied on CI.

Prior feedback

1. First review found the realloc safety fix correct across all 5 sites, recommended merge.
2. hellerve requested "let's reduce duplication here" — addressed by extracting buf_grow_for_read and buf_ensure into src/common.h.

Findings

Reviewed src/common.h (full file), src/tcp_stream.h, src/unix_stream.h, src/udp_socket.h in full. Checked for remaining realloc duplication across all source files.

No issues found.

1. buf_grow_for_read is correct. Capacity check buf->capacity - buf->len >= SOCK_BUF_SIZE is safe (capacity >= len is an Array invariant). Growth formula (len + SOCK_BUF_SIZE) * 2 guarantees at least SOCK_BUF_SIZE free bytes after realloc. NULL check preserves buf->data on failure.

2. buf_ensure is correct. Simple minimum-capacity guarantee with NULL check. Exact allocation (no growth factor) is appropriate for the UDP use case.

3. All 5 callers are consistent. Each checks != 0 and returns -1 on failure before proceeding to the I/O call.

4. __attribute__((unused)) on static functions in a header follows the existing pattern in this file (sock_error_string, sockaddr_to_string, resolve_address, send_all). Standard approach for Carp FFI headers.

5. No remaining realloc duplication. Checked tcp_bufio.h, unix_bufio.h, tcp_listener.h, unix_listener.h, poll.h — none contain similar realloc patterns. The TLS fix mentioned in the PR description lives in a separate repo.

Verdict: merge

Clean mechanical extraction. Both helpers are correct, all callers consistent, no remaining duplication.

[hellerve]

why is this attribute here?

[carpentry-agent]

@hellerve — the attribute suppresses -Wunused-function. poll.h includes common.h but doesn't call buf_grow_for_read or buf_ensure (it only needs the includes, SOCK_BUF_SIZE, and the SIGPIPE constructor). Without the attribute, any TU that pulls in poll.h gets a warning for each unused static helper. Same reason it's on every other static function in the file (sock_error_string, resolve_address, send_all, etc.).

[carpentry-reviewer]

Build & Tests

CI: passing on both ubuntu-latest and macos-latest.
Local build: no Carp compiler available — relied on CI. C headers reviewed by inspection.

Prior feedback

1. First review: found the realloc safety fix correct, recommended merge.
2. hellerve — "let's reduce duplication here": addressed. buf_grow_for_read and buf_ensure extracted into src/common.h.
3. Second review: confirmed extraction correct, recommended merge.
4. hellerve — "why is this attribute here?": agent explained __attribute__((unused)) is needed because poll.h includes common.h without calling the helpers, which would trigger -Wunused-function. consistent with the 6 other static functions in the same file (sock_error_string, sockaddr_to_string, sockaddr_port, resolve_address, send_all, carp_sock_ignore_sigpipe has constructor instead). hellerve left a subsequent empty comment, likely acknowledging.

Findings

re-reviewed all changed files in full after the helper extraction. no issues found.

1. buf_grow_for_read (common.h:87-95): capacity check buf->capacity - buf->len >= SOCK_BUF_SIZE is safe (capacity >= len is an Array invariant). growth formula (len + SOCK_BUF_SIZE) * 2 guarantees at least SOCK_BUF_SIZE free bytes. NULL check preserves buf->data. correct.

2. buf_ensure (common.h:100-107): minimum-capacity guarantee with NULL check. exact allocation (no growth factor) is appropriate for the UDP use case where a single recv needs a known buffer size. correct.

3. all 5 call sites are consistent:

   • TcpStream_read_MINUS_append_ (tcp_stream.h:87)
   • TcpStream_read_MINUS_append_MINUS_nb_ (tcp_stream.h:126)
   • UnixStream_read_MINUS_append_ (unix_stream.h:71)
   • UnixStream_read_MINUS_append_MINUS_nb_ (unix_stream.h:131)
   • UdpSocket_recv_ (udp_socket.h:46)

   each checks != 0 and returns -1 on failure before the I/O call. the Carp-side wrappers convert -1 to Result.Error with System.error-text, giving ENOMEM on realloc failure.

4. no remaining duplication. checked tcp_bufio.h, unix_bufio.h, tcp_listener.h, unix_listener.h, poll.h — none contain similar realloc patterns.

Verdict: merge

clean, correct change. CI green, duplication eliminated, all reviewer feedback addressed.