[core] CSndLossList limits the maximum offset

[maxsharabayko]

Introduction

In runtime, the maximum number of sequence numbers that may be considered lost is limited by the flight span (limited by flow window).

The actual number of items in the sender's loss list may probably be a bit more in the case when sending moved forward, but not all sequences previously reported as lost were retransmitted. Still, most of the operations ensure the loss list is properly updated. For example, CUDT::checkNeedDrop also removes dropped sequence numbers, and m_iSndLastAck ensures old numbers are not added to the list when processing a NAK report.

Anyway, the size of the loss list is set to twice the size of the flow window just in case there would be more elements to store.
m_pSndLossList = new CSndLossList(m_iFlowWindowSize * 2); - link.

What is Fixed

When a new entry is added, it is checked if the offset from the head entry fits the size of the list, which should be more than enough (m_iFlowWindowSize * 2).
Even if there is enough room to add this new entry (e.g. elements coalesced), it must not be allowed to add an entry outside of the valid sequence span.

The corresponding unit test was updated.

Notes

Addresses #1000, #1604.
@alexpokotilo Feel free to join the review.

[alexpokotilo]

this check doesn't cover case with seqno2!= seqno1.
E.g is offset < m_iSize but (offset + CSeqNo::seqlen(seqno1, seqno2)) >= m_iSize we should probably
fix seqno2 or fail CSndLossList::insert

[maxsharabayko]

Good point! Inserting a longer range should not produce any issue, but logically should be limited.
At the same time, additional validation of seqno2 might slightly impact the performance of this insert call. 🤔

[alexpokotilo]

Ignore this finding then. As I understand we need this quick fix for 1.4.*, but later we will have #1012 finally integrated so we don't have to worry too much about such minor issues.
Please reply my comment below about m_iFlowWindowSize. I think it is more important.

[maxsharabayko]

#1012 has to be reworked a bit. All entries must be stored in order in the sender loss list. In that PR the insert function tries to loop over the container and find an empty place to store a new entry.

[alexpokotilo]

@maxsharabayko, great fix and I've reviewed and commented it. One question not related to CSendLostList itself: as list is initizlized with m_iFlowWindowSize, is there any check during a handshake preventing us setting m_iFlowWindowSize, to 0, negative or invalid value? I was not able to find such check but probably I'm missing something.

[alexpokotilo]

@maxsharabayko
we need to add following code at the beginning of CSndLossList::insert instead of
SRT_ASSERT(CSeqNo::seqlen(seqno1, seqno2) > 0);

    const int inserted_range = CSeqNo::seqlen(seqno1, seqno2);

    if (seqno1 < 0 || seqno2 < 0 || inserted_range <= 0 || inserted_range >= m_iSize) {
        LOGC(qslog.Error, log << "IPE: Try to insert huge range. " << inserted_range <<  " Ignoring. "
                << "seqno " << seqno1 << ":" << seqno2);
        return 0;
    }

assert is good for debugging but in runtime we need to check these values to prevent issues like
#1604 (comment)

[alexpokotilo]

@maxsharabayko
please also add following test into test_list.cpp

TEST_F(CSndLossListTest, InsertSRT_SEQNO_NONE)
{
    cerr << "Expecting IPE message:" << endl;
    EXPECT_EQ(m_lossList->insert(1, SRT_SEQNO_NONE), 0);
    while(m_lossList->popLostSeq() != -1) {}
}

this test will panic without this fix #1733 (review)