Stop waiting messages can corrupt the packet gap map's sentinel. The last element of the packet map is always the sentinel. Its m_nEnd value is always INT64_MAX and its key should always be one more than the highest packet the receiver wants to ACK. But if the client sends a stop waiting frame such that m_nMinPktNumToSendAcks is greater the sentinel's key (i.e. greater than any packet number the receiver has received), the sentinel's key will be overwritten to instead have its value be m_nMinPktNumToSendAcks. This results in the receiver ACKing packets that it has not received, and that the sender has possibly not sent.
The relevant code is here
|
// Trim from the front of the packet gap list, |
|
// we can stop reporting these losses to the sender |
|
auto h = m_receiverState.m_mapPacketGaps.begin(); |
|
while ( h->first <= m_receiverState.m_nMinPktNumToSendAcks ) |
|
{ |
|
if ( h->second.m_nEnd > m_receiverState.m_nMinPktNumToSendAcks ) |
|
{ |
|
// Ug. You're not supposed to modify the key in a map. |
|
// I suppose that's legit, since you could violate the ordering. |
|
// but in this case I know that this change is OK. |
|
const_cast<int64 &>( h->first ) = m_receiverState.m_nMinPktNumToSendAcks; |
|
break; |
|
} |
Suppose the sender sends a stop waiting frame with a high m_nMinPktNumToSendAcks value, a value higher than the key of the sentinel. As we walk through the loop to erase packet gaps that we no longer care about, we eventually hit the sentinel. Because the sentinel's m_nEnd value is INT64_MAX, this loop treats the sentinel as a packet gap that m_nMinPktNumToSendAcks falls in the middle of. It then sets the key of the sentinel to be m_nMinPktNumToSendAcks, violating the property that the sentinel's key should always be one more than the highest packet the receiver wants to ACK.
The effects of this is that later, in SNP_SerializeAckBlocks, the highest ACK to send is determined by const int64 nLastPktToAck = itSentinel->first-1;. So if the sentinel's key is changed to a higher value, the receiver will ACK packets it hasn't received.
I think fixing this should be simple: add a check to make sure the loop that erases packet gaps upon receiving a stop waiting frame does not change the sentinel.
Stop waiting messages can corrupt the packet gap map's sentinel. The last element of the packet map is always the sentinel. Its
m_nEndvalue is alwaysINT64_MAXand its key should always be one more than the highest packet the receiver wants to ACK. But if the client sends a stop waiting frame such thatm_nMinPktNumToSendAcksis greater the sentinel's key (i.e. greater than any packet number the receiver has received), the sentinel's key will be overwritten to instead have its value bem_nMinPktNumToSendAcks. This results in the receiver ACKing packets that it has not received, and that the sender has possibly not sent.The relevant code is here
GameNetworkingSockets/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_snp.cpp
Lines 1025 to 1037 in 4fbfe83
Suppose the sender sends a stop waiting frame with a high
m_nMinPktNumToSendAcksvalue, a value higher than the key of the sentinel. As we walk through the loop to erase packet gaps that we no longer care about, we eventually hit the sentinel. Because the sentinel'sm_nEndvalue isINT64_MAX, this loop treats the sentinel as a packet gap thatm_nMinPktNumToSendAcksfalls in the middle of. It then sets the key of the sentinel to bem_nMinPktNumToSendAcks, violating the property that the sentinel's key should always be one more than the highest packet the receiver wants to ACK.The effects of this is that later, in
SNP_SerializeAckBlocks, the highest ACK to send is determined byconst int64 nLastPktToAck = itSentinel->first-1;. So if the sentinel's key is changed to a higher value, the receiver will ACK packets it hasn't received.I think fixing this should be simple: add a check to make sure the loop that erases packet gaps upon receiving a stop waiting frame does not change the sentinel.