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] Fix CSndLossList::insert with negative offset #1359

Merged
merged 2 commits into from
Aug 13, 2020
Merged

[core] Fix CSndLossList::insert with negative offset #1359

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34a7134
[core] Fix CSndLossList::insert with negative offset
maxsharabayko Jun 19, 2020
1e44b9d
Added check for upper sequence
maxsharabayko Aug 12, 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
1 change: 1 addition & 0 deletions srtcore/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,7 @@ class CSeqNo
/// distance between two sequence numbers.
///
/// Example: to check if (seq1 %> seq2): seqcmp(seq1, seq2) > 0.
/// Note: %> stands for "later than".
inline static int seqcmp(int32_t seq1, int32_t seq2)
{return (abs(seq1 - seq2) < m_iSeqNoTH) ? (seq1 - seq2) : (seq2 - seq1);}

Expand Down
13 changes: 13 additions & 0 deletions srtcore/list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,17 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
const int offset = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno1);
int loc = (m_iHead + offset + m_iSize) % m_iSize;

if (loc < 0)
{
// The size of the CSndLossList should be at least the size of the flow window.
// It means that all the packets sender has sent should fit within m_iSize.
// If the new loss does not fit, there is some error.
LOGC(mglog.Error, log << "IPE: New loss record is too old. Ignoring. "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this situation can be described as IPE here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW. Does it mean that offset is allowed to be negative in general, as long as it fits in the range from the current head and size?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Potentially the following can happen:

  1. Two packets with sequence numbers A and B (A <% B) are reported as lost.
  2. Packet A is retransmitted and removed from the loss list.
  3. Packet A is reported lost again, so that CSeqNo::seqoff(m_caSeq[m_iHead].seqstart = B, seqno1 = A) < 0.

So negative offset is allowed, it just has to fit into the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't however head moved forward after getting ACK? Isn't then a sequence that is earlier than head a sequence before the ACK? If this is true, then I can only imagine such a situation if reordering happened and the ACK that confirms the packet in question comes earlier than the lossreport that reports this sequence as lost, although it's actually outdated.

If this is the case, however, then it should be first checked if the upper sequence is below the head, and only if so should the loss record be rejected as a whole. Otherwise at least the range between the head and the upper sequence be recorded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The head is moved forward after the loss record is extracted and the corresponding packet is retransmitted.

If the same packet is reported lost again, it is placed before the head (negative offset), and the head position is updated.

// offset < 0
const int offset  = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno1);

As I wrote above, a negative offset is allowed, it just has to fit into the list.

The loc identifies the position in the list to insert at. It is not allowed to be negative, otherwise it is out of bounds memory access.

int loc = (m_iHead + offset + m_iSize) % m_iSize;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I understand that negative loc within the bounds is allowed, but I don't think it's correct to reject the whole report because of that. It should reject it only if the upper bound is behind the seqstart, and if it's not, the loss record should be accepted with the lower bound adjusted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added check for upper sequence.

<< "First loss seqno " << m_caSeq[m_iHead].seqstart
<< ", insert seqno " << seqno1 << ":" << seqno2);
return 0;
}

if (offset < 0)
{
insertHead(loc, seqno1, seqno2);
Expand Down Expand Up @@ -153,6 +164,7 @@ int CSndLossList::insert(int32_t seqno1, int32_t seqno2)
if (CSeqNo::seqcmp(seqend, seqno1) < 0 && CSeqNo::incseq(seqend) != seqno1)
{
// No overlap
// TODO: Here we should actually insert right after i, not at loc.
insertAfter(loc, i, seqno1, seqno2);
}
else
Expand Down Expand Up @@ -347,6 +359,7 @@ int32_t CSndLossList::popLostSeq()

void CSndLossList::insertHead(int pos, int32_t seqno1, int32_t seqno2)
{
SRT_ASSERT(pos >= 0);
m_caSeq[pos].seqstart = seqno1;
SRT_ASSERT(m_caSeq[pos].seqend == SRT_SEQNO_NONE);
if (seqno2 != seqno1)
Expand Down
8 changes: 2 additions & 6 deletions srtcore/list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,22 +66,18 @@ class CSndLossList
/// @param [in] seqno1 sequence number starts.
/// @param [in] seqno2 sequence number ends.
/// @return number of packets that are not in the list previously.

int insert(int32_t seqno1, int32_t seqno2);

/// Remove the given sequence number and all numbers that precede it.
/// @param [in] seqno sequence number.

void removeUpTo(int32_t seqno);

/// Read the loss length.∏
/// @return The length of the list.

int getLossLength() const;

/// Read the first (smallest) loss seq. no. in the list and remove it.
/// @return The seq. no. or -1 if the list is empty.

int32_t popLostSeq();

void traceState() const;
Expand All @@ -90,7 +86,7 @@ class CSndLossList
struct Seq
{
int32_t seqstart; // sequence number starts
int32_t seqend; // seqnence number ends
int32_t seqend; // sequence number ends
int inext; // index of the next node in the list
} * m_caSeq;

Expand All @@ -116,7 +112,7 @@ class CSndLossList

/// Update existing element with the new range (increase only)
/// @param pos position of the element being updated
/// @param seqno1 first seqnuence number in range
/// @param seqno1 first sequence number in range
/// @param seqno2 last sequence number in range (SRT_SEQNO_NONE if no range)
bool updateElement(int pos, int32_t seqno1, int32_t seqno2);

Expand Down
50 changes: 43 additions & 7 deletions test/test_list.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ TEST_F(CSndLossListTest, InsertHeadOverlap02)
CheckEmptyArray();
}

TEST_F(CSndLossListTest, DISABLED_InsertHeadNegativeOffset01)
TEST_F(CSndLossListTest, InsertHeadNegativeOffset01)
{
EXPECT_EQ(m_lossList->insert(10000000, 10000000), 1);
EXPECT_EQ(m_lossList->insert(10000001, 10000001), 1);
Expand All @@ -470,17 +470,53 @@ TEST_F(CSndLossListTest, DISABLED_InsertHeadNegativeOffset01)

/////////////////////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////////////////////
TEST_F(CSndLossListTest, DISABLED_InsertFullList)
TEST_F(CSndLossListTest, InsertFullListCoalesce)
{
for (int i = 1; i <= CSndLossListTest::SIZE; i++)
m_lossList->insert(i, i);
EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE);
m_lossList->insert(CSndLossListTest::SIZE + 1, CSndLossListTest::SIZE + 1);
EXPECT_EQ(m_lossList->insert(i, i), 1);
EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE);
for (int i = 1; i <= CSndLossListTest::SIZE; i++)
// 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++)
{
EXPECT_EQ(m_lossList->popLostSeq(), i);
EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE - i);
EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1 - i);
}
EXPECT_EQ(m_lossList->popLostSeq(), -1);
EXPECT_EQ(m_lossList->getLossLength(), 0);

CheckEmptyArray();
}

TEST_F(CSndLossListTest, DISABLED_InsertFullListNoCoalesce)
{
// We will insert each element with a gap of one elements.
// This should lead to having space for only [i; SIZE] sequence numbers.
for (int i = 1; i <= CSndLossListTest::SIZE / 2; i++)
EXPECT_EQ(m_lossList->insert(2 * i, 2 * i), 1);

// At this point the list has every second element empty
// [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.
const int seqno1 = CSndLossListTest::SIZE + 2;
EXPECT_EQ(m_lossList->insert(seqno1, seqno1), 1);

// 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);

EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE + 1);
for (int i = 1; i <= CSndLossListTest::SIZE + 1; i++)
{
EXPECT_EQ(m_lossList->popLostSeq(), 2 * i);
EXPECT_EQ(m_lossList->getLossLength(), CSndLossListTest::SIZE - i);
}
EXPECT_EQ(m_lossList->popLostSeq(), -1);
EXPECT_EQ(m_lossList->getLossLength(), 0);
Expand Down