From 648e8b5b266edaaa60f7a7178da21df1d1ca9aa2 Mon Sep 17 00:00:00 2001 From: Maxim Sharabayko Date: Thu, 14 Jan 2021 15:42:21 +0100 Subject: [PATCH] [core] CSndLossList limits the maximum offset (#1733) and checks sequence numbers for validity Co-authored-by: Alex Pokotilo --- srtcore/list.cpp | 25 +++++++++++++++++++++++-- test/test_list.cpp | 44 +++++++++++++++++++++++++++----------------- 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/srtcore/list.cpp b/srtcore/list.cpp index 01e7e6016..c7ab7cfc8 100644 --- 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 index 72164a5e5..73e66c98b 100644 --- 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,13 +515,14 @@ 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); @@ -519,7 +530,7 @@ TEST_F(CSndLossListTest, InsertFullListCoalesce) 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);