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.

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

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

Merged

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Jun 19, 2020

Work in progress.

Probably CSndLossList::insert(..) should return -1 on failure, but then it needs to be properly handled in CUDT.

Related to #1000.

Suggestion by @ethouris

In file mode you just have to stop sending new packets, until retransmissions are successful enough and enough losses are covered
In live mode I don't think it's possible to live up to the live stream requirements with a large number of losses (or it's not a problem because sender drops would solve the problem earlier).
Meaning, if sender drops are working, this won't be a problem - or at least sender dropping could be enforced with a large number of drops. But we also have an option to turn sender drops completely off - in this case the only way to go is to close the connection.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Jun 19, 2020
@maxsharabayko maxsharabayko added this to the v1.5.0 - Sprint 17 milestone Jun 19, 2020
@maxsharabayko maxsharabayko self-assigned this Jun 19, 2020
@maxsharabayko maxsharabayko changed the title [tests] Expect result from list::insert [core] Fix CSndLossList::insert with negative offset Jun 22, 2020
srtcore/common.h Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 17, v1.5.0 - Sprint 19 Jun 26, 2020
@maxsharabayko maxsharabayko modified the milestones: v1.5.0 - Sprint 19, v1.5.0 - Sprint 20 Jul 28, 2020
@maxsharabayko maxsharabayko marked this pull request as ready for review August 7, 2020 08:21
srtcore/list.cpp Outdated
// 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.

@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 20, v1.5.0 - Sprint 21 Aug 10, 2020
@maxsharabayko maxsharabayko merged commit 7790171 into Haivision:master Aug 13, 2020
@maxsharabayko maxsharabayko deleted the hotfix/snd-loss-list-no1000 branch August 13, 2020 09:25
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 21, v1.4.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Priority: High Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants