reliable 26-2/26-3: zero the reassembly buffer, terminate config.name

[gafferongames]

Two small hardening fixes from the security review (security#26-2 and security#26-3), each with a red-first test.

security#26-2: zero the fragment reassembly buffer

reliable_endpoint_receive_packet allocates the reassembly buffer through the caller-supplied
allocate_function, which is malloc-shaped and returns uninitialized memory. Completeness is
guaranteed today by the fragment bitmap plus validated offsets, so uninitialized bytes cannot reach
the process callback through correct code. But any future logic error that skipped storing a fragment
would leak stale heap contents into a delivered packet. The buffer is now memset to zero for
packet_buffer_size bytes immediately after the NULL check succeeds.

The caller's allocator is untouched: the allocation is still allocate_function, not calloc, and
the caller's allocator is not bypassed.

test_fragment_reassembly_buffer_zeroed drives a fragmented packet through a receiver whose
allocator fills fresh memory with 0xCC, delivers only the first fragment, and asserts the
untouched tail byte of the reassembly buffer is zero. Red before the fix (the tail was 0xCC),
green after.

security#26-3: terminate config.name

reliable_printf("[%s]", endpoint->config.name) reads past the 256-byte array if a caller fills all
256 bytes with no NUL. reliable_endpoint_create now forces the last byte of its own copy of
config.name to '\0' right after the copy, so every use of the name after create is bounded by
that terminator. The create-time error paths run before that copy and print the caller's own buffer:
the twelve rejection logs in reliable_config_valid now print the name with a %.*s width of
sizeof(config.name) - 1, and the allocation-failure log prints the endpoint's terminated copy.
Caller-controlled only.

test_endpoint_name_terminated creates an endpoint from a config whose name is 256 non-NUL bytes
and asserts the endpoint's own copy is NUL-terminated at the last byte. Red before the fix, green
after. test_config_name_bounded_in_rejection_log hands an invalid config with a
256-byte non-NUL name, sized on the heap so the over-read leaves the allocation, and asserts the
logged name stops at 255 characters. Red before the fix as an ASan heap-buffer-overflow READ in
reliable_printf (and as a failed length check without a sanitizer), green after.

Gates

Run under cmake -B build -DCMAKE_BUILD_TYPE=Debug -DRELIABLE_SANITIZE=ON:

• cmake -B build -DCMAKE_BUILD_TYPE=Debug -DRELIABLE_SANITIZE=ON — configured
• cmake --build build --parallel — built
• ctest --test-dir build --output-on-failure — 100% tests passed, 0 failed out of 7
• ./build/bin/fuzz 2000000 — completed, exit 0
• python3 tools/conformance/verify_standard.py — 2608 checks, 0 failures

[gafferongames]

Security seat second eyes at exact head f8d8ded (Alex; these are my security#26 findings 2 and 3, dispositioned this morning as optional non-defects — this PR hardens both).

APPROVE — both fixes are the right shapes, and the tests are genuinely red-first quality.

1. security#26-3 (name terminator): endpoint->config.name[sizeof(...) - 1] = '\0' at create — the endpoint forces a terminator on its own copy, so a caller that fills all 256 bytes cannot make reliable_printf("%s") read past the array. The test fills the whole name with 'x' and checks the terminator. Correct placement (at create, before any logging can use it).

2. security#26-2 (reassembly zeroing): memset(packet_data, 0, packet_buffer_size) after the alloc-null check and before first use. My original disposition: completeness is guaranteed today by the fragment bitmap plus validated offsets, so this is defense-in-depth — a future logic error that skips storing a fragment leaks zeros instead of stale heap contents into a delivered packet. The poison-allocator test is the right proof: 0xCC fill, deliver only fragment 0, assert the untouched tail byte reads 0 (it read 0xCC before the fix). The test's computed size matches the allocation exactly (header + fragments × size + 8), so the tail-byte read is in bounds.

Cost check: one memset per reassembly allocation, bounded by the endpoint's own config — negligible.

Built and ran the suite at this head on this bench: cmake Release, 100% of 7 ctest targets pass (including the two new tests).

One housekeeping note for the record: the alloc-null check this stacks on (reliable#63) was my #26 finding 1 — so this PR completes the disposition of all three findings from the reliable audit.

[rowan-claude]

Read (Fable) at f8d8ded

Verdict: APPROVE.

Cold read of the diff against main (reliable.c only, +137). Built Debug + RELIABLE_SANITIZE=ON on macOS arm64; ctest 7/7; bin/test 26 tests, exit 0. Each fix line was then commented out, clean-rebuilt, run, and restored. PR CI: 10/10 checks pass at this head.

security#26-2 — zero the reassembly buffer (fix at reliable.c:1450)

Finding: "Reassembly buffers use malloc, not calloc ... any future logic error that skipped storing a fragment would leak heap contents into a delivered packet. Zeroing would be cheap defense-in-depth."

• Size is exact: memset( reassembly_data->packet_data, 0, packet_buffer_size ) uses the same local the allocation at :1435 takes (RELIABLE_MAX_PACKET_HEADER_BYTES + num_fragments * fragment_size + 8, :1428). Nothing more, nothing less.
• Allocator not bypassed: still endpoint->allocate_function at :1435; memset sits after the NULL check at :1436-1445, so a NULL return never reaches it.
• Test observes real uninitialized bytes: the receiver's allocator poisons with 0xCC (:3130-3141); fragment 0 of a 2-fragment packet writes only [9-hdr, 9+fragment_size) (store path :1201-1240), so packet_data[2064] is untouched and reads 0xCC without the fix.
• Red (memset removed): check failed: ( reassembly_data->packet_data[packet_buffer_size - 1] == 0 ), function test_fragment_reassembly_buffer_zeroed, ... line 3212 — exit 133, 15 tests reached. Green restored: exit 0, 26 tests.

security#26-3 — terminate config.name (fix at reliable.c:718)

Finding: "Latent over-read on a non-NUL-terminated config.name: reliable_printf("[%s]", config.name) reads past the 256-byte array if a caller fills all 256 bytes ... Trivial to fix with a forced NUL."

• Index is exactly sizeof( endpoint->config.name ) - 1 = 255 (char name[256], reliable.h:140). Written right after the copy at :714; the only other write to endpoint->config in the file is that copy, and reset does not re-copy, so the terminator cannot be bypassed once the endpoint exists. Every send/receive log site (:909-:1398) prints endpoint->config.name, the terminated copy.
• Red (terminator removed): check failed: ( endpoint->config.name[sizeof( endpoint->config.name ) - 1] == '\0' ), function test_endpoint_name_terminated, ... line 3793 — exit 133, 22 tests reached. Green restored: exit 0.

Findings

MEDIUM — residual over-read on the create-time error paths. reliable_config_valid( config ) runs at :648, before the copy, and its twelve rejection logs at :564-:640 print config->name — the caller's unterminated buffer. The allocation-failure log at :753 also prints config->name rather than endpoint->config.name. So the PR body's "before any printf can run" is not accurate: with a 256-byte non-NUL name and an invalid config (or a NULL-returning allocator) the over-read the finding describes still happens. Same class the audit rated Info (caller-controlled, error path only), so not blocking; the steady-state paths are closed. Fix in a follow-up: %.255s at those thirteen sites, or validate a locally terminated copy; and extend test_endpoint_name_terminated with an invalid config so it goes red.

LOW — test_fragment_reassembly_buffer_zeroed (:3205) recomputes the buffer size from sender_config.fragment_size; the allocation uses the receiver's. Equal today (both default), but the receiver's is the one the contract names.

LOW — behaviour change for correct callers: one memset of up to max_fragments * fragment_size per new reassembly slot (~16 KB at defaults), dwarfed by the fragment memcpys that follow; no wire or output change. A 256-non-NUL name is now logged as 255 chars, which is the intent.

LOW — STANDARD.md: no sentence on reassembly buffer contents or the config name (grep: reassembl, config.name, zero); nothing to update.

Counts: HIGH 0, MEDIUM 1, LOW 3.

[rowan-claude]

Read (Opus) at f8d8ded

Verdict: HOLD.

Cold read on a second model; no other reader's comment read first. Clean clone, git rev-parse HEAD = f8d8ded284a48d7ccece23f90ed25c877c1898be.
Built the documented way (CLAUDE.md:75-77) with -DCMAKE_BUILD_TYPE=Debug -DRELIABLE_SANITIZE=ON: ctest 7/7, verify_standard.py
2608 checks, 0 failures. No wire change; nothing in STANDARD.md needs updating. Both tests proven red-first by reverting each fix
hunk locally, rebuilding, and restoring (git diff --quiet after):

check failed: ( reassembly_data->packet_data[packet_buffer_size - 1] == 0 ), function test_fragment_reassembly_buffer_zeroed
check failed: ( endpoint->config.name[sizeof( endpoint->config.name ) - 1] == '\0' ), function test_endpoint_name_terminated

26-2 checks out mechanically: reliable.c:1434 allocates packet_buffer_size and reliable.c:1450 memsets the same variable, it is
the only allocation site for packet_data, it runs only inside if ( !reassembly_data ), and the caller's allocate_function is
untouched. The poison allocator makes the red deterministic rather than malloc-luck: allow_packets = 1 delivers fragment 0 only, so
the tail byte is genuinely never written by the store path. 26-3's index is right: sizeof( endpoint->config.name ) - 1 = 255 on a
real char[256], not a decayed pointer.

MEDIUM — security#26-3 is not actually closed: config->name is still logged unterminated

The finding's words: "reliable_printf("[%s]", config.name) reads past the 256-byte array if a caller fills all 256 bytes." The PR
body says the NUL is forced "before any printf can run." True of endpoint->config.name, but 15 printf sites read the caller's
config->name, not the terminated copy — 13 in reliable_config_valid (reliable.c:564, 570, 576, 582, 590, 596, 603,
613, 620, 627, 633, 640), which runs at reliable.c:649, before the copy at reliable.c:714 and so before the fix at
reliable.c:718; plus reliable.c:753, the "[%s] failed to allocate endpoint\n" path, which passes config->name when
endpoint->config.name was terminated 35 lines earlier.

PoC at this head, ASan, 256 'x' with no NUL and max_packet_size = 0 so reliable_config_valid logs:

bytes logged inside [...] = 261  (array is 256)

Five bytes past name[255] — low bytes of the adjacent context pointer (reliable.h:140-141: name, then context, no padding on
64-bit) — reach the log. Matches the audit's "reads into adjacent struct members", no crash. With context = NULL it logs exactly 256
and stops on that pointer's first zero byte, which is why the suite never sees it. Completion is three lines: terminate a local copy at
the top of reliable_config_valid, and use endpoint->config.name at reliable.c:753. I hold on the claim, not on memory-safety
severity — the change as written is a strict improvement and introduces nothing — but closing security#26-3 on this head would be
wrong, and test_endpoint_name_terminated only asserts the copy, so it would not go red on the residual.

LOW — the zeroing is now attacker-proportional work on the hostile path

packet_buffer_size (reliable.c:1427) uses num_fragments, which is wire-supplied, so a peer now chooses the memset length as well
as the allocation length, once per new reassembly entry. At defaults (16 fragments, 1024 bytes, 64 slots) that is ~1 MB per full slot
turnover — negligible, and bounded exactly as audit finding 5 bounds the allocation. Named only because it commits pages a
sparsely-filled buffer previously left untouched; not a reason to change the fix.

LOW — reliable.h:140 does not document the truncation

char name[256]; // name of the endpoint. used in log output says nothing about termination. No behaviour change for a correct caller
(255 chars + NUL untouched; a caller filling all 256 was already reading OOB), but the endpoint's copy is now silently truncated to 255
characters. One clause on that line would make the new contract readable.

LOW — test nit

test_fragment_reassembly_buffer_zeroed (reliable.c:3207) sizes its expectation from sender_config.fragment_size while the buffer
it indexes is the receiver's — equal only because both are defaults; receiver_config.fragment_size would match the allocation.

[rowan-claude]

Read (Fable) at a38f88c

Verdict: APPROVE.

Re-gate after the follow-up commit closed the f8d8ded residual. Whole diff read against origin/main (three commits: 39dbc19 red tests, f8d8ded fixes, a38f88c bounded logging). Fresh clone, cmake -DRELIABLE_SANITIZE=ON, cmake --build build --clean-first for every build below. No HIGH, no MEDIUM.

security#26 item 2 — "Reassembly buffers use malloc, not calloc ... any future logic error that skipped storing a fragment would leak heap contents into a delivered packet. Zeroing would be cheap defense-in-depth."

• reliable.c:1457 memset( reassembly_data->packet_data, 0, packet_buffer_size ); runs right after the NULL check on the allocation, with the same packet_buffer_size computed at :1434 that the allocation used. Delivered bytes are unchanged for correct code (every byte the callback sees is written by a validated fragment), so no behaviour change on the wire or in the callback.

security#26 item 3 — "reliable_printf("[%s]", config.name) reads past the 256-byte array if a caller fills all 256 bytes ... Trivial to fix with a forced NUL."

• reliable.c:725 endpoint->config.name[sizeof( endpoint->config.name ) - 1 ] = '\0'; immediately after endpoint->config = *config; at :722, before any log of the copy.
• reliable.c:554 #define RELIABLE_MAX_NAME_CHARS ( (int) sizeof( ( (struct reliable_config_t *) NULL )->name ) - 1 ) — unevaluated sizeof of the char name[256] member (reliable.h:140), so 255 = sizeof(name)-1, cast to int as %.*s requires. Checked.

Every remaining %s that could print a name, classified (grep of reliable.c at tip):

• :571 :577 :583 :589 :597 :603 :610 :620 :627 :634 :640 :647 — the twelve rejection logs in reliable_config_valid, all %.*s with RELIABLE_MAX_NAME_CHARS, config->name in that order (int precision, then pointer) at every site. Bounded.
• :760 "[%s] failed to allocate endpoint\n", endpoint->config.name — the create-time alloc-failure log now prints the copy terminated at :725, not the caller's buffer. Terminated copy. (This was the residual.)
• :916 :927 :941 :963 :1257 :1277 :1289 :1296 :1303 :1308 :1316 :1342 :1367 :1376 :1405 :1413 :1427 :1447 :1467 :1475 :1480 :1496 — all endpoint->config.name. Terminated copy.
• :1016-:1185 in reliable_read_packet_header / reliable_read_fragment_header print a name parameter; the only callers pass endpoint->config.name (:1274, :1391). Terminated copy.
• :525-:533 reliable_default_config writes "endpoint\0". Test, fuzz and soak sources only write names via reliable_copy_string. No over-read remains.

Build and tests at tip (sanitizers on, clean-first): 0 warnings under -Wall -Wextra; ctest 100% tests passed out of 7; bin/test *** ALL TESTS PASSED *** with test_fragment_reassembly_buffer_zeroed, test_endpoint_name_terminated, test_config_name_bounded_in_rejection_log in the run.

Red proofs (one revert each, clean rebuild, run, git checkout reliable.c, tree clean after each):

1. :571 back to bare "[%s] ... ", config->name → rc 134:
   ERROR: AddressSanitizer: heap-buffer-overflow ... READ of size 369 / #2 reliable_printf reliable.c:87 / #3 reliable_config_valid reliable.c:571 / #4 reliable_endpoint_create reliable.c:656
2. :725 terminator commented out → rc 133:
   check failed: ( endpoint->config.name[sizeof( endpoint->config.name ) - 1] == '\0' ), function test_endpoint_name_terminated, ... line 3800
3. :1457 memset commented out → rc 133:
   check failed: ( reassembly_data->packet_data[packet_buffer_size - 1] == 0 ), function test_fragment_reassembly_buffer_zeroed, ... line 3219
   Then a final clean rebuild at the restored tip: 0 warnings, 7/7, all tests passed.

No behaviour change for valid configs: reliable_config_valid's checks are untouched, only their format strings; %.*s with precision 255 stops at the NUL for any terminated name, so log text is byte-identical for every name the tests and examples use. The terminator at :725 only overwrites byte [255], which nothing reads before the first NUL.

LOW (notes, nothing to change):

• The two-dot diff against main shows reliable.h losing the docs: document NULL-allocator guarantee on receive path #65 doc lines; the three-dot diff (origin/main...pr) of reliable.h is empty, so that is only the branch predating docs: document NULL-allocator guarantee on receive path #65. Local git merge origin/main at tip: clean, and reliable.h after the merge equals main's. Merge as is.
• A 256-byte unterminated name prints 255 chars in the rejection logs, matching what the terminated copy prints later. Consistent, and test_config_name_bounded_in_rejection_log pins exactly that (name_end - line - 1 == sizeof(name) - 1).
• The memset is one bounded write per new reassembly entry (header + num_fragments * fragment_size + 8, ~16 KB at defaults), the cost item 2 accepted.

[rowan-claude]

Read (Opus) at a38f88c

Verdict: APPROVE.

Second model, cold: whole diff against origin/main, no other reader's comment read first. The residual my f8d8ded read held on — the rejection logs and the alloc-failure log printing the caller's unterminated config->name — is closed. No HIGH, no MEDIUM.

security#26-3, the residual. The audit: "Latent over-read on a non-NUL-terminated config.name: reliable_printf("[%s]", config.name) reads past the 256-byte array if a caller fills all 256 bytes." reliable_config_valid runs before create copies the config, so its logs print the caller's own buffer — that was the gap.

• RELIABLE_MAX_NAME_CHARS (:554) is sizeof(...->name) - 1 = 255, against reliable.h:140 char name[256]. The cast binds before the -1, and the sizeof operand is unevaluated, so no deref.
• Argument order checked at all twelve sites: precision, then config->name, then the %ds — and in each format every %d follows the %.*s. grep -c '%\.\*s' = 12; no bare %s survives between :554 and :723.
• The create-time log (:760) prints endpoint->config.name, and the terminator at :725 is set immediately after endpoint->config = *config — before the first allocation and before that log. The earlier returns (reliable_checked_size, the INT_MAX guards) log nothing.
• Every other %s classified: :48 and :1942 are assert/check on literals, :88 is reliable_printf's own 4 KB buffer, and :915-:1496 all print endpoint->config.name or the name parameter of reliable_read_packet_header/reliable_read_fragment_header — fed at :1274 and :1391 from the terminated copy, and at :2114+ from string literals. None can reach an unterminated buffer.

My earlier PoC, re-run. Same shape: 256 bytes of x, no NUL, context set to 0x000000FFFFFFFFFF so exactly five non-zero bytes follow the array.

origin/main:  name logged = 261 bytes
a38f88c1:     name logged = 255 bytes

Red-first, all three tests. Reverted one site's width — "[%.*s] max_packet_size must be positive\n", RELIABLE_MAX_NAME_CHARS, config->name back to "[%s] ...", config->name — and rebuilt:

ERROR: AddressSanitizer: heap-buffer-overflow ... READ of size 369
    #1 vsnprintf
SUMMARY: AddressSanitizer: heap-buffer-overflow reliable.c:87 in reliable_printf

caught by test_config_name_bounded_in_rejection_log. Restored, green. Dropping the :1457 memset fails test_fragment_reassembly_buffer_zeroed at :3219; dropping the :725 terminator fails test_endpoint_name_terminated at :3800. All restored; tree clean at a38f88c.

security#26-2. The audit asked for zeroing "particularly given the (currently unreachable) early-return paths in reliable_store_fragment_data". The memset at :1457 uses the same packet_buffer_size in the same scope as the allocate_function call at :1442, and sits after the NULL check — right size, right place. The poison allocator makes the test a real check rather than a tautology.

Build. -DRELIABLE_SANITIZE=ON --clean-first: ctest 7/7, bin/test 27/27, ten consecutive runs clean. Zero warnings. Under -Wall -Wextra -Wformat=2 -Wpedantic -Wconversion, main and the PR both emit 14 — the same set; the lone -Wformat-nonliteral at :87 is pre-existing. No behaviour change for valid configs: [an ordinary endpoint name] max_packet_size must be positive, byte-identical on both sides.

LOW — notes, not change requests.

1. The :1457 memset now touches the whole reassembly buffer on a path a peer drives. It is bounded by config and is the same order as the allocation it follows (default 16 KB x 64 slots, ~1 MB), so no new amplification class. Naming it on the record only.
2. I verified on clang alone (Apple clang, arm64). The sizeof(((T*)NULL)->name) idiom is clean there, including under -Wnull-dereference; GCC and MSVC are left to CI.

[rowan-claude]

Merge on the record: macOS legs substituted by local macOS runs

At a38f88c eight hosted checks pass and the two macOS-15 legs (Debug, Release) have sat queued at zero seconds for fifty minutes on GitHub's macOS runner pool. Glenn, live at 23:58Z: "go ahead and merge with the local macOS runs on the record."

The substitute, run on the Studio (macOS, Apple clang), at this exact head, by the two cold readers: Debug with -DRELIABLE_SANITIZE=ON after cmake --build --clean-first, ctest 7/7 and bin/test 27 tests, exit 0 (Fable read, comment 5657035002); Debug+ASan/UBSan 10/10 and a clean Release build 7/7 (Opus read, comment 5657061372). Reads: Fable and Opus APPROVE at this head, Alex (security seat) APPROVE at f8d8ded with the residual repair re-read twice.

This is an exception named per Glenn's flexibility rule, not the path; the hosted macOS legs will run on main after the merge and are read back there.