diff --git a/CLAUDE.md b/CLAUDE.md index 0905fd0..ba17e10 100644 --- a/CLAUDE.md +++ b/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. # CLAUDE.md diff --git a/netcode.c b/netcode.c index 9103d6c..e57bd70 100755 --- a/netcode.c +++ b/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 );