Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix: properly handle cases of stale loss report #1079

Merged
merged 3 commits into from
Jan 22, 2020
Merged

Bugfix: properly handle cases of stale loss report #1079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b30470f
Bugfix: properly handle cases of stale loss report
Jan 21, 2020
ec3156b
Small post-review fix
Jan 22, 2020
f958636
Removed unnecessary macro blockers
Jan 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 60 additions & 4 deletions srtcore/core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7350,6 +7350,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 @@ -7551,6 +7579,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 @@ -7697,9 +7733,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;
maxsharabayko marked this conversation as resolved.
Show resolved Hide resolved

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 @@ -7712,7 +7764,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 @@ -8017,6 +8070,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
Open in desktop
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