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

[core] CSndLossList limits the maximum offset #1733

Merged
merged 4 commits into from
Jan 14, 2021
Merged

[core] CSndLossList limits the maximum offset #1733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5d6569
[core] CSndLossList limits the maximum offset
maxsharabayko Jan 11, 2021
370995f
Fixed InsertFullListCoalesce test
maxsharabayko Jan 11, 2021
05e66fc
Added a check for a valid seqnos
maxsharabayko Jan 14, 2021
58999b5
Added unit test for negative seqno insertion
maxsharabayko Jan 14, 2021
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
25 changes: 23 additions & 2 deletions srtcore/list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Copy link
Contributor

@alexpokotilo alexpokotilo Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

{
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)
{
Expand Down
44 changes: 27 additions & 17 deletions test/test_list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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.
Expand All @@ -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);

Expand Down