From eee1437dbe6bb0e7f2b7603760e40bc7879cd31f Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Mon, 31 Aug 2026 21:57:27 +0200 Subject: [PATCH 1/2] fix(reliability): detect in-session path-MTU black holes and step the MTU down The connection handshake probes the path MTU in one direction only and the negotiated size is then applied to both directions for the life of the connection. A tunnel whose return path carries less than the probed direction (OpenVPN and friends drop, rather than fragment, datagrams over their ceiling; see OpenVPN/openvpn#823) black-holes every large datagram one way while the handshake's small packets sail through: the peer connects, then hangs on the first split payload while the reliability layer resends the same too-large datagram until the connection times out. This is the follow-up deferred by the MAXIMUM_MTU_SIZE cap in #55. Detection (MtuBlackHole.h, portable and unit tested on every platform): a reliable packet that has gone unacked through MTU_BLACKHOLE_RESEND_THRESHOLD transmissions, and would actually shrink if the MTU dropped a rung, is the black-hole signature -- packets that already fit the next rung indicate loss, not size, so ordinary packet loss can never shrink a healthy connection's MTU. The ladder is now shared with the handshake probe in RakPeer.cpp. Recovery (ReliabilityLayer): step currentMtuBytes one rung down, shrink the congestion manager's datagram ceiling, and re-split every queued message that no longer fits. Split messages are rebuilt from the refcounted whole-message block their fragments share (fragments now record the original length in the sender-only splitOriginalByteLength) and re-sent under a fresh splitPacketId with their ordering indices preserved, so the receiver's stalled partial channel is superseded in place. Oversized unsplit reliable messages are simply split; unreliable ones are dropped, as the network already was doing. Tested with a hermetic two-ReliabilityLayer harness over a fake socket with fully simulated time (Tests/Unit/ReliabilityLayerBlackHoleTests.cpp): the one-directional black hole, recovery down to the bottom rung, ordered delivery and ack receipts across a re-split, and the false-positive guard under plain loss. Verified on macOS and on Linux in Debug and Release. --- Source/CMakeLists.txt | 2 + Source/include/mafianet/InternalPacket.h | 9 + Source/include/mafianet/MTUSize.h | 19 +- Source/include/mafianet/MtuBlackHole.h | 62 ++++ Source/include/mafianet/ReliabilityLayer.h | 27 ++ Source/src/MtuBlackHole.cpp | 39 +++ Source/src/RakPeer.cpp | 16 +- Source/src/ReliabilityLayer.cpp | 233 ++++++++++++++ Tests/Unit/MtuBlackHoleTests.cpp | 97 ++++++ Tests/Unit/ReliabilityLayerBlackHoleTests.cpp | 294 ++++++++++++++++++ 10 files changed, 784 insertions(+), 14 deletions(-) create mode 100644 Source/include/mafianet/MtuBlackHole.h create mode 100644 Source/src/MtuBlackHole.cpp create mode 100644 Tests/Unit/MtuBlackHoleTests.cpp create mode 100644 Tests/Unit/ReliabilityLayerBlackHoleTests.cpp diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index 9a95e202a..0648a8965 100644 --- a/Source/CMakeLists.txt +++ b/Source/CMakeLists.txt @@ -52,6 +52,7 @@ set(MAFIANET_SOURCES src/LogCommandParser.cpp src/MessageFilter.cpp src/MmsgBatch.cpp + src/MtuBlackHole.cpp src/NatPunchthroughClient.cpp src/NatPunchthroughServer.cpp src/NatTypeDetectionClient.cpp @@ -206,6 +207,7 @@ set(MAFIANET_HEADERS include/mafianet/MessageFilter.h include/mafianet/MessageIdentifiers.h include/mafianet/MmsgBatch.h + include/mafianet/MtuBlackHole.h include/mafianet/MTUSize.h include/mafianet/NativeFeatureIncludes.h include/mafianet/NativeFeatureIncludesOverrides.h diff --git a/Source/include/mafianet/InternalPacket.h b/Source/include/mafianet/InternalPacket.h index 53312c502..c468f2425 100644 --- a/Source/include/mafianet/InternalPacket.h +++ b/Source/include/mafianet/InternalPacket.h @@ -122,6 +122,15 @@ struct InternalPacket : public InternalPacketFixedSizeTransmissionHeader /// If the reliability type requires a receipt, then return this number with it uint32_t sendReceiptSerial; + /// Sender-side bookkeeping, never transmitted: for a split fragment, the + /// byte length of the whole original message. All fragments of one message + /// share their data through refCountedData->sharedDataBlock, so together + /// with this length the original message can be rebuilt and re-split at a + /// smaller size when an in-session MTU black hole forces the negotiated + /// MTU down (see ReliabilityLayer::ReSplitOversizedMessages). Zero for + /// anything that is not a split fragment. + unsigned int splitOriginalByteLength; + // Used for the resend queue // Linked list implementation so I can remove from the list via a pointer, without finding it in the list InternalPacket *resendPrev, *resendNext,*unreliablePrev,*unreliableNext; diff --git a/Source/include/mafianet/MTUSize.h b/Source/include/mafianet/MTUSize.h index 0c9a2f931..7c55b6730 100644 --- a/Source/include/mafianet/MTUSize.h +++ b/Source/include/mafianet/MTUSize.h @@ -36,16 +36,17 @@ /// The handshake probes the path in one direction only -- the connecting peer /// pads ID_OPEN_CONNECTION_REQUEST_1 down the mtuSizes ladder in RakPeer.cpp and /// the accepting peer echoes back whatever size arrived -- and the result is -/// then frozen for the life of the connection and applied to BOTH directions. -/// Nothing detects a path-MTU black hole afterwards: a datagram too large for -/// the return path is resent at the same size until the connection times out. -/// -/// So the top rung has to be a size that survives whatever encapsulation a peer -/// sits behind, without that peer having to discover it. 1400 clears WireGuard +/// then applied to BOTH directions. A black hole the handshake missed (a tunnel +/// dropping large datagrams on the return path only, or a path that shrinks +/// mid-session) is caught later by the in-session step-down in +/// ReliabilityLayer/MtuBlackHole.h, which walks the same ladder downward and +/// re-splits queued messages -- but that detection costs several +/// retransmission timeouts, so the top rung still has to be a size most +/// tunnelled peers survive without discovering anything. 1400 clears WireGuard /// (1420) and typical IPSec/IKEv2 (1400) tunnels on a 1500-byte path; anything -/// smaller is still found by the ladder. The cost is ~6% of payload per -/// datagram on a clean path, against a class of connection failure that looks -/// to the user like the server ignoring them. +/// smaller is still found by the ladder or the step-down. The cost is ~6% of +/// payload per datagram on a clean path, against a class of connection failure +/// that looks to the user like the server ignoring them. /// /// Lowering this further is safe. RAISING it is not, on its own: two peers /// converge on the smaller of their two caps only because the accepting side diff --git a/Source/include/mafianet/MtuBlackHole.h b/Source/include/mafianet/MtuBlackHole.h new file mode 100644 index 000000000..f4207fece --- /dev/null +++ b/Source/include/mafianet/MtuBlackHole.h @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +/// \file +/// \brief In-session MTU black-hole detection. +/// +/// The connection handshake probes the path MTU in one direction only +/// (RakPeer.cpp pads ID_OPEN_CONNECTION_REQUEST_1 down the ladder) and the +/// negotiated size is then applied to both directions for the life of the +/// connection. A tunnel whose return path carries less than the probed +/// direction -- OpenVPN and friends drop, rather than fragment, datagrams over +/// their ceiling -- black-holes every large datagram one way while the +/// handshake's small packets sail through. The connection establishes, then +/// hangs on the first split payload. +/// +/// These helpers are the portable decision logic the reliability layer uses to +/// break that loop: recognise the black-hole signature on a resent packet and +/// pick the next rung to drop to. Pure functions, no I/O, unit tested on every +/// platform (Tests/Unit/MtuBlackHoleTests.cpp). + +#ifndef __MTU_BLACK_HOLE_H +#define __MTU_BLACK_HOLE_H + +#include + +#include "mafianet/Export.h" + +namespace MafiaNet { + +/// The MTU rung ladder, in bytes including UDP/IP headers, highest first. +/// The same rungs the connection handshake probes (mtuSizes in RakPeer.cpp); +/// MTU_LADDER[0] must equal MAXIMUM_MTU_SIZE so a step-down never lands on a +/// rung the handshake could not have negotiated. +const int MTU_LADDER_SIZE = 4; +extern RAK_DLL_EXPORT const int MTU_LADDER[MTU_LADDER_SIZE]; + +/// How many unacked transmissions of a too-large packet it takes before the +/// connection's MTU steps down one rung. Resends are RTO-spaced with backoff, +/// so this represents several round-trip times of a specific packet failing +/// while the connection is otherwise alive -- ordinary loss does not +/// concentrate on one packet like that. +const uint32_t MTU_BLACKHOLE_RESEND_THRESHOLD = 4; + +/// The ladder rung strictly below \a currentMtuBytes, or 0 when already at or +/// below the bottom rung. +int RAK_DLL_EXPORT NextLowerMtu(int currentMtuBytes); + +/// Whether a reliable packet occupying \a requiredDatagramBytes on the wire +/// (datagram payload plus UDP/IP headers) that has gone unacked through +/// \a timesSent transmissions is evidence of an MTU black hole worth stepping +/// down for. False when the packet already fits the next rung down: resending +/// it at the same size after a step-down would change nothing on the wire, so +/// its failures indicate loss, not a black hole. +bool RAK_DLL_EXPORT ShouldStepDownMtu(uint32_t timesSent, int requiredDatagramBytes, int currentMtuBytes); + +} // namespace MafiaNet + +#endif // __MTU_BLACK_HOLE_H diff --git a/Source/include/mafianet/ReliabilityLayer.h b/Source/include/mafianet/ReliabilityLayer.h index c48779547..1351f9527 100644 --- a/Source/include/mafianet/ReliabilityLayer.h +++ b/Source/include/mafianet/ReliabilityLayer.h @@ -177,6 +177,12 @@ class ReliabilityLayer// /// \param[out] the value passed to SetTimeoutTime MafiaNet::TimeMS GetTimeoutTime(void); + /// The connection's current MTU in bytes, including UDP/IP headers. Starts + /// at the handshake-negotiated size passed to Reset() and steps down the + /// ladder in MtuBlackHole.h when an in-session path-MTU black hole is + /// detected. Never rises again for the life of the connection. + int GetCurrentMtuBytes(void) const; + /// Packets are read directly from the socket layer and skip the reliability layer because unconnected players do not use the reliability layer /// This function takes packet data after a player has been confirmed as connected. /// \param[in] buffer The socket data @@ -324,6 +330,24 @@ class ReliabilityLayer// /// Split the passed packet into chunks under MTU_SIZE bytes (including headers) and save those new chunks void SplitPacket( InternalPacket *internalPacket ); + /// In-session path-MTU black-hole recovery: drop currentMtuBytes one rung + /// down the ladder in MtuBlackHole.h, shrink the congestion manager's + /// datagram ceiling to match, and re-split every queued message that no + /// longer fits. Called from Update() when a too-large reliable packet has + /// burnt its resend budget without an ack (ShouldStepDownMtu). + void StepDownMtuAfterBlackHole(void); + + /// Rebuild and re-split, at the current (lowered) MTU, every queued message + /// with a packet too large for one datagram: split messages are + /// reconstructed from the shared data block their fragments reference and + /// re-split under a fresh splitPacketId (the receiver's partial channel for + /// the old id never completes and is superseded because the ordering + /// indices are preserved); oversized unsplit reliable messages are simply + /// split; oversized unsplit unreliable messages are dropped, as the network + /// was already free to drop them. Must only run when + /// packetsToSendThisUpdate is empty, since it frees queued packets. + void ReSplitOversizedMessages(void); + /// Insert a packet into the split packet list void InsertIntoSplitPacketList( InternalPacket * internalPacket, CCTimeType time ); @@ -578,6 +602,9 @@ class ReliabilityLayer// #if USE_SLIDING_WINDOW_CONGESTION_CONTROL==1 MafiaNet::CCRakNetSlidingWindow congestionManager; + // Current wire MTU in bytes including UDP/IP headers. Seeded from Reset()'s + // mtuSize, stepped down by black-hole detection. See GetCurrentMtuBytes(). + int currentMtuBytes; #else MafiaNet::CCRakNetUDT congestionManager; #endif diff --git a/Source/src/MtuBlackHole.cpp b/Source/src/MtuBlackHole.cpp new file mode 100644 index 000000000..68b0365fa --- /dev/null +++ b/Source/src/MtuBlackHole.cpp @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + */ + +#include "mafianet/MtuBlackHole.h" + +#include "mafianet/MTUSize.h" + +namespace MafiaNet { + +const int MTU_LADDER[MTU_LADDER_SIZE] = {MAXIMUM_MTU_SIZE, 1280, 1024, 576}; + +int NextLowerMtu(int currentMtuBytes) +{ + for (int i = 0; i < MTU_LADDER_SIZE; i++) + { + if (MTU_LADDER[i] < currentMtuBytes) + return MTU_LADDER[i]; + } + return 0; +} + +bool ShouldStepDownMtu(uint32_t timesSent, int requiredDatagramBytes, int currentMtuBytes) +{ + if (timesSent < MTU_BLACKHOLE_RESEND_THRESHOLD) + return false; + const int nextLower = NextLowerMtu(currentMtuBytes); + if (nextLower == 0) + return false; + // Only a packet that would actually shrink is evidence of a black hole: + // one that already fits the next rung would go out unchanged after a + // step-down, so its failures indicate loss, not size. + return requiredDatagramBytes > nextLower; +} + +} // namespace MafiaNet diff --git a/Source/src/RakPeer.cpp b/Source/src/RakPeer.cpp index 7fe90ab6f..f1821f236 100644 --- a/Source/src/RakPeer.cpp +++ b/Source/src/RakPeer.cpp @@ -41,6 +41,7 @@ #include #include "mafianet/GetTime.h" #include "mafianet/MessageIdentifiers.h" +#include "mafianet/MtuBlackHole.h" #include "mafianet/DS_HuffmanEncodingTree.h" #include "mafianet/Rand.h" #include "mafianet/PluginInterface2.h" @@ -104,10 +105,6 @@ extern void Console2GetIPAndPort(unsigned int, char *, unsigned short *, unsigne #endif -static const int NUM_MTU_SIZES=4; - - - // Probed high to low while connecting: the connecting peer pads // ID_OPEN_CONNECTION_REQUEST_1 to a rung and steps down when nothing comes back, // so the negotiated MTU is the largest rung that survived the path. Each rung is @@ -118,7 +115,11 @@ static const int NUM_MTU_SIZES=4; // covers heavier or stacked encapsulation; 576 is the dial-up floor. The gap // from MAXIMUM_MTU_SIZE straight to 1200 that used to sit here meant a peer one // byte over the top rung gave up ~20% of its payload capacity to find that out. -static const int mtuSizes[NUM_MTU_SIZES]={MAXIMUM_MTU_SIZE, 1280, 1024, 576}; +// +// The ladder itself lives in MtuBlackHole.h: the in-session black-hole +// step-down (ReliabilityLayer) walks the same rungs this handshake probes. +static const int NUM_MTU_SIZES=MafiaNet::MTU_LADDER_SIZE; +static const int *const mtuSizes=MafiaNet::MTU_LADDER; // How many connection attempts are spent on each rung of mtuSizes before // stepping down. @@ -6279,6 +6280,11 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream ) else remoteSystem->reliabilityLayer.Update( remoteSystem->rakNetSocket, systemAddress, remoteSystem->MTUSize, timeNS, maxOutgoingBPS, pluginListNTS, &rnr, updateBitStream ); // systemAddress only used for the internet simulator test + // The reliability layer steps the negotiated MTU down when it + // detects an in-session path-MTU black hole; keep the value + // GetMTUSize() reports in sync with what is actually on the wire. + remoteSystem->MTUSize = remoteSystem->reliabilityLayer.GetCurrentMtuBytes(); + // Check for failure conditions if ( remoteSystem->reliabilityLayer.IsDeadConnection() || ((remoteSystem->connectMode==RemoteSystemStruct::DISCONNECT_ASAP || remoteSystem->connectMode==RemoteSystemStruct::DISCONNECT_ASAP_SILENTLY) && remoteSystem->reliabilityLayer.IsOutgoingDataWaiting()==false) || diff --git a/Source/src/ReliabilityLayer.cpp b/Source/src/ReliabilityLayer.cpp index 9e094fa16..59541f5e4 100644 --- a/Source/src/ReliabilityLayer.cpp +++ b/Source/src/ReliabilityLayer.cpp @@ -20,6 +20,7 @@ #include "mafianet/ReliabilityLayer.h" #include "mafianet/MmsgBatch.h" +#include "mafianet/MtuBlackHole.h" #include "mafianet/GetTime.h" #include "mafianet/SocketLayer.h" #include "mafianet/PluginInterface2.h" @@ -424,6 +425,13 @@ void ReliabilityLayer::Reset(bool resetVariables, int mtuSize, bool _useSecurity #endif // LIBCAT_SECURITY congestionManager.Init(MafiaNet::GetTimeUS(), mtuSize - UDP_HEADER_SIZE); } + currentMtuBytes = mtuSize; +} + +//------------------------------------------------------------------------------------------------------- +int ReliabilityLayer::GetCurrentMtuBytes(void) const +{ + return currentMtuBytes; } //------------------------------------------------------------------------------------------------------- @@ -455,6 +463,7 @@ void ReliabilityLayer::InitializeVariables() memset( &heapIndexOffsets, 0, sizeof( heapIndexOffsets ) ); statistics.connectionStartTime = MafiaNet::GetTimeUS(); + currentMtuBytes = MAXIMUM_MTU_SIZE; splitPacketId = 0; elapsedTimeSinceLastUpdate=0; throughputCapCountdown=0; @@ -1957,6 +1966,25 @@ void ReliabilityLayer::UpdateInternal( RakNetSocket2 *s, SystemAddress &systemAd dhf.hasBAndAS=false; ResetPacketsAndDatagrams(); + // In-session path-MTU black-hole detection. The handshake probed the + // path in one direction only; a tunnel that drops large datagrams on + // the other direction leaves the head of the resend queue burning its + // resend budget on a packet that can never arrive. Checked only here, + // before any packet is pushed this tick, because the step-down frees + // queued packets and packetsToSendThisUpdate must not hold pointers to + // them. + if (IsResendQueueEmpty()==false) + { + InternalPacket *stuck = resendLinkedListHead; + if (time - stuck->nextActionTime < (((CCTimeType)-1)/2)) + { + const int requiredDatagramBytes = (int) BITS_TO_BYTES(stuck->headerLength + stuck->dataBitLength) + + (int) DatagramHeaderFormat::GetDataHeaderByteLength() + UDP_HEADER_SIZE; + if (ShouldStepDownMtu(stuck->timesSent, requiredDatagramBytes, currentMtuBytes)) + StepDownMtuAfterBlackHole(); + } + } + int transmissionBandwidth = congestionManager.GetTransmissionBandwidth(time, timeSinceLastTick, unacknowledgedBytes,dhf.isContinuousSend); int retransmissionBandwidth = congestionManager.GetRetransmissionBandwidth(time, timeSinceLastTick, unacknowledgedBytes,dhf.isContinuousSend); if (retransmissionBandwidth>0 || transmissionBandwidth>0) @@ -3094,6 +3122,7 @@ void ReliabilityLayer::SplitPacket( InternalPacket *internalPacket ) internalPacketArray[ splitPacketIndex ]->splitPacketIndex = splitPacketIndex; internalPacketArray[ splitPacketIndex ]->splitPacketId = splitPacketId; internalPacketArray[ splitPacketIndex ]->splitPacketCount = internalPacket->splitPacketCount; + internalPacketArray[ splitPacketIndex ]->splitOriginalByteLength = dataByteLength; RakAssert(internalPacketArray[ splitPacketIndex ]->dataBitLengthsplitPacketCount ); @@ -3131,6 +3160,209 @@ void ReliabilityLayer::SplitPacket( InternalPacket *internalPacket ) rakFree_Ex(internalPacketArray, _FILE_AND_LINE_ ); } +//------------------------------------------------------------------------------------------------------- +// In-session path-MTU black-hole recovery. Documented in ReliabilityLayer.h. +//------------------------------------------------------------------------------------------------------- +void ReliabilityLayer::StepDownMtuAfterBlackHole(void) +{ + const int nextMtu = NextLowerMtu(currentMtuBytes); + if (nextMtu == 0) + return; + currentMtuBytes = nextMtu; + congestionManager.SetMTU((uint32_t) (nextMtu - UDP_HEADER_SIZE)); + ReSplitOversizedMessages(); +} +//------------------------------------------------------------------------------------------------------- +void ReliabilityLayer::ReSplitOversizedMessages(void) +{ + const BitSize_t maxDatagramBits = GetMaxDatagramSizeExcludingMessageHeaderBits(); + unsigned int i; + InternalPacket *p; + + DataStructures::List staleSplitIds; + DataStructures::List rebuiltMessages; + DataStructures::List resendVictims; + + // Rebuild a full-length copy of the message pkt belongs to, before any of + // its packets are freed. Returns 0 (leaving the queues untouched) when the + // rebuild is impossible, so a failure degrades to the old stalled-resend + // behaviour rather than losing a reliable message. + auto rebuildMessage = [this](InternalPacket *pkt) -> InternalPacket* { + unsigned char *source; + unsigned int byteLength; + if (pkt->splitPacketCount > 0) + { + // Fragments carry a slice of a shared, refcounted copy of the whole + // message; splitOriginalByteLength is its full length. + RakAssert(pkt->allocationScheme == InternalPacket::REF_COUNTED); + RakAssert(pkt->refCountedData != 0 && pkt->splitOriginalByteLength > 0); + if (pkt->allocationScheme != InternalPacket::REF_COUNTED || pkt->refCountedData == 0 || pkt->splitOriginalByteLength == 0) + return 0; + source = pkt->refCountedData->sharedDataBlock; + byteLength = pkt->splitOriginalByteLength; + } + else + { + source = pkt->data; + byteLength = (unsigned int) BITS_TO_BYTES(pkt->dataBitLength); + } + unsigned char *copy = (unsigned char*) rakMalloc_Ex(byteLength, _FILE_AND_LINE_); + if (copy == 0) + { + notifyOutOfMemory(_FILE_AND_LINE_); + return 0; + } + memcpy(copy, source, byteLength); + InternalPacket *rebuilt = AllocateFromInternalPacketPool(); + if (rebuilt == 0) + { + rakFree_Ex(copy, _FILE_AND_LINE_); + notifyOutOfMemory(_FILE_AND_LINE_); + return 0; + } + // Keep reliability, priority, ordering/sequencing indices and receipt + // serial: the receiver's ordered channel is waiting on exactly this + // ordering index, so the re-split message slots into the stream where + // the original stalled. + *rebuilt = *pkt; + AllocInternalPacketData(rebuilt, copy); + rebuilt->refCountedData = 0; + rebuilt->dataBitLength = BYTES_TO_BITS(byteLength); + rebuilt->splitPacketCount = 0; + rebuilt->splitPacketIndex = 0; + rebuilt->splitPacketId = 0; + rebuilt->splitOriginalByteLength = 0; + rebuilt->messageNumberAssigned = false; + rebuilt->timesSent = 0; + rebuilt->nextActionTime = 0; + rebuilt->retransmissionTime = 0; + return rebuilt; + }; + + auto isStaleId = [&staleSplitIds](SplitPacketIdType id) -> bool { + for (unsigned int k = 0; k < staleSplitIds.Size(); k++) + if (staleSplitIds[k] == id) + return true; + return false; + }; + + // An oversized fragment marks its whole message stale: every sibling is + // pulled from the queues below and the message re-sent re-split under a + // fresh splitPacketId. The receiver's partial channel for the old id never + // completes and is simply superseded. + auto noteStaleSplit = [&](InternalPacket *pkt) { + if (isStaleId(pkt->splitPacketId)) + return; + InternalPacket *rebuilt = rebuildMessage(pkt); + if (rebuilt == 0) + return; + staleSplitIds.Push(pkt->splitPacketId, _FILE_AND_LINE_); + rebuiltMessages.Push(rebuilt, _FILE_AND_LINE_); + }; + + // Drop a not-yet-sent packet in place. The outgoing buffer is a heap, so + // entries cannot be unlinked by identity; the pop path already discards + // data==0 tombstones (and settles the send-buffer statistics then). + auto tombstoneOutgoing = [this](InternalPacket *pkt) { + FreeInternalPacketData(pkt, _FILE_AND_LINE_); + pkt->data = 0; + pkt->allocationScheme = InternalPacket::NORMAL; + pkt->refCountedData = 0; + RemoveFromUnreliableLinkedList(pkt); + }; + + // Pass 1: find every message with a packet that no longer fits a datagram. + if (resendLinkedListHead) + { + p = resendLinkedListHead; + do + { + if (GetMessageHeaderLengthBits(p) + p->dataBitLength > maxDatagramBits) + { + if (p->splitPacketCount > 0) + noteStaleSplit(p); + else + { + InternalPacket *rebuilt = rebuildMessage(p); + if (rebuilt) + { + rebuiltMessages.Push(rebuilt, _FILE_AND_LINE_); + resendVictims.Push(p, _FILE_AND_LINE_); + } + } + } + p = p->resendNext; + } while (p != resendLinkedListHead); + } + for (i = 0; i < outgoingPacketBuffer.Size(); i++) + { + p = outgoingPacketBuffer[i]; + if (p->data == 0) + continue; + if (GetMessageHeaderLengthBits(p) + p->dataBitLength <= maxDatagramBits) + continue; + if (p->splitPacketCount > 0) + noteStaleSplit(p); + else + { + bool isReliable = + p->reliability != MafiaNet::Reliability::Unreliable && + p->reliability != MafiaNet::Reliability::UnreliableSequenced && + p->reliability != MafiaNet::Reliability::UnreliableWithAckReceipt; + if (isReliable) + { + InternalPacket *rebuilt = rebuildMessage(p); + if (rebuilt == 0) + continue; + rebuiltMessages.Push(rebuilt, _FILE_AND_LINE_); + } + // An unreliable message the path already black-holed is not owed + // delivery; dropping it here is what the network was doing anyway. + tombstoneOutgoing(p); + } + } + + // Pass 2: sweep every packet of the stale messages out of both queues -- + // including small tail fragments that still fit, since the whole message + // is re-sent under its new id. + if (staleSplitIds.Size() > 0) + { + if (resendLinkedListHead) + { + p = resendLinkedListHead; + do + { + if (p->splitPacketCount > 0 && isStaleId(p->splitPacketId)) + resendVictims.Push(p, _FILE_AND_LINE_); + p = p->resendNext; + } while (p != resendLinkedListHead); + } + for (i = 0; i < outgoingPacketBuffer.Size(); i++) + { + p = outgoingPacketBuffer[i]; + if (p->data != 0 && p->splitPacketCount > 0 && isStaleId(p->splitPacketId)) + tombstoneOutgoing(p); + } + } + + // Unlink the resend-list victims the way an ack would, minus the receipt. + for (i = 0; i < resendVictims.Size(); i++) + { + p = resendVictims[i]; + if (resendBuffer[p->reliableMessageNumber & (uint32_t) RESEND_BUFFER_ARRAY_MASK] == p) + resendBuffer[p->reliableMessageNumber & (uint32_t) RESEND_BUFFER_ARRAY_MASK] = 0; + statistics.messagesInResendBuffer--; + statistics.bytesInResendBuffer -= BITS_TO_BYTES(p->dataBitLength); + RemoveFromList(p, true); + FreeInternalPacketData(p, _FILE_AND_LINE_); + ReleaseToInternalPacketPool(p); + } + + // Queue the rebuilt messages, split at the new, smaller size. + for (i = 0; i < rebuiltMessages.Size(); i++) + SplitPacket(rebuiltMessages[i]); +} + //------------------------------------------------------------------------------------------------------- // Insert a packet into the split packet list //------------------------------------------------------------------------------------------------------- @@ -3788,6 +4020,7 @@ InternalPacket* ReliabilityLayer::AllocateFromInternalPacketPool(void) ip->allocationScheme=InternalPacket::NORMAL; ip->data=0; ip->timesSent=0; + ip->splitOriginalByteLength=0; return ip; } //------------------------------------------------------------------------------------------------------- diff --git a/Tests/Unit/MtuBlackHoleTests.cpp b/Tests/Unit/MtuBlackHoleTests.cpp new file mode 100644 index 000000000..e537ba453 --- /dev/null +++ b/Tests/Unit/MtuBlackHoleTests.cpp @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + * + * Hermetic unit tests for the in-session MTU black-hole step-down decision. + * + * The connection handshake probes the path MTU in one direction only and the + * result is then applied to both directions for the life of the connection. + * A tunnel (OpenVPN, WireGuard, ...) whose *return* path carries less than the + * probed direction silently drops every datagram over its ceiling: the peer + * connects, then hangs on the first split payload while the reliability layer + * resends the same too-large datagram until the connection times out. + * + * ShouldStepDownMtu is the decision that breaks that loop: a reliable packet + * that has gone unacked through enough transmissions, and that would actually + * shrink if the MTU dropped a rung, indicates a black hole rather than plain + * loss. The damaging direction is the false positive -- ordinary packet loss + * must never shrink a healthy connection's MTU, so packets that already fit + * the next rung down can never trigger a step-down no matter how often they + * are resent. + */ + +#include + +#include "mafianet/MtuBlackHole.h" +#include "mafianet/MTUSize.h" + +using namespace MafiaNet; + +TEST(MtuBlackHole, LadderTopIsTheNegotiationCeiling) +{ + // The in-session ladder must start where the handshake ladder starts, or a + // step-down could move to a rung the handshake would never have negotiated. + EXPECT_EQ(MAXIMUM_MTU_SIZE, MTU_LADDER[0]); +} + +TEST(MtuBlackHole, NextLowerMtuWalksTheLadder) +{ + EXPECT_EQ(1280, NextLowerMtu(1400)); + EXPECT_EQ(1024, NextLowerMtu(1280)); + EXPECT_EQ(576, NextLowerMtu(1024)); +} + +TEST(MtuBlackHole, NextLowerMtuStopsAtTheBottomRung) +{ + EXPECT_EQ(0, NextLowerMtu(576)); + EXPECT_EQ(0, NextLowerMtu(400)); +} + +TEST(MtuBlackHole, NextLowerMtuFromBetweenRungsPicksTheRungStrictlyBelow) +{ + // Defensive: the negotiated MTU is normally a ladder value, but a peer built + // with a custom MAXIMUM_MTU_SIZE can negotiate anything up to the cap. + EXPECT_EQ(1280, NextLowerMtu(1300)); + EXPECT_EQ(576, NextLowerMtu(1000)); +} + +TEST(MtuBlackHole, StepsDownWhenALargePacketExhaustsItsResendBudget) +{ + // A datagram that fills the negotiated 1400-byte MTU has failed + // MTU_BLACKHOLE_RESEND_THRESHOLD times: this is the black-hole signature. + EXPECT_TRUE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD, 1400, 1400)); +} + +TEST(MtuBlackHole, DoesNotStepDownBeforeTheResendBudgetIsExhausted) +{ + EXPECT_FALSE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD - 1, 1400, 1400)); +} + +TEST(MtuBlackHole, DoesNotStepDownForAPacketThatAlreadyFitsTheNextRung) +{ + // Resending a small packet at the same size after a step-down changes + // nothing on the wire, so its failures say "loss", not "black hole". + // This is the false positive that would shrink healthy connections under + // ordinary packet loss. + EXPECT_FALSE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD, 1280, 1400)); + EXPECT_FALSE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD * 10, 100, 1400)); +} + +TEST(MtuBlackHole, StepsDownForAPacketJustOverTheNextRung) +{ + EXPECT_FALSE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD, 1280, 1400)); + EXPECT_TRUE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD, 1281, 1400)); +} + +TEST(MtuBlackHole, DoesNotStepDownBelowTheBottomRung) +{ + EXPECT_FALSE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD * 10, 576, 576)); +} + +TEST(MtuBlackHole, StepsDownFromIntermediateRungs) +{ + EXPECT_TRUE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD, 1280, 1280)); + EXPECT_TRUE(ShouldStepDownMtu(MTU_BLACKHOLE_RESEND_THRESHOLD, 1024, 1024)); +} diff --git a/Tests/Unit/ReliabilityLayerBlackHoleTests.cpp b/Tests/Unit/ReliabilityLayerBlackHoleTests.cpp new file mode 100644 index 000000000..b7fbec394 --- /dev/null +++ b/Tests/Unit/ReliabilityLayerBlackHoleTests.cpp @@ -0,0 +1,294 @@ +/* + * Copyright (c) 2026, MafiaHub + * + * This source code is licensed under the MIT-style license found in the + * license.txt file in the root directory of this source tree. + * + * Hermetic tests for in-session MTU black-hole recovery in ReliabilityLayer. + * + * The scenario is a tunnelled peer (OpenVPN and friends): the handshake's + * one-directional MTU probe passes, the connection establishes, and then the + * path silently drops every datagram over its real ceiling in one direction. + * Before the fix the reliability layer resent the same too-large datagram at + * the same size until the connection timed out. + * + * These tests drive two real ReliabilityLayer instances joined by a fake + * socket, with fully simulated time -- no loopback traffic, no wall clock. + * The channel between them applies a per-test drop rule to model the tunnel. + */ + +#include + +#include +#include + +#include "mafianet/BitStream.h" +#include "mafianet/DS_List.h" +#include "mafianet/MTUSize.h" +#include "mafianet/MessageIdentifiers.h" +#include "mafianet/MtuBlackHole.h" +#include "mafianet/PacketPriority.h" +#include "mafianet/PluginInterface2.h" +#include "mafianet/Rand.h" +#include "mafianet/ReliabilityLayer.h" +#include "mafianet/socket2.h" + +using namespace MafiaNet; + +namespace { + +const int TEST_MTU = MAXIMUM_MTU_SIZE; +const MafiaNet::TimeMS TEST_TIMEOUT_MS = 200000; + +// Captures every datagram the reliability layer hands to the OS, so the test +// fixture can shuttle it to the other endpoint (or drop it, like a tunnel). +struct CapturingSocket : public RakNetSocket2 +{ + std::vector > sent; + + virtual RNS2SendResult Send(RNS2_SendParameters *sendParameters, const char *file, unsigned int line) + { + (void) file; + (void) line; + sent.push_back(std::vector(sendParameters->data, sendParameters->data + sendParameters->length)); + return sendParameters->length; + } +}; + +class RelLayerBlackHole : public ::testing::Test +{ +protected: + ReliabilityLayer a, b; + CapturingSocket aSock, bSock; + SystemAddress aAddr, bAddr; + DataStructures::List handlers; + RakNetRandom rnr; + BitStream updateBitStream; + CCTimeType now; + + // Datagrams from a to b strictly larger than this many socket-level bytes + // are dropped, like a tunnel whose ceiling the handshake never probed. + // Socket-level bytes exclude the UDP/IP headers the MTU figures include. + int aToBDropOverBytes; + // When non-zero, drop this fraction (in percent) of ALL a->b datagrams, + // deterministically, to model plain loss. + int aToBLossPercent; + uint32_t lcgState; + + std::vector > received; // complete messages b got + + RelLayerBlackHole() + : updateBitStream(MAXIMUM_MTU_SIZE) + , now(1000000) + , aToBDropOverBytes(MAXIMUM_MTU_SIZE) + , aToBLossPercent(0) + , lcgState(0x12345678) + { + } + + virtual void SetUp() + { + ASSERT_TRUE(aAddr.FromStringExplicitPort("127.0.0.1", 40001)); + ASSERT_TRUE(bAddr.FromStringExplicitPort("127.0.0.1", 40002)); + a.Reset(true, TEST_MTU, false); + b.Reset(true, TEST_MTU, false); + a.SetTimeoutTime(TEST_TIMEOUT_MS); + b.SetTimeoutTime(TEST_TIMEOUT_MS); + } + + bool DropAToB(int datagramBytes) + { + if (datagramBytes > aToBDropOverBytes) + return true; + if (aToBLossPercent > 0) + { + lcgState = lcgState * 1664525u + 1013904223u; + if ((int)(lcgState % 100u) < aToBLossPercent) + return true; + } + return false; + } + + void SendFromA(const std::vector &payload) + { + ASSERT_TRUE(a.Send((char *) &payload[0], BYTES_TO_BITS((BitSize_t) payload.size()), + MafiaNet::Priority::Medium, MafiaNet::Reliability::ReliableOrdered, 0, true, TEST_MTU, now, 0)); + } + + // One 10ms tick: update both layers, then deliver the surviving datagrams. + void Tick() + { + now += 10000; // CCTimeType is microseconds + a.Update(&aSock, bAddr, TEST_MTU, now, 0, handlers, &rnr, updateBitStream); + b.Update(&bSock, aAddr, TEST_MTU, now, 0, handlers, &rnr, updateBitStream); + + for (size_t i = 0; i < aSock.sent.size(); i++) + { + if (DropAToB((int) aSock.sent[i].size())) + continue; + b.HandleSocketReceiveFromConnectedPlayer(&aSock.sent[i][0], (unsigned int) aSock.sent[i].size(), + aAddr, handlers, TEST_MTU, &bSock, &rnr, now, updateBitStream); + } + aSock.sent.clear(); + + for (size_t i = 0; i < bSock.sent.size(); i++) + { + // The return path is clean: the black hole under test is one-directional. + a.HandleSocketReceiveFromConnectedPlayer(&bSock.sent[i][0], (unsigned int) bSock.sent[i].size(), + bAddr, handlers, TEST_MTU, &aSock, &rnr, now, updateBitStream); + } + bSock.sent.clear(); + + unsigned char *data; + BitSize_t bitSize; + while ((bitSize = b.Receive(&data)) > 0) + { + received.push_back(std::vector(data, data + BITS_TO_BYTES(bitSize))); + rakFree_Ex(data, _FILE_AND_LINE_); + } + } + + // Pump until b has assembled wantMessages complete messages or simulated + // time runs out. Returns whether the goal was reached. + bool PumpUntilReceived(size_t wantMessages, int maxSimulatedMs) + { + for (int elapsed = 0; elapsed < maxSimulatedMs; elapsed += 10) + { + Tick(); + if (received.size() >= wantMessages) + return true; + EXPECT_FALSE(a.IsDeadConnection()); + EXPECT_FALSE(b.IsDeadConnection()); + if (a.IsDeadConnection() || b.IsDeadConnection()) + return false; + } + return received.size() >= wantMessages; + } + + static std::vector PatternMessage(size_t bytes, unsigned char seed) + { + std::vector msg(bytes); + msg[0] = 200; // stay clear of internal message ids + for (size_t i = 1; i < bytes; i++) + msg[i] = (unsigned char) (seed + i * 31); + return msg; + } +}; + +TEST_F(RelLayerBlackHole, DeliversALargeSplitMessageOverACleanChannel) +{ + // Harness sanity: split, transmission, ack and reassembly all work through + // the fake socket before any drop rule is involved. + std::vector msg = PatternMessage(8000, 3); + SendFromA(msg); + ASSERT_TRUE(PumpUntilReceived(1, 30000)); + EXPECT_EQ(msg, received[0]); + EXPECT_EQ(TEST_MTU, a.GetCurrentMtuBytes()); +} + +TEST_F(RelLayerBlackHole, RecoversWhenTheForwardPathBlackHolesLargeDatagrams) +{ + // The OpenVPN case: every datagram over the tunnel's real ceiling vanishes + // in one direction, and the ceiling sits below even the 1280 rung. The + // layer must notice the black hole, step its MTU down the ladder, re-split + // the stuck message, and deliver it -- all well inside the timeout. + aToBDropOverBytes = 1100; + + std::vector msg = PatternMessage(8000, 7); + SendFromA(msg); + + ASSERT_TRUE(PumpUntilReceived(1, 150000)); + EXPECT_EQ(msg, received[0]); + EXPECT_LE(a.GetCurrentMtuBytes(), 1100 + UDP_HEADER_SIZE); +} + +TEST_F(RelLayerBlackHole, PreservesOrderedDeliveryAcrossAStepDown) +{ + // Messages queued behind the stuck one on the same ordered channel must + // come out in send order once the step-down unblocks the channel. + aToBDropOverBytes = 1100; + + std::vector > messages; + messages.push_back(PatternMessage(8000, 11)); + messages.push_back(PatternMessage(60, 13)); + // Big enough to black-hole but small enough that it was never split: the + // step-down must also re-split stuck standalone messages. + messages.push_back(PatternMessage(1200, 29)); + messages.push_back(PatternMessage(2500, 17)); + messages.push_back(PatternMessage(60, 19)); + for (size_t i = 0; i < messages.size(); i++) + SendFromA(messages[i]); + + ASSERT_TRUE(PumpUntilReceived(messages.size(), 150000)); + ASSERT_EQ(messages.size(), received.size()); + for (size_t i = 0; i < messages.size(); i++) + EXPECT_EQ(messages[i], received[i]) << "message " << i << " out of order or corrupted"; +} + +TEST_F(RelLayerBlackHole, RecoversEvenWhenOnlyTheBottomRungFits) +{ + // Three successive step-downs (1400 -> 1280 -> 1024 -> 576), which also + // re-splits fragments that were themselves produced by an earlier re-split. + aToBDropOverBytes = 600; + + std::vector msg = PatternMessage(5000, 31); + SendFromA(msg); + + ASSERT_TRUE(PumpUntilReceived(1, 150000)); + EXPECT_EQ(msg, received[0]); + EXPECT_EQ(576, a.GetCurrentMtuBytes()); +} + +TEST_F(RelLayerBlackHole, DeliversTheAckReceiptForAReSplitMessage) +{ + // The receipt serial must survive the rebuild: the sender asked to be told + // when this message arrived, and it does arrive -- re-split. + aToBDropOverBytes = 1100; + + std::vector msg = PatternMessage(8000, 37); + const uint32_t receiptSerial = 777; + ASSERT_TRUE(a.Send((char *) &msg[0], BYTES_TO_BITS((BitSize_t) msg.size()), + MafiaNet::Priority::Medium, MafiaNet::Reliability::ReliableOrderedWithAckReceipt, 0, true, TEST_MTU, now, receiptSerial)); + + ASSERT_TRUE(PumpUntilReceived(1, 150000)); + EXPECT_EQ(msg, received[0]); + + // The receipt surfaces in the sender's own receive queue. + bool gotReceipt = false; + for (int elapsed = 0; elapsed < 10000 && !gotReceipt; elapsed += 10) + { + Tick(); + unsigned char *data; + BitSize_t bitSize; + while ((bitSize = a.Receive(&data)) > 0) + { + if (BITS_TO_BYTES(bitSize) == 5 && data[0] == ID_SND_RECEIPT_ACKED) + { + uint32_t serial; + memcpy(&serial, data + 1, sizeof(serial)); + EXPECT_EQ(receiptSerial, serial); + gotReceipt = true; + } + rakFree_Ex(data, _FILE_AND_LINE_); + } + } + EXPECT_TRUE(gotReceipt) << "ID_SND_RECEIPT_ACKED never surfaced for the re-split message"; +} + +TEST_F(RelLayerBlackHole, OrdinaryPacketLossDoesNotShrinkTheMtu) +{ + // The damaging false positive: plain loss hits datagrams of every size, so + // no single large packet should ever burn its whole resend budget while + // the connection is alive. A step-down here would permanently tax every + // datagram of an otherwise healthy connection. + aToBLossPercent = 10; + + std::vector msg = PatternMessage(8000, 23); + SendFromA(msg); + + ASSERT_TRUE(PumpUntilReceived(1, 60000)); + EXPECT_EQ(msg, received[0]); + EXPECT_EQ(TEST_MTU, a.GetCurrentMtuBytes()); +} + +} // namespace From 01c0648211751e3937d969cf7f09a0834cf08ee3 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Mon, 31 Aug 2026 22:36:20 +0200 Subject: [PATCH 2/2] fix(reliability): address review findings and Windows CI failure - Anchor the black-hole harness's simulated clock to the real clock at SetUp: ReliabilityLayer stamps timeLastDatagramArrived with the real GetTimeMS() on every receive and AckTimeout compares it against the time the caller passes in, so a fixed fake epoch declared every connection dead on any machine whose process uptime exceeded it -- which is what failed all six tests on Windows CI. Reproduced locally by anchoring the clock 20s behind real time. - Preserve the exact bit length of a message across a re-split (CodeRabbit): Send() takes bit lengths and normal split reassembly reproduces them exactly via the last fragment, but the rebuild rounded up to whole bytes. splitOriginalByteLength becomes splitOriginalBitLength; new regression test sends a message 3 bits short of a byte boundary through the black hole. - Move currentMtuBytes out of the USE_SLIDING_WINDOW_CONGESTION_CONTROL conditional (CodeRabbit + ultrareview): it is not congestion-manager- specific and every use is unguarded. The UDT config still fails on a pre-existing legacy-identifier error (RakNetTimeMS, ReliabilityLayer.cpp:143) present before this branch. - Write remoteSystem->MTUSize only when the value actually changed (CodeRabbit): GetMTUSize() reads it unsynchronized from the user thread, as it always has for the connect-time write; this keeps the field write-once-per-event instead of continuously written. --- Source/include/mafianet/InternalPacket.h | 16 ++++++---- Source/include/mafianet/ReliabilityLayer.h | 7 ++-- Source/src/RakPeer.cpp | 8 ++++- Source/src/ReliabilityLayer.cpp | 21 ++++++------ Tests/Unit/ReliabilityLayerBlackHoleTests.cpp | 32 +++++++++++++++++-- 5 files changed, 61 insertions(+), 23 deletions(-) diff --git a/Source/include/mafianet/InternalPacket.h b/Source/include/mafianet/InternalPacket.h index c468f2425..db4ccc754 100644 --- a/Source/include/mafianet/InternalPacket.h +++ b/Source/include/mafianet/InternalPacket.h @@ -123,13 +123,15 @@ struct InternalPacket : public InternalPacketFixedSizeTransmissionHeader uint32_t sendReceiptSerial; /// Sender-side bookkeeping, never transmitted: for a split fragment, the - /// byte length of the whole original message. All fragments of one message - /// share their data through refCountedData->sharedDataBlock, so together - /// with this length the original message can be rebuilt and re-split at a - /// smaller size when an in-session MTU black hole forces the negotiated - /// MTU down (see ReliabilityLayer::ReSplitOversizedMessages). Zero for - /// anything that is not a split fragment. - unsigned int splitOriginalByteLength; + /// exact bit length of the whole original message (bits, not bytes -- + /// Send() takes bit lengths and reassembly reproduces them exactly). All + /// fragments of one message share their data through + /// refCountedData->sharedDataBlock, so together with this length the + /// original message can be rebuilt and re-split at a smaller size when an + /// in-session MTU black hole forces the negotiated MTU down (see + /// ReliabilityLayer::ReSplitOversizedMessages). Zero for anything that is + /// not a split fragment. + BitSize_t splitOriginalBitLength; // Used for the resend queue // Linked list implementation so I can remove from the list via a pointer, without finding it in the list diff --git a/Source/include/mafianet/ReliabilityLayer.h b/Source/include/mafianet/ReliabilityLayer.h index 1351f9527..7b1d1fff0 100644 --- a/Source/include/mafianet/ReliabilityLayer.h +++ b/Source/include/mafianet/ReliabilityLayer.h @@ -602,13 +602,14 @@ class ReliabilityLayer// #if USE_SLIDING_WINDOW_CONGESTION_CONTROL==1 MafiaNet::CCRakNetSlidingWindow congestionManager; - // Current wire MTU in bytes including UDP/IP headers. Seeded from Reset()'s - // mtuSize, stepped down by black-hole detection. See GetCurrentMtuBytes(). - int currentMtuBytes; #else MafiaNet::CCRakNetUDT congestionManager; #endif + // Current wire MTU in bytes including UDP/IP headers. Seeded from Reset()'s + // mtuSize, stepped down by black-hole detection. See GetCurrentMtuBytes(). + int currentMtuBytes; + uint32_t unacknowledgedBytes; diff --git a/Source/src/RakPeer.cpp b/Source/src/RakPeer.cpp index f1821f236..a6f8e5ffc 100644 --- a/Source/src/RakPeer.cpp +++ b/Source/src/RakPeer.cpp @@ -6283,7 +6283,13 @@ bool RakPeer::RunUpdateCycle(BitStream &updateBitStream ) // The reliability layer steps the negotiated MTU down when it // detects an in-session path-MTU black hole; keep the value // GetMTUSize() reports in sync with what is actually on the wire. - remoteSystem->MTUSize = remoteSystem->reliabilityLayer.GetCurrentMtuBytes(); + // Written only on an actual step-down: GetMTUSize() reads this + // field from the user thread without synchronization (as it always + // has for the connect-time write), so avoid turning it into a + // continuously-written field. + const int reliabilityMtu = remoteSystem->reliabilityLayer.GetCurrentMtuBytes(); + if (remoteSystem->MTUSize != reliabilityMtu) + remoteSystem->MTUSize = reliabilityMtu; // Check for failure conditions if ( remoteSystem->reliabilityLayer.IsDeadConnection() || diff --git a/Source/src/ReliabilityLayer.cpp b/Source/src/ReliabilityLayer.cpp index 59541f5e4..8298c4446 100644 --- a/Source/src/ReliabilityLayer.cpp +++ b/Source/src/ReliabilityLayer.cpp @@ -3122,7 +3122,7 @@ void ReliabilityLayer::SplitPacket( InternalPacket *internalPacket ) internalPacketArray[ splitPacketIndex ]->splitPacketIndex = splitPacketIndex; internalPacketArray[ splitPacketIndex ]->splitPacketId = splitPacketId; internalPacketArray[ splitPacketIndex ]->splitPacketCount = internalPacket->splitPacketCount; - internalPacketArray[ splitPacketIndex ]->splitOriginalByteLength = dataByteLength; + internalPacketArray[ splitPacketIndex ]->splitOriginalBitLength = internalPacket->dataBitLength; RakAssert(internalPacketArray[ splitPacketIndex ]->dataBitLengthsplitPacketCount ); @@ -3189,23 +3189,24 @@ void ReliabilityLayer::ReSplitOversizedMessages(void) // behaviour rather than losing a reliable message. auto rebuildMessage = [this](InternalPacket *pkt) -> InternalPacket* { unsigned char *source; - unsigned int byteLength; + BitSize_t bitLength; if (pkt->splitPacketCount > 0) { // Fragments carry a slice of a shared, refcounted copy of the whole - // message; splitOriginalByteLength is its full length. + // message; splitOriginalBitLength is its exact full length. RakAssert(pkt->allocationScheme == InternalPacket::REF_COUNTED); - RakAssert(pkt->refCountedData != 0 && pkt->splitOriginalByteLength > 0); - if (pkt->allocationScheme != InternalPacket::REF_COUNTED || pkt->refCountedData == 0 || pkt->splitOriginalByteLength == 0) + RakAssert(pkt->refCountedData != 0 && pkt->splitOriginalBitLength > 0); + if (pkt->allocationScheme != InternalPacket::REF_COUNTED || pkt->refCountedData == 0 || pkt->splitOriginalBitLength == 0) return 0; source = pkt->refCountedData->sharedDataBlock; - byteLength = pkt->splitOriginalByteLength; + bitLength = pkt->splitOriginalBitLength; } else { source = pkt->data; - byteLength = (unsigned int) BITS_TO_BYTES(pkt->dataBitLength); + bitLength = pkt->dataBitLength; } + const unsigned int byteLength = (unsigned int) BITS_TO_BYTES(bitLength); unsigned char *copy = (unsigned char*) rakMalloc_Ex(byteLength, _FILE_AND_LINE_); if (copy == 0) { @@ -3227,11 +3228,11 @@ void ReliabilityLayer::ReSplitOversizedMessages(void) *rebuilt = *pkt; AllocInternalPacketData(rebuilt, copy); rebuilt->refCountedData = 0; - rebuilt->dataBitLength = BYTES_TO_BITS(byteLength); + rebuilt->dataBitLength = bitLength; rebuilt->splitPacketCount = 0; rebuilt->splitPacketIndex = 0; rebuilt->splitPacketId = 0; - rebuilt->splitOriginalByteLength = 0; + rebuilt->splitOriginalBitLength = 0; rebuilt->messageNumberAssigned = false; rebuilt->timesSent = 0; rebuilt->nextActionTime = 0; @@ -4020,7 +4021,7 @@ InternalPacket* ReliabilityLayer::AllocateFromInternalPacketPool(void) ip->allocationScheme=InternalPacket::NORMAL; ip->data=0; ip->timesSent=0; - ip->splitOriginalByteLength=0; + ip->splitOriginalBitLength=0; return ip; } //------------------------------------------------------------------------------------------------------- diff --git a/Tests/Unit/ReliabilityLayerBlackHoleTests.cpp b/Tests/Unit/ReliabilityLayerBlackHoleTests.cpp index b7fbec394..2f69e9ba0 100644 --- a/Tests/Unit/ReliabilityLayerBlackHoleTests.cpp +++ b/Tests/Unit/ReliabilityLayerBlackHoleTests.cpp @@ -24,6 +24,7 @@ #include "mafianet/BitStream.h" #include "mafianet/DS_List.h" +#include "mafianet/GetTime.h" #include "mafianet/MTUSize.h" #include "mafianet/MessageIdentifiers.h" #include "mafianet/MtuBlackHole.h" @@ -38,7 +39,7 @@ using namespace MafiaNet; namespace { const int TEST_MTU = MAXIMUM_MTU_SIZE; -const MafiaNet::TimeMS TEST_TIMEOUT_MS = 200000; +const MafiaNet::TimeMS TEST_TIMEOUT_MS = 1000000; // Captures every datagram the reliability layer hands to the OS, so the test // fixture can shuttle it to the other endpoint (or drop it, like a tunnel). @@ -76,10 +77,11 @@ class RelLayerBlackHole : public ::testing::Test uint32_t lcgState; std::vector > received; // complete messages b got + std::vector receivedBits; // their exact bit lengths RelLayerBlackHole() : updateBitStream(MAXIMUM_MTU_SIZE) - , now(1000000) + , now(0) , aToBDropOverBytes(MAXIMUM_MTU_SIZE) , aToBLossPercent(0) , lcgState(0x12345678) @@ -88,6 +90,14 @@ class RelLayerBlackHole : public ::testing::Test virtual void SetUp() { + // The simulated clock must start at the real clock: ReliabilityLayer + // stamps timeLastDatagramArrived with the real GetTimeMS() on every + // receive, and AckTimeout compares that against the time we pass in. + // A fixed fake epoch fails on any machine whose process uptime exceeds + // it (this killed every test on Windows CI). All progress is still + // driven by fixed fake increments from this anchor. + now = MafiaNet::GetTimeUS(); + ASSERT_TRUE(aAddr.FromStringExplicitPort("127.0.0.1", 40001)); ASSERT_TRUE(bAddr.FromStringExplicitPort("127.0.0.1", 40002)); a.Reset(true, TEST_MTU, false); @@ -144,6 +154,7 @@ class RelLayerBlackHole : public ::testing::Test while ((bitSize = b.Receive(&data)) > 0) { received.push_back(std::vector(data, data + BITS_TO_BYTES(bitSize))); + receivedBits.push_back(bitSize); rakFree_Ex(data, _FILE_AND_LINE_); } } @@ -275,6 +286,23 @@ TEST_F(RelLayerBlackHole, DeliversTheAckReceiptForAReSplitMessage) EXPECT_TRUE(gotReceipt) << "ID_SND_RECEIPT_ACKED never surfaced for the re-split message"; } +TEST_F(RelLayerBlackHole, PreservesSubByteBitLengthAcrossAReSplit) +{ + // Send() takes a length in BITS and SplitPacket deliberately keeps the + // exact count on the last fragment, so a normally-split message reassembles + // to its precise bit length. A re-split must not round it up to a whole + // byte: BitStream consumers on the receiver see the message's bit size. + aToBDropOverBytes = 1100; + + std::vector msg = PatternMessage(8000, 41); + const BitSize_t oddBits = BYTES_TO_BITS((BitSize_t) msg.size()) - 3; + ASSERT_TRUE(a.Send((char *) &msg[0], oddBits, + MafiaNet::Priority::Medium, MafiaNet::Reliability::ReliableOrdered, 0, true, TEST_MTU, now, 0)); + + ASSERT_TRUE(PumpUntilReceived(1, 150000)); + EXPECT_EQ(oddBits, receivedBits[0]); +} + TEST_F(RelLayerBlackHole, OrdinaryPacketLossDoesNotShrinkTheMtu) { // The damaging false positive: plain loss hits datagrams of every size, so