netcode_generate_connect_token: bound num_server_addresses at runtime (#173)

CLAUDE.md

@@ -48,6 +48,30 @@ Both were found by WRITING TESTS against a release build, not by reading the sou
 Neither is asserted in mas-bandwidth/apt's autopkgtest on purpose: the shipped `-DNDEBUG`
 library does not promise the first, and the second is unfixed. A packaging test that
 asserts an unshipped fix makes a faithful package look broken.
+
+THE WRITE/READ RULE — read this BEFORE reporting any assert as a missing bounds check
+Glenn, 2026-07-26: "intention is on write, user is responsible to not crash or do undefined
+behavior. asserts are there to help. callers responsibility. on read, obviously, we must
+check." Plus Postel: "be conservative in what you send, permissive in what you receive."
+  WRITE / caller-supplied -> the CALLER validates. An assert-only bound is the DESIGN, and
+    -DNDEBUG removing it is correct. Do not add runtime checks here.
+  READ / off the wire     -> the library checks at runtime, for SAFETY (bounds, sizes).
+    Permissive about format variation; strict about never crashing.
+This repo states the policy itself at netcode.c:4101-4103 -- "an out of range value here
+must not get through in release builds where asserts compile out" -- attached to the entry
+points that were hardened (3387, 4104, 4998, 5010, every client_index accessor). I audited
+this file, quoted that comment, and STILL filed the write-path asserts as defects. Do not
+repeat that.
+DELIBERATELY LENIENT, do not "fix": netcode_parse_address("[::1") returns OK and yields
+::1. That is permissive-on-receive and intended (#174 closed as by-design).
+netcode_generate_connect_token WAS genuinely missing its runtime check -- it was the one
+entry point skipped in the hardening pass above. Fixed; regression test
+test_generate_connect_token_out_of_range.
+GOTCHA: netcode_set_assert_function( NULL ) is a landmine. netcode_assert calls the pointer
+with NO null guard (netcode.h:350-356), so NULL turns the next failing assert into a crash.
+Restore &netcode_default_assert_handler instead -- which is what netcode.c:6984 already does.
+A custom handler MAY return (documented netcode.h:362-366); that is the only way a test with
+asserts compiled in can reach a release-build code path.
 <!-- HOT:END -->
 
 # CLAUDE.md

netcode.c

@@ -5285,6 +5285,16 @@ int netcode_generate_connect_token( int num_server_addresses,
     netcode_assert( num_server_addresses <= NETCODE_MAX_SERVERS_PER_CONNECT );
     netcode_assert( public_server_addresses );
     netcode_assert( internal_server_addresses );
+
+    // the parsed address arrays below are sized NETCODE_MAX_SERVERS_PER_CONNECT. an out of
+    // range value here must not get through in release builds where asserts compile out.
+    // every other public entry point already does this; this one was missed
+
+    if ( num_server_addresses <= 0 || num_server_addresses > NETCODE_MAX_SERVERS_PER_CONNECT )
+    {
+        netcode_printf( NETCODE_LOG_LEVEL_ERROR, "error: number of server addresses must be in [1,%d], got %d\n", NETCODE_MAX_SERVERS_PER_CONNECT, num_server_addresses );
+        return NETCODE_ERROR;
+    }
     netcode_assert( private_key );
     netcode_assert( user_data );
     netcode_assert( output_buffer );
@@ -6003,6 +6013,63 @@ static void test_address()
 #define TEST_CONNECT_TOKEN_EXPIRY   30
 #define TEST_TIMEOUT_SECONDS        15
 
+static int test_oor_asserts_fired = 0;
+
+static void test_oor_assert_handler( NETCODE_CONST char * condition, NETCODE_CONST char * function, NETCODE_CONST char * file, int line )
+{
+    (void) condition; (void) function; (void) file; (void) line;
+    test_oor_asserts_fired++;
+    // deliberately RETURNS. netcode.h documents that a custom handler may do this and
+    // execution continues past the failed assert -- which is the only way to reach the
+    // release-build code path from a test binary that has asserts compiled in.
+}
+
+static void test_generate_connect_token_out_of_range()
+{
+    // netcode_generate_connect_token parses into arrays sized NETCODE_MAX_SERVERS_PER_CONNECT.
+    // The bounds used to be assert-only, so -DNDEBUG release builds -- which is what ships,
+    // and what Debian packages -- wrote past a stack array. Every other public entry point
+    // already paired its asserts with a runtime check; this one was missed.
+    //
+    // The asserts fire first by design, so this installs a handler that returns in order to
+    // reach the runtime check underneath them. Without the fix, execution continues into the
+    // parse loop and writes out of bounds instead of returning NETCODE_ERROR.
+
+    uint8_t private_key[NETCODE_KEY_BYTES];
+    uint8_t user_data[NETCODE_USER_DATA_BYTES];
+    uint8_t connect_token[NETCODE_CONNECT_TOKEN_BYTES];
+
+    memset( private_key, 0, sizeof( private_key ) );
+    memset( user_data, 0, sizeof( user_data ) );
+
+    NETCODE_CONST char * server_address = "127.0.0.1:40000";
+
+    test_oor_asserts_fired = 0;
+    netcode_set_assert_function( &test_oor_assert_handler );
+
+    check( netcode_generate_connect_token( 0, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR );
+    check( netcode_generate_connect_token( -1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR );
+    check( netcode_generate_connect_token( NETCODE_MAX_SERVERS_PER_CONNECT + 1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_ERROR );
+
+    // In a DEBUG build the asserts must still have fired -- they are the caller's debug aid
+    // and this test must not silently prove they were removed. In a RELEASE build they are
+    // compiled to ((void)0) by design, so requiring them there would fail the very
+    // configuration this fix exists for. That asymmetry IS the point of the fix.
+
+#ifndef NDEBUG
+    check( test_oor_asserts_fired > 0 );
+#endif // #ifndef NDEBUG
+
+    // restore the DEFAULT handler, not NULL: netcode_assert calls the pointer with no null
+    // guard, so NULL would turn the next failing assert anywhere in the suite into a crash
+
+    netcode_set_assert_function( &netcode_default_assert_handler );
+
+    // and an in-range call still succeeds, so the guard cannot pass by rejecting everything
+
+    check( netcode_generate_connect_token( 1, &server_address, &server_address, 30, 5, 1000ULL, TEST_PROTOCOL_ID, private_key, user_data, connect_token ) == NETCODE_OK );
+}
+
 static void test_connect_token()
 {
     // generate a connect token
@@ -9628,6 +9695,7 @@ void netcode_test()
         RUN_TEST( test_address );
         RUN_TEST( test_sequence );
         RUN_TEST( test_connect_token );
+        RUN_TEST( test_generate_connect_token_out_of_range );
         RUN_TEST( test_challenge_token );
         RUN_TEST( test_connection_request_packet );
         RUN_TEST( test_connection_denied_packet );