Skip to content

Commit

Permalink
[core] Fixed handling of stale loss report (#1079)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ethouris @maxsharabayko
ethouris authored and maxsharabayko committed Jan 22, 2020
1 parent 7bf96c7 commit 617fb80
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 4 deletions.
64 changes: 60 additions & 4 deletions srtcore/core.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7349,6 +7349,34 @@ void CUDT::processCtrl(const CPacket &ctrlpkt)
// more important, so simply drop the part that predates ACK.
num = m_pSndLossList->insert(m_iSndLastAck, losslist_hi);
}
else
{
// This should be treated as IPE, but this may happen in one situtation:
// - redundancy second link (ISN was screwed up initially, but late towards last sent)
// - initial DROPREQ was lost
// This just causes repeating DROPREQ, as when the receiver continues sending
// LOSSREPORT, it's probably UNAWARE OF THE SITUATION.
//
// When this DROPREQ gets lost in UDP again, the receiver will do one of these:
// - repeatedly send LOSSREPORT (as per NAKREPORT), so this will happen again
// - finally give up rexmit request as per TLPKTDROP (DROPREQ should make
// TSBPD wake up should it still wait for new packets to get ACK-ed)

HLOGC(mglog.Debug, log << CONID() << "LOSSREPORT: IGNORED with SndLastAck=%"
<< m_iSndLastAck << ": %" << losslist_lo << "-" << losslist_hi
<< " - sending DROPREQ (IPE or DROPREQ lost with ISN screw)");

// This means that the loss touches upon a range that wasn't ever sent.
// Normally this should never happen, but this might be a case when the
// ISN FIX for redundant connection was missed.

// In distinction to losslist, DROPREQ has always a range
// always just one range, and the data are <LO, HI>, with no range bit.
int32_t seqpair[2] = {losslist_lo, losslist_hi};
const int32_t no_msgno = 0; // We don't know - this wasn't ever sent

sendCtrl(UMSG_DROPREQ, &no_msgno, seqpair, sizeof(seqpair));
}

enterCS(m_StatsLock);
m_stats.traceSndLoss += num;
Expand Down Expand Up @@ -7550,6 +7578,14 @@ void CUDT::processCtrl(const CPacket &ctrlpkt)
case UMSG_DROPREQ: // 111 - Msg drop request
enterCS(m_RecvLock);
m_pRcvBuffer->dropMsg(ctrlpkt.getMsgSeq(using_rexmit_flag), using_rexmit_flag);
// When the drop request was received, it means that there are
// packets for which there will never be ACK sent; if the TSBPD thread
// is currently in the ACK-waiting state, it may never exit.
if (m_bTsbPd)
{
HLOGP(mglog.Debug, "DROPREQ: signal TSBPD");
pthread_cond_signal(&m_RcvTsbPdCond);
}
leaveCS(m_RecvLock);

dropFromLossLists(*(int32_t *)ctrlpkt.m_pcData, *(int32_t *)(ctrlpkt.m_pcData + 4));
Expand Down Expand Up @@ -7696,9 +7732,25 @@ int CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origintime
const int offset = CSeqNo::seqoff(m_iSndLastDataAck, w_packet.m_iSeqNo);
if (offset < 0)
{
LOGC(dlog.Error,
log << "IPE: packLostData: LOST packet negative offset: seqoff(m_iSeqNo " << w_packet.m_iSeqNo
<< ", m_iSndLastDataAck " << m_iSndLastDataAck << ")=" << offset << ". Continue");
// XXX Likely that this will never be executed because if the upper
// sequence is not in the sender buffer, then most likely the loss
// was completely ignored.
LOGC(dlog.Error, log << "IPE/EPE: packLostData: LOST packet negative offset: seqoff(m_iSeqNo "
<< w_packet.m_iSeqNo << ", m_iSndLastDataAck " << m_iSndLastDataAck
<< ")=" << offset << ". Continue");

// No matter whether this is right or not (maybe the attack case should be
// considered, and some LOSSREPORT flood prevention), send the drop request
// to the peer.
int32_t seqpair[2];
seqpair[0] = w_packet.m_iSeqNo;
seqpair[1] = m_iSndLastDataAck;

HLOGC(mglog.Debug, log << "PEER reported LOSS not from the sending buffer - requesting DROP: "
<< "msg=" << MSGNO_SEQ::unwrap(w_packet.m_iMsgNo) << " SEQ:"
<< seqpair[0] << " - " << seqpair[1] << "(" << (-offset) << " packets)");

sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, sizeof(seqpair));
continue;
}

Expand All @@ -7711,7 +7763,8 @@ int CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origintime
int32_t seqpair[2];
seqpair[0] = w_packet.m_iSeqNo;
seqpair[1] = CSeqNo::incseq(seqpair[0], msglen);
sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, 8);

sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, sizeof(seqpair));

// only one msg drop request is necessary
m_pSndLossList->remove(seqpair[1]);
Expand Down Expand Up @@ -8015,6 +8068,9 @@ void CUDT::sendLossReport(const std::vector<std::pair<int32_t, int32_t> > &loss_

int CUDT::processData(CUnit *in_unit)
{
if (m_bClosing)
return -1;

CPacket &packet = in_unit->m_Packet;

// XXX This should be called (exclusively) here:
Expand Down
16 changes: 16 additions & 0 deletions srtcore/list.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ modified by

#include "list.h"
#include "packet.h"
#include "logging.h"

// Use "inline namespace" in C++11
namespace srt_logging
{
extern Logger dlog, mglog;
}

using srt_logging::mglog;


using namespace srt::sync;
Expand Down Expand Up @@ -459,6 +468,13 @@ void CRcvLossList::insert(int32_t seqno1, int32_t seqno2)

// otherwise searching for the position where the node should be
int offset = CSeqNo::seqoff(m_caSeq[m_iHead].data1, seqno1);
if (offset < 0)
{
LOGC(mglog.Error, log << "RCV-LOSS/insert: IPE: new LOSS %(" << seqno1 << "-" << seqno2
<< ") PREDATES HEAD %" << m_caSeq[m_iHead].data1 << " -- REJECTING");
return;
}

int loc = (m_iHead + offset) % m_iSize;

if ((-1 != m_caSeq[m_iTail].data2) && (CSeqNo::incseq(m_caSeq[m_iTail].data2) == seqno1))
Expand Down

0 comments on commit 617fb80

Please sign in to comment.