Follow-ups: MSVC /WX, no asserts on network input (fuzz-debug), 12 loopback suites onto the simulated network

.github/workflows/build.yml

@@ -444,7 +444,7 @@ jobs:
           persist-credentials: false
 
       - name: Configure CMake
-        run: cmake -B build -DMAFIANET_BUILD_SAMPLES=ON -DMAFIANET_BUILD_TESTS=ON
+        run: cmake -B build -DMAFIANET_BUILD_SAMPLES=ON -DMAFIANET_BUILD_TESTS=ON -DMAFIANET_WARNINGS_AS_ERRORS=ON
 
       - name: Build
         run: cmake --build build --config Release --parallel 4

.github/workflows/fuzz.yml

@@ -16,9 +16,16 @@ permissions:
   contents: read
 
 jobs:
+  # fuzz: optimized, NDEBUG, as shipped. fuzz-debug: RakAssert enabled, so any
+  # assert a remote can reach through crafted input is reported as a crash.
   fuzz:
     runs-on: ubuntu-latest
     timeout-minutes: 120
+    strategy:
+      fail-fast: false
+      matrix:
+        preset: [fuzz, fuzz-debug]
+    name: fuzz (${{ matrix.preset }})
     steps:
       - name: Checkout repository
         uses: actions/checkout@v4
@@ -30,15 +37,15 @@ jobs:
 
       - name: Build harnesses
         run: |
-          cmake --preset fuzz -G Ninja
-          cmake --build --preset fuzz
+          cmake --preset ${{ matrix.preset }} -G Ninja
+          cmake --build --preset ${{ matrix.preset }}
 
       - name: Restore corpus
         uses: actions/cache/restore@v4
         with:
           path: fuzz-corpus
-          key: fuzz-corpus-${{ github.run_id }}
-          restore-keys: fuzz-corpus-
+          key: fuzz-corpus-${{ matrix.preset }}-${{ github.run_id }}
+          restore-keys: fuzz-corpus-${{ matrix.preset }}-
 
       - name: Fuzz
         env:
@@ -48,7 +55,7 @@ jobs:
         run: |
           mkdir -p fuzz-artifacts
           status=0
-          for harness in build/fuzz/Tests/Fuzz/*_fuzz; do
+          for harness in build/${{ matrix.preset }}/Tests/Fuzz/*_fuzz; do
             name=$(basename "$harness" _fuzz)
             mkdir -p fuzz-corpus/"$name"
             cp -n Tests/Fuzz/corpus/"$name"/* fuzz-corpus/"$name"/ || true
@@ -64,11 +71,11 @@ jobs:
         uses: actions/cache/save@v4
         with:
           path: fuzz-corpus
-          key: fuzz-corpus-${{ github.run_id }}
+          key: fuzz-corpus-${{ matrix.preset }}-${{ github.run_id }}
 
       - name: Upload crash reproducers
         if: failure()
         uses: actions/upload-artifact@v4
         with:
-          name: fuzz-crashes
+          name: fuzz-crashes-${{ matrix.preset }}
           path: fuzz-artifacts/

CLAUDE.md

@@ -67,19 +67,20 @@ Available generators (run `cmake --help` for full list):
 | `MAFIANET_MINIMAL` | OFF | Compile out every optional plugin (core transport only); built by the `linux-minimal` CI job |
 | `MAFIANET_SANITIZER` | (empty) | `address+undefined` or `thread`; applied to every target. Prefer the `asan` / `tsan` presets |
 | `MAFIANET_USE_COMPILER_CACHE` | ON | Use sccache/ccache as compiler launcher when found on `PATH` |
-| `MAFIANET_WARNINGS_AS_ERRORS` | OFF | `-Werror` / `/WX` on MafiaNet's own targets (on in the Linux/macOS CI jobs) |
+| `MAFIANET_WARNINGS_AS_ERRORS` | OFF | `-Werror` / `/WX` on MafiaNet's own targets (on in every CI job except the oldest-toolchain Docker one) |
 | `MAFIANET_BUILD_FUZZERS` | OFF | libFuzzer harnesses in `Tests/Fuzz` (Clang); use the `fuzz` preset |
 | `MAFIANET_BUILD_BENCHMARKS` | OFF | google/benchmark microbenchmarks in `Benchmarks/`; use the `bench` preset |
 | `MAFIANET_COVERAGE` | OFF | `--coverage` instrumentation; use the `coverage` preset, report with gcovr |
 
 Presets (`CMakePresets.json`, CMake 3.25+): `debug`, `release`, `ci`, `minimal`, `asan`, `tsan`,
-`fuzz`, `bench`, `coverage`.
+`fuzz`, `fuzz-debug` (harnesses with RakAssert enabled), `bench`, `coverage`.
 `cmake --workflow --preset <debug|asan|tsan>` configures, builds and tests in one go; run
 sanitizer tests through `ctest --preset asan|tsan` so the sanitizer options and
 `ci/tsan.supp` are applied. Known by-design TSan races live in `ci/tsan.supp`; add an entry only
 for a race that is part of RakNet's lock-free getter model, never to silence a new bug.
 
-**Warnings:** MafiaNet's own code builds with zero `-Wall -Wextra` warnings on Clang and GCC, and
+**Warnings:** MafiaNet's own code builds with zero `-Wall -Wextra` warnings on Clang and GCC and zero `/W4`
+warnings on MSVC (C4251 is disabled), and
 CI enforces it (`MAFIANET_WARNINGS_AS_ERRORS`). Fix a new warning properly; suppress only with a
 narrow, commented `[[maybe_unused]]` or pragma, never by loosening the flags. Warnings are applied
 per target by `mafianet_enable_warnings()` (`cmake/MafiaNetHelpers.cmake`), never globally, so
@@ -91,6 +92,7 @@ Each is also built as `<name>_replay`, which replays `Tests/Fuzz/corpus/<name>/`
 `FuzzCorpus.<name>` on every compiler. When a fuzzer finds a crash, fix it and add the minimized
 reproducer to that corpus directory. Code that parses network input must not `RakAssert` on
 malformed data: a Debug build would then abort on whatever a peer sends — reject and drop instead.
+The `fuzz-debug` preset (run nightly) builds the harnesses with asserts enabled to find such cases.
 
 When adding or removing a plugin guarded by a `_RAKNET_SUPPORT_<Name>` flag, update
 `MAFIANET_OPTIONAL_FEATURES` in `Source/CMakeLists.txt` as well, or the CMake

CMakePresets.json

@@ -124,6 +124,15 @@
                 "MAFIANET_BUILD_FUZZERS": "ON",
                 "MAFIANET_SANITIZER": "address+undefined+fuzzer-no-link"
             }
+        },
+        {
+            "name": "fuzz-debug",
+            "inherits": "fuzz",
+            "displayName": "libFuzzer harnesses with RakAssert enabled",
+            "description": "Debug defines _DEBUG, so any RakAssert a remote can reach through crafted input aborts and is reported as a crash. Network input must never trip an assert.",
+            "cacheVariables": {
+                "CMAKE_BUILD_TYPE": "Debug"
+            }
         }
     ],
     "buildPresets": [
@@ -134,6 +143,7 @@
         { "name": "asan", "configurePreset": "asan" },
         { "name": "tsan", "configurePreset": "tsan" },
         { "name": "fuzz", "configurePreset": "fuzz" },
+        { "name": "fuzz-debug", "configurePreset": "fuzz-debug" },
         { "name": "coverage", "configurePreset": "coverage" },
         { "name": "bench", "configurePreset": "bench", "targets": ["MafiaNetBenchmarks"] }
     ],

Source/include/mafianet/peer.h

@@ -639,7 +639,10 @@ class RAK_DLL_EXPORT RakPeer : public RakPeerInterface, public RNS2EventHandler
 	void SetSocketFactory(RakNetSocket2Factory *factory);
 	/// No internal update thread: the caller drives the peer with
 	/// RunManualUpdate(). Shutdown(blockDuration) then flushes once instead of
-	/// waiting, since nothing else would advance the peer meanwhile.
+	/// waiting, since nothing else would advance the peer meanwhile. That flush
+	/// sends nothing if the clock has not moved since the last update (the
+	/// reliability layer skips a zero-length tick), so advance a test clock
+	/// before shutting down if the disconnection notification must go out.
 	void SetManualUpdate(bool manual);
 	/// One update-thread iteration: the user update callback, then the update
 	/// cycle. Only valid with SetManualUpdate(true).

Source/src/peer.cpp

@@ -6485,11 +6485,14 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream )
 
 			while ( bitSize > 0 )
 			{
-				// These types are for internal use and should never arrive from a network packet
+				// These types are for internal use and should never arrive from a network packet.
+				// A remote that sends one anyway gets it dropped: freed, and the rest of the
+				// queue still processed (this used to leak it and stall the queue, and to
+				// RakAssert in Debug).
 				if (data[0]==ID_CONNECTION_ATTEMPT_FAILED)
 				{
-					RakAssert(0);
-					bitSize=0;
+					rakFree_Ex(data, _FILE_AND_LINE_ );
+					bitSize = remoteSystem->reliabilityLayer.Receive( &data );
 					continue;
 				}
 
@@ -6555,7 +6558,7 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream )
 							// arrives in ID_SESSION_CONFIG_REQUEST and we answer it.
 							remoteSystem->connectMode=RemoteSystemStruct::EXCHANGING_SESSION_DATA;
 							remoteSystem->sessionConfigIsConnectingSide=false;
-							remoteSystem->sessionConfigStartTime=timeMS!=0 ? timeMS : MafiaNet::GetTimeMS();
+							remoteSystem->sessionConfigStartTime=timeMS!=0 ? (MafiaNet::TimeMS) timeMS : MafiaNet::GetTimeMS(); // compared with GetTimeMS(), 32-bit
 							PingInternal( systemAddress, true, MafiaNet::Reliability::Unreliable );
 
 							// Update again immediately after this tick so the ping goes out right away
@@ -6593,7 +6596,7 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream )
 							// Hold this until the session handshake completes; the bytes are copied because `data`
 							// is freed as soon as this branch returns.
 							ClearSessionConfig(remoteSystem);
-							remoteSystem->sessionConfigStartTime=timeMS!=0 ? timeMS : MafiaNet::GetTimeMS();
+							remoteSystem->sessionConfigStartTime=timeMS!=0 ? (MafiaNet::TimeMS) timeMS : MafiaNet::GetTimeMS(); // compared with GetTimeMS(), 32-bit
 							remoteSystem->withheldConnectionPacketData = (unsigned char*) rakMalloc_Ex(byteSize, _FILE_AND_LINE_);
 							if (remoteSystem->withheldConnectionPacketData!=0)
 							{
@@ -6741,7 +6744,7 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream )
 						// connecting side like the other replies, so a client cannot push one at a server.
 						if (remoteSystem->connectMode==RemoteSystemStruct::EXCHANGING_SESSION_DATA && remoteSystem->sessionConfigIsConnectingSide)
 						{
-							remoteSystem->sessionConfigStartTime=timeMS!=0 ? timeMS : MafiaNet::GetTimeMS();
+							remoteSystem->sessionConfigStartTime=timeMS!=0 ? (MafiaNet::TimeMS) timeMS : MafiaNet::GetTimeMS(); // compared with GetTimeMS(), 32-bit
 							packet=AllocPacket(byteSize, data, _FILE_AND_LINE_);
 							packet->bitSize = bitSize;
 							packet->systemAddress = systemAddress;
@@ -6848,7 +6851,7 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream )
 								// Transport is up, but the application is not told so until the session payloads have been
 								// exchanged. ID_CONNECTION_REQUEST_ACCEPTED is stashed and replayed from ID_SESSION_CONFIG.
 								remoteSystem->connectMode=RemoteSystemStruct::EXCHANGING_SESSION_DATA;
-								remoteSystem->sessionConfigStartTime=timeMS!=0 ? timeMS : MafiaNet::GetTimeMS();
+								remoteSystem->sessionConfigStartTime=timeMS!=0 ? (MafiaNet::TimeMS) timeMS : MafiaNet::GetTimeMS(); // compared with GetTimeMS(), 32-bit
 
 								// Bug: If A connects to B through R, A's firstExternalID is set to R. If A tries to send to R, sends to loopback because R==firstExternalID
 								// Correct fix is to specify in Connect() if target is through a proxy.
@@ -6863,7 +6866,7 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream )
 								// copied because `data` is freed as soon as this branch returns.
 								ClearSessionConfig(remoteSystem);
 								remoteSystem->sessionConfigIsConnectingSide=true;
-								remoteSystem->sessionConfigStartTime=timeMS!=0 ? timeMS : MafiaNet::GetTimeMS();
+								remoteSystem->sessionConfigStartTime=timeMS!=0 ? (MafiaNet::TimeMS) timeMS : MafiaNet::GetTimeMS(); // compared with GetTimeMS(), 32-bit
 								remoteSystem->withheldConnectionPacketData = (unsigned char*) rakMalloc_Ex(byteSize, _FILE_AND_LINE_);
 								if (remoteSystem->withheldConnectionPacketData!=0)
 								{

Source/src/reliability_layer.cpp

@@ -1051,8 +1051,8 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer(
 						}
 					} else { // holeCount>=receivedPackets.Size()
 						if (holeCount > (DatagramSequenceNumberType)1000000) {
-							RakAssert("Hole count too high. See reliability_layer.h" && 0);
-
+							// A peer jumped its message numbering far ahead: drop the
+							// packet. No RakAssert -- the remote chooses this value.
 							for (unsigned int messageHandlerIndex = 0; messageHandlerIndex < messageHandlerList.Size(); messageHandlerIndex++) {
 								messageHandlerList[messageHandlerIndex]->OnReliabilityLayerNotification("holeCount > 1000000", BYTES_TO_BITS(length), systemAddress, true);
 							}
@@ -1172,10 +1172,10 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer(
 						internalPacket->orderingChannel = 255; // Use 255 to designate not sequenced and not ordered
 
 					// Insert may free the fragment (duplicate or malformed): read its id first.
-					const SplitPacketIdType splitPacketId = internalPacket->splitPacketId;
+					const SplitPacketIdType fragmentMessageId = internalPacket->splitPacketId;
 					InsertIntoSplitPacketList( internalPacket, timeRead );
 
-					internalPacket = BuildPacketFromSplitPacketList( splitPacketId, timeRead,
+					internalPacket = BuildPacketFromSplitPacketList( fragmentMessageId, timeRead,
 						s, systemAddress, rnr, extraSocketOptions);
 
 					if ( internalPacket == 0 )
@@ -1269,10 +1269,10 @@ bool ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer(
 						internalPacket->orderingChannel = 255; // Use 255 to designate not sequenced and not ordered
 
 					// Insert may free the fragment (duplicate or malformed): read its id first.
-					const SplitPacketIdType splitPacketId = internalPacket->splitPacketId;
+					const SplitPacketIdType fragmentMessageId = internalPacket->splitPacketId;
 					InsertIntoSplitPacketList( internalPacket, timeRead );
 
-					internalPacket = BuildPacketFromSplitPacketList( splitPacketId, timeRead,
+					internalPacket = BuildPacketFromSplitPacketList( fragmentMessageId, timeRead,
 						s, systemAddress, rnr, updateBitStream);
 
 					if ( internalPacket == 0 )
@@ -2898,9 +2898,8 @@ InternalPacket* ReliabilityLayer::CreateInternalPacketFromBitStream(MafiaNet::Bi
 
 	if ( bitStreamSucceeded == false )
 	{
-		// If this hits, most likely the variable buff is too small in RunUpdateCycle in peer.cpp
-		RakAssert("Couldn't read all the data"  && 0);
-
+		// The packet claims more data than the datagram carries: malformed or
+		// hostile input, so drop it rather than RakAssert.
 		FreeInternalPacketData(internalPacket, _FILE_AND_LINE_ );
 		ReleaseToInternalPacketPool( internalPacket );
 		return 0;

Tests/Fuzz/CMakeLists.txt

@@ -43,6 +43,9 @@ foreach(harness_source IN LISTS MAFIANET_FUZZ_HARNESSES)
         add_executable(${harness} "${harness_source}" ${MAFIANET_FUZZ_SUPPORT})
         target_include_directories(${harness} PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/../Support")
         target_link_libraries(${harness} PRIVATE MafiaNet::MafiaNetStatic)
+        # Must match the library: several classes the harnesses put on the
+        # stack carry _DEBUG-only members (see Tests/CMakeLists.txt).
+        target_compile_definitions(${harness} PRIVATE $<$<CONFIG:Debug>:_DEBUG>)
         target_compile_options(${harness} PRIVATE -fsanitize=fuzzer)
         target_link_options(${harness} PRIVATE -fsanitize=fuzzer)
         set_target_properties(${harness} PROPERTIES FOLDER "Tests/Fuzz")

Tests/Integration/ComprehensiveConvertTests.cpp

@@ -191,7 +191,7 @@ TEST_F(ComprehensiveConvert, DoesALittleBitOfEverything)
 			bool broadcast;
 
 			//	data[0]=ID_RESERVED1+(randomMT()%10);
-			data[0]=ID_USER_PACKET_ENUM;
+			data[0]=(char)(unsigned char)ID_USER_PACKET_ENUM;
 			dataLength=3+(randomMT()%8000);
 			//			dataLength=600+(randomMT()%7000);
 			priority=(MafiaNet::Priority)(randomMT()%(int)MafiaNet::NUMBER_OF_PRIORITIES);