Testing & tooling modernization: fuzzing, simulated network, zero warnings, parallel tests (+ security fixes) by Segfaultd · Pull Request #76 · MafiaHub/MafiaNet

Segfaultd · 2026-10-17T14:59:20Z

Modernizes the testing and tooling setup. The new fuzzers and the warning cleanup turned up real bugs, including remotely triggerable memory-safety bugs, which are fixed here with regression tests. Commits are split by topic; the security fixes are in their own commit (fix(security): …) so they can be reviewed or backported alone.

Security fixes (any connected peer could trigger these)

Found by the new libFuzzer harnesses, or by reading the code around what they hit:

ReliabilityLayer split packets. Every field of a fragment header comes from the remote.
- The first fragment preallocated one pointer per claimed fragment, so one datagram claiming 2³² fragments meant a 32 GB allocation.
- Later fragments weren't checked against the first fragment's count, so a write past the fragment table was possible: a heap overflow in Release.
- Fragments that weren't whole bytes overran the reassembly buffer.
- The summed message length could wrap.
- A rejected or duplicate fragment was freed and then read by the caller: use-after-free.
- Fragment storage now grows with what is received, not what is claimed. Inconsistent fragments are dropped, and lengths are summed in 64 bits.
BitStream.
- Read(char*, n) and ReadAlignedBytes checked n << 3 in 32 bits. Lengths of 512 MB and up wrapped, passed the check, and memcpy read past the stream.
- ReadAlignedBytesSafeAlloc leaked its buffer when the read failed.
- ReadDelta branched on an uninitialized flag.
Datagram parsing no longer RakAsserts on malformed input. A Debug build could be aborted by whatever a peer sends.

Zero compiler warnings, enforced

-Wall -Wextra is applied to MafiaNet's own targets only (mafianet_enable_warnings), and Apple Clang and GCC 13 are at zero in Debug and Release. The Linux, macOS and sanitizer CI jobs now build with MAFIANET_WARNINGS_AS_ERRORS. The cleanup exposed and fixed:

a use-after-free in TCPInterface on every outgoing TCP connect;
Table::Cell's copy constructor freeing an uninitialized pointer;
a one-byte overrun in the vsnprintf_s shim;
an infinite loop in FileList::PopulateDataFromDisk;
RelayPlugin ignoring the sender's reliability, priority and channel for group messages;
test plugins whose callbacks no longer matched PluginInterface2 and were never called;
socketType left uninitialized outside AllocRNS2;
Windows GUID collisions when peers are created back to back.

New tooling

Fuzzing (Tests/Fuzz, the fuzz preset). Three harnesses: bit_stream, reliability_layer, and peer_datagram (a whole server driven from offline messages through a connected session, seeded with a recorded real session).
- Each harness is also built as <name>_replay on every compiler, which replays the checked-in corpus, including the crash reproducers, as unit tests.
- CI fuzzes each harness for 60 s per PR, and for 15 min nightly (fuzz.yml) with a corpus that persists between runs.
Simulated network (Tests/Support/SimulatedNetwork). It runs whole RakPeers with no sockets, no threads and a virtual clock: handshake, loss, reordering, MTU discovery, timeouts and limits are deterministic and take milliseconds, so they run in the unit suite.
- It needs three opt-in library seams: SetTimeSourceOverride, RakNetSocket2Factory and RakPeer::SetManualUpdate. All are inert when unused.
Parallel integration suite. Every integration test binds OS-assigned ports, so RUN_SERIAL is gone. The suite takes about 40 s with -j8 locally, and Windows now runs it too.
Benchmarks (Benchmarks/, google/benchmark v1.9.5, the bench preset), smoke-run in CI.
Coverage (the coverage preset): a gcovr summary in the job summary and an HTML artifact.
FetchContent: rnnoise now uses FetchContent_MakeAvailable, replacing the deprecated FetchContent_Populate that a future CMake would remove.

Trade-offs

Split-message reassembly is about 8% slower in the in-memory microbenchmark (std::map instead of a flat array). That's the cost of not trusting the claimed fragment count. Small messages are unchanged.
Windows keeps /W4 warnings non-fatal until CI shows how many there are. MSVC wasn't measured locally.

Verification

macOS Debug: 264 unit tests pass. The 74 integration tests pass in parallel (-j8) with no retries.
Linux GCC 13: Debug and Release with -Werror build cleanly, and all 338 tests pass.
Linux sanitizers:
- ASan + UBSan: all tests pass, with zero reports in any attempt (retries included).
- TSan: all tests pass, with zero reports beyond ci/tsan.supp.
Fuzzing after the fixes: bit_stream 4.0M runs, reliability_layer 970k, peer_datagram 159k, with no crashes.

Not in this PR

clang-format: a separate PR (a one-time reformat listed in .git-blame-ignore-revs).
Remaining RakAsserts on wire input: others may remain deeper in the receive path. The fuzz builds use NDEBUG and can't see them.
Moving loopback integration tests onto the simulated network: a follow-up.

FetchContent_Populate with declared details is deprecated (CMP0169) and slated for removal. Neither rnnoise nor its model archive has a CMakeLists.txt, so MakeAvailable just downloads them, as before. Also sets DOWNLOAD_EXTRACT_TIMESTAMP on the model archive (CMP0135).

Found by the new libFuzzer harnesses (Tests/Fuzz) and by reading the code around what they hit. Every issue is reachable by any connected peer. ReliabilityLayer split packets (every header field comes from the remote): - The first fragment of a split message preallocated one pointer per claimed fragment: a single datagram claiming 2^32 fragments made the receiver allocate 32 GB (remote denial of service). - Later fragments were not checked against the first fragment's count, so a fragment claiming a larger count and index was written past the fragment table (heap overflow in Release; only a RakAssert in Debug). - Reassembly sized its buffer from the total bit length but copied each fragment at a byte offset, so non-final fragments that were not whole bytes overran it. - The summed message length was 32-bit and could wrap. - A fragment rejected (or a duplicate) was freed by InsertIntoSplitPacketList and then read by the caller to look up its message: use-after-free. SplitPacketSort now stores fragments as they arrive (memory follows what was received, not what was claimed), rejects fragments that disagree with the first one or that are non-final and not whole bytes, and reassembly sums lengths in 64 bits. Datagram parsing no longer RakAsserts on malformed input, so a Debug build cannot be aborted remotely; it drops the packet, including one claiming more data than a datagram can hold. Reassembly is ~8% slower in the in-memory microbenchmark (std::map vs a flat array). BitStream: - Read(char*, n) and ReadAlignedBytes compared against n << 3 in 32 bits; lengths of 512 MB and up wrapped, passed the check and memcpy'd past the stream. - ReadAlignedBytesSafeAlloc returned its buffer allocated but unfilled when the read failed; callers that free only on success leaked it. - ReadDelta / ReadCompressedDelta branched on an uninitialized flag when the read hit the end of the stream.

…exposed mafianet_enable_warnings() applies -Wall -Wextra (/W4 on MSVC) to the MafiaNet libraries and test targets only, never to fetched dependencies. MAFIANET_WARNINGS_AS_ERRORS makes them errors. Apple Clang and GCC 13 now report zero warnings in Debug and Release. Real bugs behind the warnings, each fixed with a test where testable: - TCPInterface ConnectionAttemptLoop read s->bindAddress after deleting s (use-after-free on every outgoing TCP connect). - Table::Cell's copy constructor freed its own uninitialized pointer and left it uninitialized when the source held no data, so the copy's destructor freed garbage. - RelayPlugin relayed every group message as High / ReliableOrdered / channel 0, ignoring what the sender asked for. - The vsnprintf_s shim (Linux, macOS) wrote one byte past the buffer when _TRUNCATE truncated, which FormatString hit on any long string. - FileList::PopulateDataFromDisk(..., false, false, ...) never advanced to the next file and leaked the open handle: an infinite loop. - The test plugins' OnNewConnection / OnClosedConnection no longer matched PluginInterface2 and were never called. - RakVoice::SetOrderingChannels could not reject negative channels where char is unsigned (ARM). Also: fread/fgets results are checked (a short read skips the file, as an unopenable one did); sprintf -> snprintf where the bound is known; SystemAddress and RakNetGUID get explicit defaulted copy operations (their assignment operators were plain member-wise copies; the out-of-line SystemAddress::operator= is kept for the exported symbol); MSVC-only C4996 pragmas removed (the build already defines _CRT_SECURE_NO_DEPRECATE); dead code behind unused-but-set variables removed; mixed-indentation sites re-indented.

Fuzzing (Tests/Fuzz): harnesses for BitStream deserialization and for the datagram parser of a connected ReliabilityLayer, the code that parses what remote peers send. Each <name>_fuzz.cpp is built two ways: - <name>_replay, always with the tests, on every compiler: replays the checked-in corpus (seeds plus minimized crash reproducers) as the ctest case FuzzCorpus.<name>, label unit. A fixed crash stays fixed everywhere. - <name>_fuzz, a libFuzzer binary, with MAFIANET_BUILD_FUZZERS=ON (Clang). The fuzz preset builds them under ASan + UBSan with the library instrumented for coverage. Benchmarks (Benchmarks/, google/benchmark v1.9.5, MAFIANET_BUILD_BENCHMARKS, bench preset): BitStream serialization and the ReliabilityLayer send / ack / split path between two layers joined in memory. Coverage: MAFIANET_COVERAGE (--coverage) and the coverage preset; report with gcovr --root . --filter Source/ build/coverage.

…/macOS - fuzz: each libFuzzer harness for 60 s from the checked-in corpus under ASan + UBSan; a crash uploads its reproducer. - fuzz.yml (nightly, and on demand): 15 minutes per harness, with the grown corpus cached from night to night. - benchmarks: build and smoke-run the microbenchmarks, upload the JSON. Shared runners are too noisy to gate on numbers. - coverage: both suites under --coverage, gcovr summary in the job summary and the HTML report as an artifact. Informational. - linux-native, linux-minimal, macos and the sanitizer jobs build with MAFIANET_WARNINGS_AS_ERRORS. Windows (MSVC /W4) and the Ubuntu 22.04 Docker job keep warnings visible but non-fatal until measured. - Documents why the Docker job stays next to linux-native: it is the oldest supported toolchain (GCC 11, CMake 3.22).

Tests/Support/SimulatedNetwork drives real peers through connect, data transfer, MTU discovery, loss, reordering and timeouts with no sockets, no threads and a virtual clock, so those scenarios are deterministic, take milliseconds, and belong in the unit suite instead of loopback integration tests. Per-direction link rules model loss, latency, jitter, a path MTU or a dark link; randomness is a seeded LCG. Library seams, all opt-in and inert for normal use: - SetTimeSourceOverride (get_time.h): route GetTimeUS to a test clock. One atomic load per time query when unset. - RakNetSocket2Factory + RNS2T_USER (socket2.h): RakPeer::SetSocketFactory makes Startup take its sockets from a factory instead of binding OS sockets; IsBerkleySocket() is now false only for such sockets. - RakPeer::SetManualUpdate / RunManualUpdate: no internal update thread; the caller runs the update cycle. Shutdown(blockDuration) and GetSocket(s) work without the thread in that mode. Fixes found on the way: - RakNetSocket2 never initialized socketType outside AllocRNS2, so IsBerkleySocket() on any other subclass read garbage. - On Windows, Get64BitUniqueRandomNumber used only timer jitter, which collides for peers created back to back (and always under a virtual clock); it now mixes in random_device and a counter like POSIX. SimulatedPeerTests covers the handshake, ReliableOrdered under 20% loss with reordering, split reassembly, disconnect notification, timeout on a dark link, MTU discovery under a path ceiling, the incoming-connection limit, and run-to-run determinism.

peer_datagram drives a RakPeer server on the simulated network with datagrams from a remote that need not follow the protocol: offline messages, the handshake and, from the recorded session seed, everything a connected peer can send. Each input is a fresh, deterministic run.

Every integration test now binds ports the OS assigns (TestHelpers::StartOnLoopback / BoundPort; StandardServerPrep included), so RUN_SERIAL is gone and the suite runs under ctest -j: 74 tests in ~40 s locally with no retries, against several minutes serially. A taken port can no longer fail a test either (ComprehensiveConvert failed on a macOS runner that way, with Connect() returning INVALID_PARAMETER after an unchecked Startup). Every Startup() result is now asserted. The file-transfer tests also got per-run temp directories, which two parallel runs would otherwise share. Tests that check a peer's own addresses (LocalIsConnected, SystemAddressAndGuid) still bind all interfaces, on port 0. PeerBuilder's port-clash case gets a free port from the OS and then clashes on it.

…docs - ctest runs with --parallel in every job (2 under sanitizers), now that no test binds a fixed port. - Windows runs the integration suite too; it was build-only because of the shared fixed ports. - building.rst and CLAUDE.md document the warning policy, fuzzing (and adding crash reproducers to the corpus), benchmarks, coverage, the simulated network as the default home for peer-level tests, and the ephemeral-port rule.

…ability fuzz harness Constructing the per-datagram std::vector from an offset range made GCC 13 at -O2 warn about freeing a pointer with a nonzero offset, which -Werror turned into a build failure in the Linux Release job. An exactly sized heap copy keeps the ASan coverage and avoids the warning.

coderabbitai · 2026-10-17T14:59:33Z

Important

Review skipped

Too many files!

This PR contains 122 files, which is 22 over the limit of 100.

To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch.

Upgrade to a paid plan to raise the limit.

This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 3259c0a6-bd40-489a-ab34-99d17c36c733

📥 Commits

Reviewing files that changed from the base of the PR and between 7e3a194 and aeb2020.

📒 Files selected for processing: 122

.github/workflows/build.yml
.github/workflows/fuzz.yml
Benchmarks/CMakeLists.txt
Benchmarks/bit_stream_bench.cpp
Benchmarks/reliability_layer_bench.cpp
CLAUDE.md
CMakeLists.txt
CMakePresets.json
Dockerfile
Source/CMakeLists.txt
Source/include/mafianet/bit_stream.h
Source/include/mafianet/dr_sha1.h
Source/include/mafianet/ds_linked_list.h
Source/include/mafianet/ds_map.h
Source/include/mafianet/ds_ordered_list.h
Source/include/mafianet/get_time.h
Source/include/mafianet/peer.h
Source/include/mafianet/relay_plugin.h
Source/include/mafianet/reliability_layer.h
Source/include/mafianet/replica_manager3.h
Source/include/mafianet/socket2.h
Source/include/mafianet/types.h
Source/src/bit_stream.cpp
Source/src/cc_sliding_window.cpp
Source/src/data_compressor.cpp
Source/src/ds_table.cpp
Source/src/email_sender.cpp
Source/src/file_list.cpp
Source/src/fully_connected_mesh2.cpp
Source/src/get_time.cpp
Source/src/gets.cpp
Source/src/http_connection.cpp
Source/src/incremental_read_interface.cpp
Source/src/linux_adapter.cpp
Source/src/log_command_parser.cpp
Source/src/network_id_manager.cpp
Source/src/osx_adapter.cpp
Source/src/packet_logger.cpp
Source/src/packet_output_window_logger.cpp
Source/src/peer.cpp
Source/src/relay_plugin.cpp
Source/src/reliability_layer.cpp
Source/src/replica_manager3.cpp
Source/src/signaled_event.cpp
Source/src/sleep.cpp
Source/src/socket2.cpp
Source/src/socket2_berkley.cpp
Source/src/socket2_windows_linux.cpp
Source/src/socket_layer.cpp
Source/src/statistics.cpp
Source/src/statistics_history.cpp
Source/src/string.cpp
Source/src/super_fast_hash.cpp
Source/src/table_serializer.cpp
Source/src/tcp_interface.cpp
Source/src/types.cpp
Source/src/voice.cpp
Source/src/wstring.cpp
Tests/CMakeLists.txt
Tests/Fuzz/CMakeLists.txt
Tests/Fuzz/bit_stream_fuzz.cpp
Tests/Fuzz/corpus/bit_stream/numbers
Tests/Fuzz/corpus/bit_stream/regress-safe-alloc-leak
Tests/Fuzz/corpus/bit_stream/strings
Tests/Fuzz/corpus/bit_stream/vectors_and_blobs
Tests/Fuzz/corpus/peer_datagram/handshake
Tests/Fuzz/corpus/peer_datagram/offline_ping
Tests/Fuzz/corpus/peer_datagram/session
Tests/Fuzz/corpus/reliability_layer/acks
Tests/Fuzz/corpus/reliability_layer/ordered_split_sequenced
Tests/Fuzz/corpus/reliability_layer/regress-split-fragment-use-after-free
Tests/Fuzz/peer_datagram_fuzz.cpp
Tests/Fuzz/reliability_layer_fuzz.cpp
Tests/Fuzz/replay_main.cpp
Tests/Integration/ComprehensiveConvertTests.cpp
Tests/Integration/ConnectWithSocketTests.cpp
Tests/Integration/CrossConnectionConvertTests.cpp
Tests/Integration/DirectoryDeltaTransferStreamingLiveTests.cpp
Tests/Integration/DisconnectReasonTests.cpp
Tests/Integration/DroppedConnectionConvertTests.cpp
Tests/Integration/EightPeerTests.cpp
Tests/Integration/FileListTransferWindowLiveTests.cpp
Tests/Integration/LocalIsConnectedTests.cpp
Tests/Integration/ManyClientsOneServerBlockingTests.cpp
Tests/Integration/ManyClientsOneServerDeallocateBlockingTests.cpp
Tests/Integration/ManyClientsOneServerNonBlockingTests.cpp
Tests/Integration/MaximumConnectTests.cpp
Tests/Integration/MiscellaneousTestsTests.cpp
Tests/Integration/OfflineMessagesConvertTests.cpp
Tests/Integration/PacketAndLowLevelTests.cpp
Tests/Integration/PeerBuilderTests.cpp
Tests/Integration/PeerConnectDisconnectTests.cpp
Tests/Integration/PeerConnectDisconnectWithCancelPendingTests.cpp
Tests/Integration/PeerHandleTests.cpp
Tests/Integration/PingTestsTests.cpp
Tests/Integration/RelayPluginGroupMessageLiveTests.cpp
Tests/Integration/ReliableOrderedConvertedTests.cpp
Tests/Integration/SecurityFunctionsTests.cpp
Tests/Integration/SessionConfigLiveTests.cpp
Tests/Integration/SystemAddressAndGuidTests.cpp
Tests/Integration/TypedSendLiveTests.cpp
Tests/Support/CommonFunctions.cpp
Tests/Support/CommonFunctions.h
Tests/Support/DebugTools.cpp
Tests/Support/PacketChangerPlugin.h
Tests/Support/PacketDropPlugin.cpp
Tests/Support/PacketDropPlugin.h
Tests/Support/RakTimer.cpp
Tests/Support/SimulatedNetwork.cpp
Tests/Support/SimulatedNetwork.h
Tests/Support/TestHelpers.cpp
Tests/Support/TestHelpers.h
Tests/Unit/BitStreamBoundsTests.cpp
Tests/Unit/FileListDirectoryTests.cpp
Tests/Unit/GuidUtilTests.cpp
Tests/Unit/ReliabilityLayerSplitPacketTests.cpp
Tests/Unit/SafeStringShimTests.cpp
Tests/Unit/SimulatedPeerTests.cpp
Tests/Unit/TableCellTests.cpp
cmake/FetchVoiceDependencies.cmake
cmake/MafiaNetHelpers.cmake
docs/getting-started/building.rst

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

- DOWNLOAD_EXTRACT_TIMESTAMP only exists from CMake 3.24; on 3.22 (the Ubuntu 22.04 job) it was read as part of URL_HASH and configure failed. Pass it only where supported. - SimulatedNetwork.cpp used std::back_inserter without <iterator>, which MSVC does not pull in transitively. - Coverage counters were updated concurrently by the network threads and came out negative, which gcovr rejects: build with -fprofile-update=atomic, and have gcovr warn rather than abort if it still sees one.

Dropping it from the lambda capture silenced Clang's unused-capture warning but MSVC requires a non-static local to be captured. A static constant needs no capture on either compiler.

Segfaultd added 10 commits October 17, 2026 16:00

Segfaultd added 2 commits October 17, 2026 17:06

test: make GuidUtil's iteration count static for MSVC

aeb2020

Dropping it from the lambda capture silenced Clang's unused-capture warning but MSVC requires a non-static local to be captured. A static constant needs no capture on either compiler.

Segfaultd merged commit 006d369 into master Oct 17, 2026
12 checks passed

Segfaultd deleted the chore/tooling-modernization branch October 17, 2026 15:31

Segfaultd mentioned this pull request Oct 17, 2026

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

Open

Navigation Menu

Repository navigation

Testing & tooling modernization: fuzzing, simulated network, zero warnings, parallel tests (+ security fixes) - #76

Testing & tooling modernization: fuzzing, simulated network, zero warnings, parallel tests (+ security fixes)#76
Segfaultd merged 12 commits into
masterfrom
chore/tooling-modernization

Segfaultd commented Oct 17, 2026

Uh oh!

coderabbitai Bot commented Oct 17, 2026 •

edited

Loading

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Segfaultd commented Oct 17, 2026

Security fixes (any connected peer could trigger these)

Zero compiler warnings, enforced

New tooling

Trade-offs

Verification

Not in this PR

Uh oh!

coderabbitai Bot commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Oct 17, 2026 •

edited

Loading