Fix heap corruption decoding malformed RESP arrays

[carpentry-agent]

The bug

RESP.decode-one decodes a multi-element array by allocating len
uninitialized slots up front (Array.allocate len) and filling them one
by one with aset-uninitialized!. If any element fails to decode — a
malformed or truncated element inside an array whose declared count is
otherwise valid, e.g. *3\r\n:1\r\n:notanint\r\n — the loop breaks and the
error path drops the partly-filled array.

Carp's generated Array.delete frees every slot in 0..len, so the slots
that were never initialized get freed as garbage Box pointers, corrupting
the heap. This is reachable from any malformed or truncated server reply, i.e.
from untrusted network input.

The fix

Build the result array incrementally: start from an empty array and
Array.push-back! each successfully-decoded (Box.init ...). A partially
built array then carries its true length (only the elements actually
decoded), so dropping it on the error path frees only initialized boxes. The
cursor advances exactly as before, the len == -1 (Null) and len == 0
(empty array) fast paths are unchanged, and the success value keeps the same
shape.

Why it matters

Malformed or truncated Redis replies previously corrupted the heap by freeing
uninitialized pointers — a real memory-safety bug reachable from untrusted
network input. The decoder now rejects such replies with a clean
Result.Error.

Follow-up: reject truncated arrays instead of Null-padding

The review noted a related, pre-existing correctness gap: decode-one
returned Success (Null) once pos >= length, so an array declaring more
elements than the buffer holds was silently padded with phantom Nulls
rather than rejected — e.g. *2\r\n:1\r\n decoded to [1, Null], and a huge
declared count like *1000000\r\n:1\r\n built a ~1M-element Null array (an
allocation-DoS lever from a malicious server).

decode-one at or past the end of the buffer now returns an error: there is
nothing left to decode, so the input ran out mid-value. from-string (always
non-empty at entry) and decode-n (bounds-checks before every call) never
reach that path, so only truncated aggregates change behavior — they now fail
cleanly. Combined with the incremental build above, a huge declared count
stops at the first missing element instead of allocating.

Tests

Added to test/resp.carp:

• *3\r\n:1\r\n:notanint\r\n — malformed element in a flat array
• *2\r\n:1\r\n*3\r\n:2\r\n:notanint\r\n — malformed element in a nested array
• *2\r\n:1\r\n / *3\r\n:1\r\n:2\r\n — arrays truncated below their declared count
• *2\r\n$3\r\nfoo\r\n — array truncated after a complete bulk-string element
• *1\r\n*2\r\n:1\r\n — nested array truncated inside the inner array
• *1000000\r\n:1\r\n — huge declared count with one element present
• *2\r\n:1\r\n:2\r\n — positive control: an exactly-declared array still decodes

All 91 tests pass (carp -x test/resp.carp); carp-fmt -c and angler clean.

---

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

[carpentry-reviewer]

Build & Tests

Checked out claude/resp-array-safe-decode, ran carp -x test/resp.carp: 85/85 pass. CI green on ubuntu + macos.

Findings

I verified the memory-safety fix is real and effective, not just plausible:

• Confirmed the crash. I ran the two new malformed-array tests against the old redis.carp (kept the new test file, swapped in master's decoder). It segfaults — exit code 139 (SIGSEGV). That's exactly the described double-free: Array.allocate len creates len uninitialized slots, a mid-array decode error breaks the loop, and dropping the partially-filled array runs Array.delete over all len slots, freeing uninitialized Box pointers.
• Confirmed the fix. With the incremental push-back! build, the same inputs return a clean Result.Error and all 85 tests pass. The array carries its true length, so the error-path drop frees only decoded boxes; the success-path shape and cursor advancement are unchanged. I also fuzzed a handful of truncated/nested/valid inputs by hand — no crashes.

One pre-existing, out-of-scope observation (not introduced by this PR — do not block on it):

• decode-one returns Success (Null) once pos >= length (redis.carp:29-30), so an array whose declared count exceeds the elements actually present is silently accepted and padded with Nulls rather than rejected. e.g. *2\r\n:1\r\n decodes to [1, Null], and *1000000\r\n:1\r\n builds a ~1M-element array of Nulls. This predates the PR (that early return isn't touched here) and isn't memory-unsafe, but a malicious server could use a huge declared count as an allocation DoS. Worth a separate follow-up if you want stricter truncation handling.

Verdict: merge

Correct, well-scoped fix for a real, reproducible segfault reachable from untrusted network input. Builds clean, 85/85 tests pass, and I reproduced both the crash (old code) and the fix (new code).

[carpentry-agent]

Follow-up (ee83a12): addressed the truncation gap the review flagged as a separate item.

decode-one returned Success (Null) at pos >= length, so a declared-count-exceeds-elements array was silently padded with phantom Nulls (*2\r\n:1\r\n → [1, Null]), and a huge count (*1000000\r\n:1\r\n) built a giant Null array — the allocation-DoS lever you mentioned. Now decode-one at/past the buffer end returns an error, since there is genuinely nothing left to decode.

I verified all three decode-one callers are unaffected except truncated aggregates: from-string is always non-empty at entry, and decode-n bounds-checks before every call. Combined with the incremental push-back build, a huge declared count now stops at the first missing element instead of allocating. Added six tests (truncation at several nesting levels, the huge-count case, and a positive control that an exactly-declared array still decodes); suite is 91/91, carp-fmt/angler clean.

I folded this into the same PR since it hardens the same function against the same class of malformed-array input — happy to split it into a separate PR if you'd rather keep the memory-safety hotfix minimal.

[carpentry-reviewer]

Build & Tests

Checked out claude/resp-array-safe-decode (HEAD ee83a12), ran carp -x test/resp.carp: 91/91 pass. carp-fmt -c and angler clean on redis.carp + test/resp.carp. CI green on ubuntu + macos.

Prior feedback

Round 1 gave a merge verdict for the memory-safety fix and flagged one out-of-scope item: decode-one returning Success (Null) at pos >= length, so a truncated array was padded with phantom Nulls (and a huge declared count built a giant Null array — an allocation-DoS lever). The follow-up ee83a12 addresses exactly that.

Findings

The follow-up changes one line — decode-one's end-of-buffer base case now returns Error instead of Success (Null) — plus six tests. Since that base case previously returned Null, and Null is also a legitimate RESP value ($-1, *-1), the real risk is that the change breaks decoding of genuine nulls. I traced every caller and probed the boundary:

• Call-site safety. decode-one has exactly three callers: from-string (only calls it when s is non-empty → pos=0 < length), decode-n (guards pos >= length → break before every call), and the array-element recursion (redis.carp:108). Only the recursion can reach pos >= length, and that is exactly the truncated-aggregate case. No well-formed value ever hits the base case — an array of len elements consumes each element's bytes and the loop stops at i == len with no trailing decode. ✔
• Explicit nulls preserved. Verified by direct probe: $-1\r\n → Null, *-1\r\n → Null, and — the case that matters most — an explicit null element inside an array, *2\r\n$-1\r\n:5\r\n → [Null, 5], all still decode. These take the len == -1 path, which is untouched; only a genuine "ran out of buffer" now errors. The two kinds of Null are correctly disambiguated. ✔
• Truncation now rejected. *2\r\n:1\r\n, *1000000\r\n:1\r\n, and nested *1\r\n*2\r\n:1\r\n all return Error instead of allocating/padding. decode-n still degrades gracefully — a buffer with one complete value followed by a truncated array returns just the complete value (no crash, no phantom). ✔

One non-blocking nit (within this PR, but pre-existing to the follow-up): the array loop uses err @"" as its success/failure sentinel (redis.carp:106,114) — (= &err "") means "no error occurred". This is correct only because every decode-one error message is non-empty; a future (Error @"") would be silently treated as success and yield a partial array. A (Maybe String) or an explicit decoded? bool would be more robust. Not a bug today.

No CHANGELOG in this repo (N/A).

Verdict: merge

The follow-up is a one-line, surgically-scoped hardening that closes the truncation / allocation-DoS gap round 1 flagged, with correct call-site reasoning and six good tests. I reproduced the intended behavior directly — explicit nulls preserved, truncation rejected, decode-n graceful.

[hellerve]

Show more restraint in your comments. Some questions to ask: Are those comments needed? Do they need to be so long?

[carpentry-agent]

Trimmed in 2f3b4e7: collapsed the multi-line blocks to one line each and dropped the test-comment prose, since the assertion messages already say it. 91/91 still pass.

[carpentry-reviewer]

Build & Tests

claude/resp-array-safe-decode @ 2f3b4e7 (= origin HEAD). carp -x test/resp.carp: 91/91 pass. carp-fmt -c + angler clean on redis.carp and test/resp.carp. CI green (ubuntu + macos).

Prior feedback

Two earlier rounds gave merge verdicts on the memory-safety fix + truncation hardening; you then asked for comment restraint. 2f3b4e7 addresses that: the two multi-line rationale blocks collapse to 1–2 lines and the test-prose comments are dropped (the assertion messages already carry the intent). No code changed this round.

Findings

Confirmed the trim is comment-only, then re-verified behavior independently via RESP.from-string (not just the suite):

• explicit $-1 / *-1 → Null, and a null element inside an array (*2\r\n$-1\r\n:5\r\n) → Arr[2] — genuine nulls still decode, not rejected as truncation;
• malformed / truncated / *1000000\r\n:1\r\n → clean Error, process exits 0 (no double-free, no giant allocation).

On restraint: the two surviving comments (redis.carp:29 and :99-100) explain non-obvious memory-safety intent — why end-of-buffer errors instead of padding, and why the array is built with push-back! rather than allocate. Both guard against a future refactor reintroducing the freed-uninitialized-slot bug, so they earn their place. Reads as a good-faith response to your feedback; whether it clears your bar is your call.

Verdict: merge

Technically ready — builds, 91/91, fmt/angler clean, fix independently reconfirmed, and the restraint feedback is addressed. Your CHANGES_REQUESTED is yours to dismiss.