Robustness batch from the 2026-09-13 audit (closes #190)

[rowan-claude]

Robustness batch from the 2026-09-13 audit. Four findings from issue #190, taken in the issue's order, each judged against the tree at origin/main rather than against the issue's prose. No wire format change; nothing here is reachable from unauthenticated network input.

Write-side checks in this family stay debug-only asserts. Everything added below is a release if plus a log line, because each guards a public entry point against a caller's configuration error — the same line netcode_client_send_packet already draws at netcode.c:3511.

---

1. Callback-NULL class — REAL, both halves

1a. receive_packet_override called with no NULL check

Confirmed at netcode.c:3263 (client) and netcode.c:5039 (server): under if ( config.override_send_and_receive ) the pointer is called directly. Its twin is checked — netcode.c:2768 calls send_packet_override only else if ( send_packet_override ) — so the asymmetry is real and it crashes in debug and release alike.

Fix (the issue's preferred shape, setup-time): netcode_client_create_dual and netcode_server_create_dual now refuse a config where override_send_and_receive is set and either override is NULL, with two new create error codes, NETCODE_CLIENT_CREATE_ERROR_MISSING_OVERRIDE_CALLBACK 7 and NETCODE_SERVER_CREATE_ERROR_MISSING_OVERRIDE_CALLBACK 8. One check in each create path; the call sites are then unreachable with a NULL pointer by construction.

Test: the new blocks in test_client_create_error / test_server_create_error.

Red (at origin/main + the tests):

test_client_create_error
check failed: ( netcode_client_create( "0.0.0.0:50000", &override_config, 0.0 ) == NULL ), function test_client_create_error, file netcode.c, line 7382

check failed: ( netcode_server_create( "127.0.0.1:40000", &override_config, 0.0 ) == NULL ), function test_server_create_error, file netcode.c, line 7465

Green: both pass, below.

1b. send_loopback_packet_callback called with no guard

Confirmed at netcode.c:3534 (client, no assert at all) and netcode.c:5255 (server, netcode_assert only, so nothing in release).

Create time cannot see this one — loopback is entered later and is optional — so the setup point is the entry into loopback itself: netcode_client_connect_loopback and netcode_server_connect_loopback_client. Both now refuse, in every build, when the callback is unset. That closes both call sites: client->loopback is set only in the first, server->client_loopback[i] only in the second.

No netcode_assert was added beside these two ifs on purpose: the new test drives exactly this condition and the suite runs in debug as well as release.

Test: new test_loopback_callback_required.

Red:

test_loopback_callback_required
check failed: ( netcode_client_loopback( client ) == 0 ), function test_loopback_callback_required, file netcode.c, line 10434

(With the check removed the same test reaches the send and takes the null call.)

2. Network simulator: unchecked allocation — REAL

Confirmed at netcode.c:2630-2631: the result of allocate_function is memcpy'd into with no NULL check, and it is the only unchecked config allocation in the file.

Fix: allocate into a local first and return on failure, so a failed allocation drops the new packet and leaves the entry already queued in that ring slot intact, rather than freeing it and copying into NULL.

Test: new test_network_simulator_allocation_failure — a toggled allocator that fails for one send, then a check that nothing was delivered and that the simulator still delivers once the allocator recovers.

Red (release build, single-test driver):

$ ./red_test_network_simulator_allocation_failure
exit=139

139 is SIGSEGV: the memcpy into NULL.

3. netcode_address_to_string buffer contract — REAL

Confirmed: NETCODE_MAX_ADDRESS_STRING_LENGTH was defined at netcode.c:50, not in netcode.h, while the function is declared at netcode.h:185. The internal writes do truncate (inet_ntop at netcode.c:340, snprintf at :353, :361, :370, :380), so there is no overflow inside — the defect is that a public-header-only caller cannot name the size it must allocate.

Fix: the constant moves to netcode.h beside NETCODE_MAX_PACKET_SIZE; the contract is documented in a /* */ block after the declaration. Signature unchanged, so the ports that mirror it are unaffected.

The red here is a compile, not a unit test — a header-only translation unit:

#include "netcode.h"
void print_address( struct netcode_address_t * address )
{
    char buffer[NETCODE_MAX_ADDRESS_STRING_LENGTH];
    netcode_address_to_string( address, buffer );
}

Red, against origin/main's netcode.h:

caller.c:6:17: error: use of undeclared identifier 'NETCODE_MAX_ADDRESS_STRING_LENGTH'
    6 |     char buffer[NETCODE_MAX_ADDRESS_STRING_LENGTH];
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Green, against this branch's netcode.h: compiles, rc=0.

4. Console RNG — REAL, documentation only

Confirmed at sodium/sodium.c:1407: under #elif defined(__ORBIS__) || defined(__PROSPERO__), randombytes_sysrandom_buf(void * const buf, const size_t size) has an empty body — it never writes buf. That is upstream libsodium's behaviour, not a local edit, and netcode takes every key and nonce from randombytes_buf.

Not testable from this tree (no console target in CI), so it is documented: a new "Console platforms need their own RNG" section in sodium/NOTES.md, and a new finding 4 in IMPLEMENTERS.md for ports whose crypto library has the same trap.

Not changed

Negative timeout_seconds and connection-response type-3 nonce reuse — both named in the issue as out of scope, and both still match STANDARD.md. No refactors, no style sweeps, nothing else touched.

---

Whole suite, once, at the end

cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build --parallel && ctest --test-dir build --output-on-failure — under 2 minutes (build and test together well under a minute on this bench), no new warnings.

Test project netcode/build
    Start 1: netcode_test
1/1 Test #1: netcode_test .....................   Passed    1.03 sec

100% tests passed out of 1

53 test functions, including test_network_simulator_allocation_failure and test_loopback_callback_required. The suite was also run from a Debug build (-DCMAKE_BUILD_TYPE=Debug), where the asserts are live: *** ALL TESTS PASSED ***.

Reads owed: one Fable cold read and one read from another line before merge.

C# mirror owed: netcode.cs#7.

🤖 Generated with Claude Code

[gafferongames]

Reviewed at b5091c5c0290be2f019929932149b291824b8af4 against base 20d1f504ef0cff4f5123ec015a835acfa20c051a.

Verdict: HOLD (1 defect found: memory leak in new test).

---

Finding: test_network_simulator_allocation_failure leaks 256 bytes in receive_packet_data[0]

• Location: netcode.c:7565
• Trigger: In test_network_simulator_allocation_failure:

  check( netcode_network_simulator_receive_packets( network_simulator, &to, 16, receive_packet_data, receive_packet_bytes, receive_from ) == 1 );
  
  netcode_network_simulator_destroy( network_simulator );

• Explanation: netcode_network_simulator_receive_packets transfers ownership of received packet buffers to the caller (setting pending_receive_packets[i].packet_data = NULL in the simulator, as seen in test_network_simulator_determinism:7635 and client_receive_packets:3315). When netcode_network_simulator_destroy is called, that packet is no longer inside the simulator, and the test never freed receive_packet_data[0].
• Witness: Hosted CI job sanitizers (asan+ubsan) #103728448453 fails under LeakSanitizer:

  ==2253==ERROR: LeakSanitizer: detected memory leaks
  Direct leak of 256 byte(s) in 1 object(s) allocated from:
      #0 malloc
      #1 test_toggle_allocate_function netcode.c:7317
      #2 netcode_network_simulator_queue_packet netcode.c:2623
      #3 netcode_network_simulator_send_packet netcode.c:2670
      #4 test_network_simulator_allocation_failure netcode.c:7561
  

• Repair: Add free( receive_packet_data[0] ); immediately after line 7565.

---

Other Robustness Items (Verified Clean)

1. receive_packet_override (client :2921, server :4200): Create-time refusal with NETCODE_CLIENT_CREATE_ERROR_MISSING_OVERRIDE_CALLBACK (7) and NETCODE_SERVER_CREATE_ERROR_MISSING_OVERRIDE_CALLBACK (8) in netcode.h. Network-input config checked at create time in all builds; clean tests.
2. send_loopback_packet_callback (client :3654, server :5396): Release check + error log at loopback entry points, preventing NULL function pointer calls.
3. Simulator allocation failure (:2623): Allocates to local before modifying slot; drops packet gracefully on NULL allocator return.
4. NETCODE_MAX_ADDRESS_STRING_LENGTH (netcode.h:139): Exported with full contract comment on buffer size and null termination.
5. Console RNG documentation (sodium/NOTES.md, IMPLEMENTERS.md): Clearly states stub RNG behavior on __ORBIS__ / __PROSPERO__ and requires registering a platform cryptographic RNG before netcode_init.

Once free( receive_packet_data[0] ); is added to test_network_simulator_allocation_failure, this will be a clean APPROVE.

[rowan-claude]

APPROVE

Cold read of head b5091c5 against origin/main (20d1f50), STYLE-C.md in mas-bandwidth/standard, and the ports that mirror this header. Every red below was reproduced by reverting that hunk alone on the branch and rebuilding netcode_test. No HOLD-grade finding. Three NOTEs, one of which needs a line in the PR body before merge.

1a. receive_packet_override unchecked — real, fix matches doctrine

Defect on main: netcode.c:3263 and :5039 call config.receive_packet_override under if ( override_send_and_receive ) with no check; the send twin is guarded at :2768 (else if ( send_packet_override )). Real in debug and release.

Rule: STYLE-C §5, "Config validation runs in every build: reliable_config_valid range-checks each field ... logs which one failed, and create returns NULL (reliable.c:554-618)." The new checks at netcode.c:2922 and :4200 are that shape exactly (if + error: log + NULL, error code recorded), and reliable_config_valid carries no assert beside its ifs, so no assert is owed here.

Red, hunk reverted: check failed: ( netcode_client_create( "0.0.0.0:50000", &override_config, 0.0 ) == NULL ), test_client_create_error, netcode.c:7410, exit 1.

Behaviour for a correct caller: unchanged. The one caller this refuses that main accepted is override_send_and_receive = 1 with send_packet_override = NULL: on main :3338 passes NULL as the override, :2768 falls through to netcode_socket_send_packet on a socket that was never created (:2876 skips creation), whose only guard is netcode_assert( socket->handle != 0 ) (:868) — so in release that caller's sends went to fd 0. Not a correct caller.

1b. Loopback callback — every path covered; NOTE on the missing assert

Paths: client->loopback = 1 is written at one line on main (:3635, inside netcode_client_connect_loopback); server->client_loopback[i] = 1 at one line (:5369, inside netcode_server_connect_loopback_client). Both call sites (:3534, :5255) branch on those flags, and nothing else sets them, so the two entry guards close both. netcode_client_send_packet also returns on state != CONNECTED, which the refused connect never reaches.

Red, hunk reverted: check failed: ( netcode_client_loopback( client ) == 0 ), test_loopback_callback_required, netcode.c:10464, exit 1.

NOTE (style, not a defect). Rule: STYLE-C §5, "What is checked in release. Anything from the network, and any public argument that could be out of range, gets an if" and then "Both, on purpose. The assert catches it in your debugger; the if catches it in the shipped build." The PR body drops the assert "on purpose" because the test drives the condition in debug. The suite already has the pattern for exactly that: STYLE-C §5 says netcode's handler "lets a custom handler continue ... and its tests use that to run the release guards inside a debug build", and this tree does it at netcode.c:7174-7189 (test_runtime_guards_assert_handler) and :6280-6328 (test_oor_assert_handler). Adding netcode_assert( client->config.send_loopback_packet_callback ) beside each new if and installing that handler in test_loopback_callback_required keeps "both, on purpose". Server side keeps its existing call-site assert at :5292; client call site still has none. Not blocking: the if is the release guard the rule requires.

2. Simulator allocation — real, red is the SIGSEGV, ring entry question

Defect on main: netcode.c:2629-2631, allocate_function result assigned then memcpy'd with no check.

Red, hunk reverted: ./build/bin/test exit 139; under a tty (script -q /dev/null) exit 11 with the last test name printed test_network_simulator_allocation_failure. So the red is the segfault in this test, not a later one.

On the "entry left intact" question: the fix returns before the free at :2631, before the slot writes, and before current_index++ at :2643. So the index is not advanced on failure and the premise of an advanced index with a dead slot does not arise. What is observable is only on the failure path: the entry that would have been evicted from that slot stays queued and still delivers if its delivery_time has not passed (ring of NETCODE_MAX_CLIENTS * 256 entries, :2502). For a caller whose allocator never fails, byte-identical behaviour. Fix is the minimal one.

3. NETCODE_MAX_ADDRESS_STRING_LENGTH — real, minimum correct, mirrors unaffected

Red, header-only TU against main's netcode.h: error: use of undeclared identifier 'NETCODE_MAX_ADDRESS_STRING_LENGTH'; green against the branch header, rc=0.

Widest write (netcode.c:324-380): IPv6 with port is [ + 45 (INET6_ADDRSTRLEN-1) + ]: + 5 = 53 chars + NUL; IPv4 with port 21; NONE 4. Every write is snprintf( buffer, NETCODE_MAX_ADDRESS_STRING_LENGTH, ... ) or inet_ntop( ..., buffer, NETCODE_MAX_ADDRESS_STRING_LENGTH ), so the documented "at least 256, always null terminated, truncated not overflowed" holds. Placement: STYLE-C §3, "In the header, a longer explanation is a /* */ block placed after the declaration it explains" — matches netcode.h:191-195; the constant sits beside NETCODE_MAX_PACKET_SIZE with the public constants (STYLE-C §1, "then the public constants").

Mirrors: netcode.go has a private maxAddressStringLength = 256 (netcode.go:69), netcode.cs and netcode.rs expose no such constant. No port change owed for #3. grep of README.md, STANDARD.md, BUILDING.md and mas-bandwidth/standard finds no list of public constants to update.

4. Console RNG docs — claim true, placement right

sodium/sodium.c:1407 is #elif defined(__ORBIS__) || defined(__PROSPERO__); the stub is randombytes_sysrandom_buf at :1420-1423, body empty, and randombytes_buf at :1873-1879 dispatches to it via implementation->buf. randombytes_set_implementation (:1777) can be called at any time; "before netcode_init" is the safe advice since netcode_init -> sodium_init installs the default via randombytes_init_if_needed. IMPLEMENTERS.md "## 4." follows the existing 1-3; NOTES.md section sits before "## Validation". No code change, none owed.

NOTE — public API change not named for the mirror (needs a line in the PR body)

Rule (this read's brief): a public API change is named in the PR body and mirrored wherever the family mirrors it. The body names the two new codes but says nothing about netcode.cs, which mirrors both enums by value: netcode.cs/src/Client.cs:41-56 (ClientCreateError, 0-5) and src/Server.cs:21-36 (ServerCreateError, 0-6). That mirror already lacks ALLOCATE_*_FAILED (6 and 7 on main); this PR widens the gap to two codes each. The same defect class is in the C# port unguarded: Client.cs:496 _config.ReceivePacketOverride!(...), Client.cs:729 _config.SendLoopbackPacket!(...), Server.cs:881 ReceivePacketOverride!. Ask: file the netcode.cs follow-up (enum values 6/7 and 7/8, plus the create-time and loopback-entry refusals) and cite it in the PR body before merge. netcode.rs and netcode.go do not mirror the enum.

What I ran

Fresh clone at b5091c5. cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build --parallel (0 warnings), then once: ctest --test-dir build --output-on-failure -> 1/1 Test #1: netcode_test ... Passed 0.71 sec, 100% tests passed out of 1. Debug build once (-DCMAKE_BUILD_TYPE=Debug): *** ALL TESTS PASSED ***. Three single-hunk reverts rebuilt with --target netcode_test (note: --target test is CMake's ctest alias and rebuilds nothing). Header-only compile of the PR body's caller.c against both headers. Shallow clones of netcode.cs, netcode.rs, netcode.go, reliable and standard for the mirror and rule checks.

Rowan's Fable cold read, head b5091c5

[gafferongames]

Delta read at 32909b26bc75ec672be0b3f1b6875238c2243b54 (+13/-2 against b5091c5):

• Loopback assertions verified: netcode_assert( client->config.send_loopback_packet_callback ); and netcode_assert( server->config.send_loopback_packet_callback ); properly accompany the release if + log per STYLE-C §5. test_loopback_callback_required correctly intercepts the assertion via test_runtime_guards_assert_handler and restores netcode_default_assert_handler.
• Standing finding remains: test_network_simulator_allocation_failure still omits free( receive_packet_data[0] ); after netcode_network_simulator_receive_packets, causing CI job sanitizers (asan+ubsan) #103729601182 to fail under LeakSanitizer.
• Repair: Add free( receive_packet_data[0] ); immediately after line 7565 in netcode.c.

Verdict: HOLD pending the leak repair.

[gafferongames]

Delta read at 200412b605dacd708ea4bf8312169829812fba5a (+4/-0 against 32909b2):

• Leak fix verified: test_toggle_free_function( NULL, receive_packet_data[0] ); added at line 7572 before netcode_network_simulator_destroy.
• All CI checks green: Run 34759566706 passed all 19 matrix jobs, including sanitizers (asan+ubsan) (Job 103729854849).
• All items complete:
  1. receive_packet_override client and server create-time refusal (codes 7 and 8).
  2. send_loopback_packet_callback guarded by debug assert + release check & log at entry points.
  3. Network simulator allocation check and graceful drop.
  4. NETCODE_MAX_ADDRESS_STRING_LENGTH public contract.
  5. Console RNG platform requirement documented.
  6. Clean tests across all configurations with zero leaks.

Clean to merge. APPROVE.