[core] CSndLossList limits the maximum offset (#1733)

and checks sequence numbers for validity Co-authored-by: Alex Pokotilo <support@wmspanel.com>
Haivision · Jan 14, 2021 · 648e8b5 · 648e8b5
1 parent a5609d3
commit 648e8b5
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 19 deletions.
diff --git a/srtcore/list.cpp b/srtcore/list.cpp
@@ -111,7 +111,19 @@ void CSndLossList::traceState() const
 
 int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
 {
-    SRT_ASSERT(CSeqNo::seqlen(seqno1, seqno2) > 0);
+    if (seqno1 < 0 || seqno2 < 0 ) {
+        LOGC(qslog.Error, log << "IPE: Tried to insert negative seqno " << seqno1 << ":" << seqno2
+            << " into sender's loss list. Ignoring.");
+        return 0;
+    }
+
+    const int inserted_range = CSeqNo::seqlen(seqno1, seqno2);
+    if (inserted_range <= 0 || inserted_range >= m_iSize) {
+        LOGC(qslog.Error, log << "IPE: Tried to insert too big range of seqno: " << inserted_range <<  ". Ignoring. "
+                << "seqno " << seqno1 << ":" << seqno2);
+        return 0;
+    }
+
     ScopedLock listguard(m_ListLock);
 
     if (m_iLength == 0)
@@ -123,7 +135,16 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
     // Find the insert position in the non-empty list
     const int origlen = m_iLength;
     const int offset  = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno1);
-    int       loc     = (m_iHead + offset + m_iSize) % m_iSize;
+
+    if (offset >= m_iSize)
+    {
+        LOGC(qslog.Error, log << "IPE: New loss record is too far from the first record. Ignoring. "
+                << "First loss seqno " << m_caSeq[m_iHead].seqstart
+                << ", insert seqno " << seqno1 << ":" << seqno2);
+        return 0;
+    }
+
+    int loc = (m_iHead + offset + m_iSize) % m_iSize;
 
     if (loc < 0)
     {

diff --git a/test/test_list.cpp b/test/test_list.cpp
@@ -59,6 +59,16 @@ TEST_F(CSndLossListTest, InsertPopOneElem)
     CheckEmptyArray();
 }
 
+TEST_F(CSndLossListTest, InsertNegativeSeqno)
+{
+    cerr << "Expecting IPE message:" << endl;
+    EXPECT_EQ(m_lossList->insert(1, SRT_SEQNO_NONE), 0);
+    EXPECT_EQ(m_lossList->insert(SRT_SEQNO_NONE, SRT_SEQNO_NONE), 0);
+    EXPECT_EQ(m_lossList->insert(SRT_SEQNO_NONE, 1), 0);
+
+    CheckEmptyArray();
+}
+
 /// Insert two elements at once and pop one by one
 TEST_F(CSndLossListTest, InsertPopTwoElemsRange)
 {
@@ -505,21 +515,22 @@ TEST_F(CSndLossListTest, InsertFullListCoalesce)
         EXPECT_EQ(m_lossList->insert(i, i), 1);
     EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE);
     // Inserting additional element: 1 item more than list size.
-    // But given all elements coalesce into one entry, list size should still increase.
-    EXPECT_EQ(m_lossList->insert(CSndLossListTest::SIZE + 1, CSndLossListTest::SIZE + 1), 1);
-    EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1);
-    for (int i = 1; i <= CSndLossListTest::SIZE + 1; i++)
+    // Given all elements coalesce into one entry, there is a place to insert it,
+    // but sequence span now exceeds list size.
+    EXPECT_EQ(m_lossList->insert(CSndLossListTest::SIZE + 1, CSndLossListTest::SIZE + 1), 0);
+    EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE);
+    for (int i = 1; i <= CSndLossListTest::SIZE; i++)
     {
         EXPECT_EQ(m_lossList->popLostSeq(), i);
-        EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1 - i);
+        EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE - i);
     }
     EXPECT_EQ(m_lossList->popLostSeq(), -1);
     EXPECT_EQ(m_lossList->getLossLength(), 0);
 
     CheckEmptyArray();
 }
 
-TEST_F(CSndLossListTest, DISABLED_InsertFullListNoCoalesce)
+TEST_F(CSndLossListTest, InsertFullListNoCoalesce)
 {
     // We will insert each element with a gap of one elements.
     // This should lead to having space for only [i; SIZE] sequence numbers.
@@ -530,23 +541,22 @@ TEST_F(CSndLossListTest, DISABLED_InsertFullListNoCoalesce)
     // [0]:taken, [1]: empty, [2]: taken, [3]: empty, ...
     EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE / 2);
 
-    // Inserting additional element: 1 item more than list size.
-    // There should be one free place for it at list[SIZE-1]
-    // right after previously inserted element.
+    // Inserting additional element out of the list span must fail.
     const int seqno1 = CSndLossListTest::SIZE + 2;
-    EXPECT_EQ(m_lossList->insert(seqno1, seqno1), 1);
+    EXPECT_EQ(m_lossList->insert(seqno1, seqno1), 0);
 
-    // Inserting one more element into a full list.
-    // There should be no place for it.
-    const int seqno2 = CSndLossListTest::SIZE + 4;
-    EXPECT_EQ(m_lossList->insert(seqno2, seqno2), 0);
+    // There should however be a place for one element right after the last inserted one.
+    const int seqno_last = CSndLossListTest::SIZE + 1;
+    EXPECT_EQ(m_lossList->insert(seqno_last, seqno_last), 1);
 
-    EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1);
-    for (int i = 1; i <= CSndLossListTest::SIZE + 1; i++)
+    const int initial_length = m_lossList->getLossLength();
+    EXPECT_EQ(initial_length, CSndLossListTest::SIZE / 2 + 1);
+    for (int i = 1; i <= CSndLossListTest::SIZE / 2; i++)
     {
         EXPECT_EQ(m_lossList->popLostSeq(), 2 * i);
-        EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE  - i);
+        EXPECT_EQ(m_lossList->getLossLength(), initial_length - i);
     }
+    EXPECT_EQ(m_lossList->popLostSeq(), seqno_last);
     EXPECT_EQ(m_lossList->popLostSeq(), -1);
     EXPECT_EQ(m_lossList->getLossLength(), 0);