Tweak handling of some edge cases.

In several different edge cases, we may decide not to ack a packet to the peer.
(Especially if nwe need to protect ourselves against a malicious peer, etc.)

In that case, don't drop the whole packet and stop processing it.  That is too
much.  Just inhibit marking the packet as received and acking it.  Among other
things, we want our stats to reflect the fact that we did receive this packet.
Especially, we want to mark the packet as received, so that we can quickly
reject a duplicate.

Also add a bunch of clarifying comments.

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp

@@ -988,19 +988,17 @@ void CSteamNetworkConnectionBase::ClearCrypto()
 	m_cryptIVRecv.Wipe();
 }
 
-bool CSteamNetworkConnectionBase::RecvNonDataSequencedPacket( int64 nPktNum, SteamNetworkingMicroseconds usecNow )
+void CSteamNetworkConnectionBase::RecvNonDataSequencedPacket( int64 nPktNum, SteamNetworkingMicroseconds usecNow )
 {
+	// Note: order of operations is important betwen these two calls
 
-	// Let SNP know when we received it, so we can track loss events and send acks
-	if ( SNP_RecordReceivedPktNum( nPktNum, usecNow, false ) )
-	{
+	// Let SNP know when we received it, so we can track loss events and send acks.  We do
+	// not schedule acks to be sent at this time, but when they are sent, we will implicitly
+	// ack this one
+	SNP_RecordReceivedPktNum( nPktNum, usecNow, false );
 
-		// And also the general purpose sequence number/stats tracker
-		// for the end-to-end flow.
-		m_statsEndToEnd.TrackProcessSequencedPacket( nPktNum, usecNow, 0 );
-	}
-
-	return true;
+	// Update general sequence number/stats tracker for the end-to-end flow.
+	m_statsEndToEnd.TrackProcessSequencedPacket( nPktNum, usecNow, 0 );
 }
 
 bool CSteamNetworkConnectionBase::BThinkCryptoReady( SteamNetworkingMicroseconds usecNow )
@@ -2278,7 +2276,12 @@ bool CSteamNetworkConnectionBase::ReceivedMessage( const void *pData, int cbData
 	// Create a message
 	CSteamNetworkingMessage *pMsg = CSteamNetworkingMessage::New( this, cbData, nMsgNum, nFlags, usecNow );
 	if ( !pMsg )
+	{
+		// Hm.  this failure really is probably a sign that we are in a pretty bad state,
+		// and we are unlikely to recover.  Should we just abort the connection?
+		// Right now, we'll try to muddle on.
 		return false;
+	}
 
 	// Copy the data
 	memcpy( pMsg->m_pData, pData, cbData );

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h

@@ -454,11 +454,12 @@ public:
 	/// processing the packet
 	bool DecryptDataChunk( uint16 nWireSeqNum, int cbPacketSize, const void *pChunk, int cbChunk, RecvPacketContext_t &ctx );
 
-	/// Decode the plaintext
+	/// Decode the plaintext.  Returns false if the packet seems corrupt or bogus, or should abort further
+	/// processing.
 	bool ProcessPlainTextDataChunk( int usecTimeSinceLast, RecvPacketContext_t &ctx );
 
 	/// Called when we receive an (end-to-end) packet with a sequence number
-	bool RecvNonDataSequencedPacket( int64 nPktNum, SteamNetworkingMicroseconds usecNow );
+	void RecvNonDataSequencedPacket( int64 nPktNum, SteamNetworkingMicroseconds usecNow );
 
 	// Called from SNP to update transmit/receive speeds
 	void UpdateSpeeds( int nTXSpeed, int nRXSpeed );
@@ -678,7 +679,7 @@ protected:
 	int SNP_ClampSendRate();
 	void SNP_PopulateDetailedStats( SteamDatagramLinkStats &info );
 	void SNP_PopulateQuickStats( SteamNetworkingQuickConnectionStatus &info, SteamNetworkingMicroseconds usecNow );
-	bool SNP_RecordReceivedPktNum( int64 nPktNum, SteamNetworkingMicroseconds usecNow, bool bScheduleAck );
+	void SNP_RecordReceivedPktNum( int64 nPktNum, SteamNetworkingMicroseconds usecNow, bool bScheduleAck );
 	EResult SNP_FlushMessage( SteamNetworkingMicroseconds usecNow );
 
 	/// Accumulate "tokens" into our bucket base on the current calculated send rate

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp

@@ -547,6 +547,7 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa
 
 	const SteamNetworkingMicroseconds usecNow = ctx.m_usecNow;
 	const int64 nPktNum = ctx.m_nPktNum;
+	bool bInhibitMarkReceived = false;
 
 	const int nLogLevelPacketDecode = m_connectionConfig.m_LogLevel_PacketDecode.Get();
 	SpewVerboseGroup( nLogLevelPacketDecode, "[%s] decode pkt %lld\n", GetDescription(), (long long)nPktNum );
@@ -700,10 +701,17 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa
 			//
 			READ_SEGMENT_DATA_SIZE( reliable )
 
-			// Ingest the segment.  If it seems fishy, abort processing of this packet
-			// and do not acknowledge to the sender.
+			// Ingest the segment.
 			if ( !SNP_ReceiveReliableSegment( nPktNum, nDecodeReliablePos, pSegmentData, cbSegmentSize, usecNow ) )
-				return false;
+			{
+				if ( !BStateIsActive() )
+					return false; // we decided to nuke the connection - abort packet processing
+
+				// We're not able to ingest this reliable segment at the moment,
+				// but we didn't terminate the connection.  So do not ack this packet
+				// to the peer.  We need them to retransmit
+				bInhibitMarkReceived = true;
+			}
 
 			// Advance pointer for the next reliable segment, if any.
 			nDecodeReliablePos += cbSegmentSize;
@@ -1094,19 +1102,39 @@ bool CSteamNetworkConnectionBase::ProcessPlainTextDataChunk( int usecTimeSinceLa
 		}
 	}
 
-	// Update structures needed to populate our ACKs
-	bool bScheduleAck = nDecodeReliablePos > 0;
-	if ( !SNP_RecordReceivedPktNum( nPktNum, usecNow, bScheduleAck ) )
+	// Should we record that we received it?
+	if ( bInhibitMarkReceived )
 	{
-		// Do NOT mark that we received this packet
-		return false;
+		// Something really odd.  High packet loss / fragmentation.
+		// Potentially the peer is being abusive and we need
+		// to protect ourselves.
+		//
+		// Act as if the packet was dropped.  This will cause the
+		// peer's sender logic to interpret this as additional packet
+		// loss and back off.  That's a feature, not a bug.
+	}
+	else
+	{
+
+		// Update structures needed to populate our ACKs.
+		// If we received reliable data now, then schedule an ack
+		bool bScheduleAck = nDecodeReliablePos > 0;
+		SNP_RecordReceivedPktNum( nPktNum, usecNow, bScheduleAck );
 	}
 
-	// Packet is OK.  Track end-to-end flow.
+	// Track end-to-end flow.  Even if we decided to tell our peer that
+	// we did not receive this, we want our own stats to reflect
+	// that we did.  (And we want to be able to quickly reject a
+	// packet with this same number.)
+	//
+	// Also, note that order of operations is important.  This call must
+	// happen after the SNP_RecordReceivedPktNum call above
 	m_statsEndToEnd.TrackProcessSequencedPacket( nPktNum, usecNow, usecTimeSinceLast );
+
+	// Packet can be processed further
 	return true;
 
-	// Make sure these don't get used beyond where we intended them toget used
+	// Make sure these don't get used beyond where we intended them to get used
 	#undef DECODE_ERROR
 	#undef EXPECT_BYTES
 	#undef READ_8BITU
@@ -2537,7 +2565,7 @@ bool CSteamNetworkConnectionBase::SNP_ReceiveReliableSegment( int64 nPktNum, int
 				(long long)( m_receiverState.m_nReliableStreamPos + m_receiverState.m_bufReliableStream.size() ),
 				(long long)cbNewSize, (long long)nSegEnd
 			);
-			return false; 
+			return false;  // DO NOT ACK THIS PACKET
 		}
 
 		// Check if this is going to make a new gap
@@ -2565,7 +2593,7 @@ bool CSteamNetworkConnectionBase::SNP_ReceiveReliableSegment( int64 nPktNum, int
 						(long long)m_receiverState.m_mapReliableStreamGaps.rbegin()->first, (long long)m_receiverState.m_mapReliableStreamGaps.rbegin()->second,
 						(long long)nSegBegin, (long long)nSegEnd
 					);
-					return false; 
+					return false;  // DO NOT ACK THIS PACKET
 				}
 			}
 
@@ -2650,7 +2678,7 @@ bool CSteamNetworkConnectionBase::SNP_ReceiveReliableSegment( int64 nPktNum, int
 									(long long)gapFilled->first, (long long)gapFilled->second,
 									(long long)nSegBegin, (long long)nSegEnd
 								);
-								return false; 
+								return false;  // DO NOT ACK THIS PACKET
 							}
 
 							// Save bounds of the right side
@@ -2744,7 +2772,10 @@ bool CSteamNetworkConnectionBase::SNP_ReceiveReliableSegment( int64 nPktNum, int
 			uint64 nOffset;
 			pReliableDecode = DeserializeVarInt( pReliableDecode, pReliableEnd, nOffset );
 			if ( pReliableDecode == nullptr )
-				return true; // We haven't received all of the message
+			{
+				// We haven't received all of the message
+				return true; // Packet OK and can be acked.
+			}
 
 			nMsgNum += nOffset;
 
@@ -2782,7 +2813,10 @@ bool CSteamNetworkConnectionBase::SNP_ReceiveReliableSegment( int64 nPktNum, int
 			uint64 nMsgSizeUpperBits;
 			pReliableDecode = DeserializeVarInt( pReliableDecode, pReliableEnd, nMsgSizeUpperBits );
 			if ( pReliableDecode == nullptr )
-				return true; // We haven't received all of the message
+			{
+				// We haven't received all of the message
+				return true; // Packet OK and can be acked.
+			}
 
 			// Sanity check size.  Note that we do this check before we shift,
 			// to protect against overflow.
@@ -2809,12 +2843,12 @@ bool CSteamNetworkConnectionBase::SNP_ReceiveReliableSegment( int64 nPktNum, int
 		if ( pReliableDecode+cbMsgSize > pReliableEnd )
 		{
 			// Ouch, we did all that work and still don't have the whole message.
-			return true;
+			return true; // packet is OK, can be acked, and continue processing it
 		}
 
 		// We have a full message!  Queue it
 		if ( !ReceivedMessage( pReliableDecode, cbMsgSize, nMsgNum, k_nSteamNetworkingSend_Reliable, usecNow ) )
-			return false;
+			return false; // Weird failure.  Most graceful response is to not ack this packet, and maybe we will work next on retry.
 		pReliableDecode += cbMsgSize;
 		int cbStreamConsumed = pReliableDecode-pReliableStart;
 
@@ -2829,24 +2863,26 @@ bool CSteamNetworkConnectionBase::SNP_ReceiveReliableSegment( int64 nPktNum, int
 		nNumReliableBytes -= cbStreamConsumed;
 	} while ( nNumReliableBytes > 0 );
 
-	return true;
+	return true; // packet is OK, can be acked, and continue processing it
 }
 
-bool CSteamNetworkConnectionBase::SNP_RecordReceivedPktNum( int64 nPktNum, SteamNetworkingMicroseconds usecNow, bool bScheduleAck )
+void CSteamNetworkConnectionBase::SNP_RecordReceivedPktNum( int64 nPktNum, SteamNetworkingMicroseconds usecNow, bool bScheduleAck )
 {
 
 	// Check if sender has already told us they don't need us to
 	// account for packets this old anymore
-	if ( nPktNum < m_receiverState.m_nMinPktNumToSendAcks )
-		return true;
+	if ( unlikely( nPktNum < m_receiverState.m_nMinPktNumToSendAcks ) )
+		return;
 
+	// Latest time that this packet should be acked.
+	// (We might already be scheduled to send and ack that would include this packet.)
 	SteamNetworkingMicroseconds usecScheduleAck = bScheduleAck ? usecNow + k_usecMaxDataAckDelay : INT64_MAX;
 
 	// Fast path for the (hopefully) most common case of packets arriving in order
-	if ( nPktNum == m_statsEndToEnd.m_nMaxRecvPktNum+1 )
+	if ( likely( nPktNum == m_statsEndToEnd.m_nMaxRecvPktNum+1 ) )
 	{
 		m_receiverState.QueueFlushAllAcks( usecScheduleAck );
-		return true;
+		return;
 	}
 
 	// At this point, ack invariants should be met
@@ -2858,7 +2894,7 @@ bool CSteamNetworkConnectionBase::SNP_RecordReceivedPktNum( int64 nPktNum, Steam
 
 		// Protect against malicious sender!
 		if ( len( m_receiverState.m_mapPacketGaps ) >= k_nMaxPacketGaps )
-			return false; // CALLER: Do not record that we received this packet!
+			return; // Nope, we will *not* actually mark the packet as received
 
 		// Add a gap for the skipped packet(s).
 		int64 nBegin = m_statsEndToEnd.m_nMaxRecvPktNum+1;
@@ -2910,13 +2946,17 @@ bool CSteamNetworkConnectionBase::SNP_RecordReceivedPktNum( int64 nPktNum, Steam
 		// Check if this filed a gap
 		auto itGap = m_receiverState.m_mapPacketGaps.upper_bound( nPktNum );
 		--itGap;
-		Assert( itGap->first <= nPktNum );
+		if ( itGap == m_receiverState.m_mapPacketGaps.end() || itGap->first > nPktNum )
+		{
+			AssertMsg( false, "Cannot locate gap, or processing packet multiple times" );
+			return;
+		}
 		if ( itGap->second.m_nEnd <= nPktNum )
 		{
 			// We already received this packet.  But this should be impossible now,
 			// we should be rejecting duplicate packet numbers earlier
 			AssertMsg( false, "Processing a packet multiple times" );
-			return true;
+			return;
 		}
 
 		// Packet is in a gap where we previously thought packets were lost.
@@ -3011,7 +3051,7 @@ bool CSteamNetworkConnectionBase::SNP_RecordReceivedPktNum( int64 nPktNum, Steam
 			// Packet is in the middle of the gap.  We'll need to fragment this gap
 			// Protect against malicious sender!
 			if ( len( m_receiverState.m_mapPacketGaps ) >= k_nMaxPacketGaps )
-				return false; // CALLER: Do not record that we received this packet!
+				return; // Nope, we will *not* actually mark the packet as received
 
 			// Locate the next block so we can set the schedule time
 			auto itNext = itGap;
@@ -3110,9 +3150,6 @@ bool CSteamNetworkConnectionBase::SNP_RecordReceivedPktNum( int64 nPktNum, Steam
 			m_receiverState.DebugCheckPackGapMap();
 		}
 	}
-
-	// OK, this packet is legit, allow caller to continue processing it
-	return true;
 }
 
 int CSteamNetworkConnectionBase::SNP_ClampSendRate()