Srt: [BUG] Infinite loop when library is under stress

[hlecnt]

Describe the bug
When putting SRT library under stress, the library goes into infinite loops after a while. After some investigations using gdb and valgrind I've found some issues in CSndLossList class. In my use case, inifinite loops are caused because of bad reset of indexes in the structure handled by CSndLossList or a missing condition in searching loop. Thus I have reproduced these issues in unitary test.
Please find attached a patch that solves the issues described here.
srt.1.3.4.patch.zip

To Reproduce
Steps to reproduce the behavior:

1. Using a custom tool reading a multicast stream and serving SRT using the library
2. Use tsp (TSDuck tool) to send multicast stream: alternate from regulated mode (about 10Mbps) to unregulated (under stress use case, send packet is fast as it can) every 10 seconds
3. Use srt-live-transmit to get the data
4. After a couple of minutes, SRT library goes into an infinite loop

Alternatively, run the unit tests provided in the proposed patch to reproduce issues.

Expected behavior
No infinite loop in library even under stress

Screenshots
no applicable

Desktop (please provide the following information):

• OS: Linux
• SRT Version 1.3.4

Additional context
I'm not friendly with the SRT code and design; the proposed patch covers my use case but is probably not suitable for all others. Let me know.

[maxsharabayko]

@hlecnt Thanks for reporting!
Could you please attach test/test_list.cpp?

[hlecnt]

@maxlovic please find the testing file below:
test_list.zip

you will have to update the filelist.maf file.

[maxsharabayko]

CSndLossList::getLostSeq() is missing.

[hlecnt]

OK, I think you have to apply the entire patch I provided in the first message (SRT 1.3.4)

Or add it manually:
int32_t CSndLossList::getLostSeq()
{
CGuard listguard(m_ListLock);

if (0 == m_iLength)
return -1;

if (m_iLastInsertPos == m_iHead)
m_iLastInsertPos = -1;

// return the first loss seq. no.
int32_t seqno = m_caSeq[m_iHead].data1;
remove_noguard(seqno);
return seqno;
}

[maxsharabayko]

You probably had some changes not included in the patch.
Now works, thank you!

[hlecnt]

I have re zipped the patch having the code of CSndLossList::getLostSeq() up to date:
srt.1.3.4.patch.zip

[maxsharabayko]

Created branch for the fix:
maxlovic/hotfix/snd-loss-list

[hlecnt]

@maxlovic Are you waiting for any action on my side ? Please let me know.

[maxsharabayko]

@hlecnt Looked through the changes briefly.
I really like you have covered the CSndBuffer with unit tests. Thank you for that!
Will need another more thorough review.

As I see, getLostSeq() is a replacement of popLostSeq(), right?

I think we should create a PR from your changes? Would you like to create it? Just take last three commits from my branch maxlovic/hotfix/snd-loss-list.
I can do it for you, if you don't want to spend time on this.

[hlecnt]

@maxsharabayko popLostSeq() was not present in 1.3.4. Now they do the same. Just a comment about getLostSeq() in 1.3.4: CGuard was not in the right place. I will appreciate if you can do the PR. Thanks.

[maxsharabayko]

@hlecnt Code covergage for CSndLossList.
No pressure, just out of interest. Do you think you could improve the coverage of unit tests?

list-coverage.zip

[hlecnt]

@maxsharabayko did you reuse the whole set of unit tests I provided in the original patch ? Most of the CSndLossList logic was covered by them.

[maxsharabayko]

@hlecnt I did. There is a total of 29 tests for CSndLossList. Please correct me if there are more in your patch.

As you can see from the HTML file in list-coverage.zip, e.g. some places of CSndLossList::insert() are not covered by tests.

But again, just kindly asking if you have time and will to improve the coverage.
No pressure.

[hlecnt]

@maxsharabayko I can spend some time to improve code coverage within the next weeks. I'll let you informed about that. Can you give me the command line used to run tests with GCOV enabled ?

[maxsharabayko]

@hlecnt
The latest SRT version has ENABLE_CODE_COVERAGE option added to CMakeLists.txt.
You can checkout my branch maxsharabayko/hotfix/snd-loss-list to have the latest SRT version with you unit tests.

Then, you can take the shell script below, place it to SRT root folder, and run, e.g.
./generate-coverage.sh _build_lin
where "_build_lin" would be the name of the directory that will be used to store build artifacts.
Then in that directory you should find the "_codecov" folder with index.html inside.

generate-coverage.zip

You should have lcov, gcov and genhtml utilities installed.

[hlecnt]

@maxsharabayko Hello, please find attached a patch to unit tests improving CSndLoss::insert() coverage. Let me know.

The patch shall be applied on your branch maxsharabayko/hotfix/snd-loss-list.

0001-Improve-CSndLossList-insert-unit-tests-coverage.zip

[maxsharabayko]

Hi @hlecnt
Thank you for your contribution!
I've added those new tests from you to the PR #1012.
I am doing some refactoring to reduce the complexity of the CSndLossLists functions. Then I think the PR will be good to go. Will keep you updated.

[maxsharabayko]

Status Update

Some improvements to CSndLossList were done, out-of-bounds access was fixed.

• PR #1097 - Minor refactoring.
• PR #1092, #1357 - Unit tests for CSndLossList.
• PR #1359 - Fix CSndLossList::insert with negative offset (out-of-bounds access)..

TODO: Investigate the infinite loop situation. Try to reproduce. Certain logic of the loss list assumes there is a certain span of sequence numbers of lost packets (2 x flight_window). Is the assumption violated under heavy losses?

inifinite loops are caused because of bad reset of indexes in the structure handled by CSndLossList or a missing condition in searching loop.

[CommonsNat]

I'm now following this issue after running into it in the field and doing some debugging. If you need assistance reproducing I can try to come up with a minimal case that does so, or if you need any other help characterizing the issue. In general: this seems to occur when a the receiver doesn't close the connection but stops reading incoming data. After a long enough time of marking dropped packets it seems the m_caSeq list resets, causing it to get stuck in the while loop that's looking for the highest sequence number less than seq1.

[maxsharabayko]

@CommonsNat It would be very helpful to get a reproducible test case.
I was thinking to reproduce the test as it is described by @hlecnt using TSDuck, but also to send SRT (TSDuck now supports SRT input and output).

If you say it is enough that the receiver stops reading the data, then there can be an easier reproduction example: an SRT receiver that receives but does not read data from SRT.

In general: this seems to occur when a the receiver doesn't close the connection but stops reading incoming data.
After a long enough time of marking dropped packets it seems the m_caSeq list resets, causing it to get stuck in the while loop that's looking for the highest sequence number less than seq1.

It is likely this loop is going infinite:

// 2. Find the highest sequence number less than seqno1.
while (m_caSeq[i].inext != -1 && CSeqNo::seqcmp(m_caSeq[m_caSeq[i].inext].seqstart, seqno1) < 0)
    i = m_caSeq[i].inext;

[CommonsNat]

If you say it is enough that the receiver stops reading the data, then there can be an easier reproduction example: an SRT receiver that receives but does not read data from SRT.

Yes, I do believe this is the case, though I don't have the code for the receiving application, only debugging logs, so that's what I'm going off of for now. I'll try to dig into this deeper tomorrow.

It is likely this loop is going infinite:

Yes, that's the one, I've verified with GDB.

[CommonsNat]

Sorry, I haven't had time to look into this, and probably won't until next week. I'm going to see if I can modify the srt transit app to reproduce the error. In the mean time, do you have any ideas where to look for troubleshooting?

Certain logic of the loss list assumes there is a certain span of sequence numbers of lost packets (2 x flight_window). Is the assumption violated under heavy losses?

Where is the pertinent code where this assumption is made?

[maxsharabayko]

This infinite loop behavior should be reproduced with unit test.
Infinite lopp test case:

TEST_F(CSndLossListTest, InsertPositiveOffsetTooFar)
{
    const int32_t head_seqno = 1000;
    EXPECT_EQ(m_lossList->insert(head_seqno, head_seqno), 1);
    EXPECT_EQ(m_lossList->getLossLength(), 1);

    // The offset of the sequence number being added does not fit
    // into the size of the loss list, it must be ignored.
    // Normally this situation should not happen.

    const int32_t outofbound_seqno = head_seqno + CSndLossListTest::SIZE;
    m_lossList->insert(outofbound_seqno, outofbound_seqno);

    const int32_t outofbound_seqno2 = head_seqno + 2 * CSndLossListTest::SIZE;
    m_lossList->insert(outofbound_seqno2, outofbound_seqno2);
}

[maxsharabayko]

@CommonsNat

Where is the pertinent code where this assumption is made?

core.cpp#L881

m_pSndLossList = new CSndLossList(m_iFlowWindowSize * 2);

m_iFlowWindowSize is the maximum number of packets in flight, and only those packets in flight can be reported lost.
The sender's loss list has twice the size of the flight window.
Probably, if there are too many losses, the list holds too many elements, and the assumption breaks.

[CommonsNat]

That unit test does reproduce the infinite loops case. I patched CSndLossList to report this case, mostly so I could have a breakpoint right before the infinite loop:

diff --git a/srtcore/list.cpp b/srtcore/list.cpp
index d229fb1..d554e59 100644
--- a/srtcore/list.cpp
+++ b/srtcore/list.cpp
@@ -122,6 +122,11 @@ 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 (offset >= m_iSize)
+    {
+        LOGC(mglog.Error, log << "Sequence number " << seqno1 << " exceeds send drop list size " << m_iSize << ".");
+    }
+
     if (loc < 0)
     {
         const int offset_seqno2 = CSeqNo::seqoff(m_caSeq[m_iHead].seqstart, seqno2);

The unit test hits it, as expected, on the two inserts that are out of range. My live scenario also hits this, but the state of CSndLossList is different between these two scenarios. In the unit test, the first sequence number that's out of range (head_seqno + CSndLossListTest::SIZE), inserts into the head position, and incorrectly points inext to itself, then the subsequent insertion causes the infinite loop.

In the case I'm witnessing in my live scenario, by stopping at the breakpoint set to that logging message, I can see that the state of the m_caSeq list is that every single entry in it is {seqstart = -1, seqend = -1, inext = 0}, so when this first out of bounds sequence number goes to be inserted, it hits the infinite loop.

It seems, given the list state, that m_iLength should probably be 0, but for some reason it's 2147282938. Immediately prior to execution stopping at the breakpoint I set, SRT prints the following message from CUDT::packLostData 200707 times, with m_iSeqNo incrementing from 0 to 200706:

IPE/EPE: packLostData: LOST packet negative offset: ...

Perhaps there's some error in managing m_iLength when popping from the list? That's all I have time for tonight. Let me know if there's anything specific I can do to help, otherwise I'll resume troubleshooting tomorrow.

[maxsharabayko]

@CommonsNat Thank you for these important observations!
From your description, there are probably more issues with the CSndLossList then. The state you see (seqstart = -1, seqend = -1, inext = 0) is not a correct state of the list. And of course, m_iLength should not be positive it that case as well.
Maybe it is related to the result of sequence range function CSeqNo::seqoff. It is not expected to work properly If the range is too big (more than ~INT32_MAX / 4).

Do you have a minimal reproducable example for your experiment you could share?

[CommonsNat]

Sorry I wasn't able to get to this yesterday. I tried patching srt-live-transmit to reproduce the issue, but haven't been able to induce the infinite loop yet. I'm going to compare the settings for the SRT connection to make sure they're identical to my application's and try this test again, after the weekend.

[CommonsNat]

I stripped my application code to a minimized example, but I can't reproduce the issue. I figured that something about my application setup was causing the issue, but I'm not seeing packLostData message, nor is the thread getting stuck in the lost list logic. I have my application send to a patched version of srt-live-transmit that enters an infinite sleep loop 10 seconds after establishing a connection. Despite this, my application never even puts any packets into the CSndLossList. I also attempted to run the unmodified srt-live-transmit and halt it with GDB, but that breaks the connection on the sending side in my test application. Tomorrow I'm going to test my minimal application in the live environment to test if it produces the issue.

[CommonsNat]

Tested the minimal application in the live environment today and it produced the expected problem. This creates a problem for me trying to provide a reproducible case for this thread: in this live scenario, the receiving application is not mine, and I apparently don't know exactly what it's doing with respect to SRT when I see it stall. My attempts to reproduce the behavior yesterday, and how I originally asserted, was that the listener just had to stop reading data from an open connection, but it appears that's not all it takes to induce the issue. I took a look at the receiving application in GDB, but it looks like references to any SRT code is obfuscated. At this point I'm not sure how to provide a test case. If you have any suggestions let me know, otherwise I will go back to troubleshooting the issue myself.

[maxsharabayko]

@CommonsNat Thank you for your help!

At this moment we at least have the unit test to reproduce the hang up, even though there might be some other issues with the list. The hang up can be addressed next then (probably next week).

There is a proposed fix of the while loop submitted by @hlecnt . As I understand, it can change the sequential placements of loss entries, which may affect other logic of the list. Probably the best way at this point would be to avoid adding loss entries with too large span from the entries already in the list.

I will also review whether all relevant calls to the sender's loss list are protected by mutex where needed. Maybe something will be found there as well.

[CommonsNat]

I'm guessing the proposed fix by @hlecnt is in #1012? I'll hold off further troubleshooting until the PR is complete or if you're able to find something.

[CommonsNat]

I tracked back the source of the CSndLossList getting messed up. I figured the bad values for m_iLength were a big red flag, so I followed them up. m_iLength gets set to a negative value in CSndLossList::insertHead when a very large negative value is added to it:

m_iLength += CSeqNo::seqlen(seqno1, seqno2);

In the cases I've seen, seqno1 = seqno2 + 1, which is causing the result of seqlen to be INT32_MIN.

I followed up these sequence numbers to CUD::checkRexmitTimer, and they're being inserted here

const int32_t csn = m_iSndCurrSeqNo;
const int     num = m_pSndLossList->insert(m_iSndLastAck, csn)

It seems in this case m_iSndLastAck is one greater than m_iSndCurrSeqNo, though I'm not sure what's causing this yet.

[CommonsNat]

I traced to where m_iSndLastAck gets set to be greater than m_iSndCurrSeqNo, it seems to happen in CUDT::checkNeedDrop. m_iSndLastAck and m_iSndLastDataAck get set to a fakeack greater than m_iSndCurrSeqNo. I added this line to the function for more information and turned on the logging:

LOGC(dlog.Error, log << "SND-DROP: Update ack " << oldack << " > " << m_iSndLastAck << ". send seq: " << m_iSndCurrSeqNo << " > " << minlastack);

oldack here is set to m_iSndLastAck before it's updated to fakeack.

I also inserted a line for logging just prior to the insertion in CUDT::checkRexmitTimer:

LOGC(mglog.Error, log << "reXmit insert SndLossList " << m_iSndLastAck << "->" << csn << " (" << m_iSndCurrSeqNo << ")");

Here's a log of the results that show m_iSndLastAck being updated to be ahead of m_iSndCurrSeqNo, and the following bad insertion:

22:22:26.130042/SRT:SndQ:w1*E:SRT.mg: @302540387:packData: Applying EXTRACTION sequence 884557853 over SCHEDULING sequence 884557853 DIFF: 0 STAMP:31D64272
22:22:26.130076/SRT:SndQ:w1*E:SRT.mg: @302540387:packData: Applying EXTRACTION sequence 884557854 over SCHEDULING sequence 884557854 DIFF: 0 STAMP:1F61264C
22:22:26.134000/infiniteloop*E:SRT.d: SND-DROP: Update ack 884557853 > 884557856.  send seq: 884557855 > 884557855
22:22:26.134006/infiniteloop*E:SRT.d: SND-DROP: %(884557853-884557855) n=3pkt 1880B, span=1021 ms, FIRST #57009
22:22:26.134031/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 884557856->884557855 (884557855)

^ This is the bad insert

22:22:26.134091/SRT:SndQ:w1*E:SRT.d: IPE/EPE: packLostData: LOST packet negative offset: seqoff(m_iSeqNo 0, m_iSndLastDataAck 884557856)=-884557856. Continue

It seems that m_iSndLastAck being updated to be ahead of m_iSndCurrSeqNo inside of CUDT::checkNeedDrop is actually pretty typical in the case where you're dropping all the packets up to m_iSndCurrSeqNo. This occurs many times before the bad insert occurs in my setup, but CUDT::packData runs and increases m_iSndCurrSeqNo before CUDT::checkRexmitTimer runs, for example here:

22:22:25.984133/infiniteloop*E:SRT.d: SND-DROP: Update ack 884557782 > 884557785.  send seq: 884557784 > 884557784
22:22:25.984141/infiniteloop*E:SRT.d: SND-DROP: %(884557782-884557784) n=3pkt 1692B, span=1022 ms, FIRST #56938
22:22:25.984207/SRT:SndQ:w1*E:SRT.mg: @302540387:packData: Applying EXTRACTION sequence 884557785 over SCHEDULING sequence 884557785 DIFF: 0 STAMP:2ED6427D
22:22:25.984244/SRT:SndQ:w1*E:SRT.mg: @302540387:packData: Applying EXTRACTION sequence 884557786 over SCHEDULING sequence 884557786 DIFF: 0 STAMP:1861254C
22:22:25.988972/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 884557785->884557786 (884557786)

This seems to be odd, should it be possible to drop data ahead of m_iSndCurrSeqNo? Or record it as ACKed by incrementing m_iSndLastAck ahead of it? I suppose it is probably important to know why data that hasn't been sent yet is late, though I suspect this is due to repeated heavy loss due to the state of the receiver being very slow to receive and ACK new packets. I'll look into this next.

[CommonsNat]

I went back in the logs for this stall and it's as I suspected in the previous post, it seems that the drops occur after the non-responsive receiver fails to ACK a packet that's repeatedly retransmitted after the loss timeout is up.

08:29:13.724044/SRT:SndQ:w1*E:SRT.mg: @511418812:packData: Applying EXTRACTION sequence 1817946040 over SCHEDULING sequence 1817946040 DIFF: 0 STAMP:3A7560B5
08:29:13.829991/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:13.830007/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:13.906989/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:13.907005/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:13.996996/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:13.997014/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:14.086989/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:14.087006/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:14.165007/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:14.165022/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:14.254963/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:14.254979/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:14.332984/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:14.332998/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:14.424989/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:14.425004/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:14.502985/SRT:RcvQ:w1*E:SRT.mg: reXmit insert SndLossList 1817946039->1817946040 (1817946040)
08:29:14.502999/SRT:RcvQ:w1*E:SRT.mg: @511418812:ENFORCED FASTREXMIT by ACK-TMOUT (scheduling): 1817946040-1817946040 (1 packets)
08:29:14.588087/infiniteloop*E:SRT.d: SND-DROP: Update ack 1817946039 > 1817946041.  send seq: 1817946040 > 1817946040
08:29:14.588103/infiniteloop*E:SRT.d: SND-DROP: %(1817946039-1817946040) n=2pkt 2632B, span=1022 ms, FIRST #52998

So this seems to be behaving as expected.

So, to summarize the entire problem from top to bottom:

• Receiver encounters a stall of some kind (we are not debugging this) and stops ACKing packets.
• Sender tries to retransmit unACKed packet until loss time is exceeded.
• Packets that haven't been sent yet are dropped, putting m_iSndLastAck ahead of m_iSndCurrSeqNo.
• If CUDT::checkRexmitTimer runs before the next packet is sent it inserts a range of X to X-1.
• This violates the assumption of inserting into the loss list (second value is strictly later than first value), and so this range is calculated to be massive and subtracted from the list size, which makes it negative.
• This causes subsequent inserts to mess up the state of the list further, until it gets stuck in the infinite loop on insert.

So then, as far as I can tell, the issue is that there's a problem in the logic for updating m_iSndCurrSeqNo in CUDT::checkNeedDrop. m_iSndCurrSeqNo gets updated by these lines:

            /* If we dropped packets not yet sent, advance current position */
            // THIS MEANS: m_iSndCurrSeqNo = MAX(m_iSndCurrSeqNo, m_iSndLastDataAck-1)
            if (CSeqNo::seqcmp(m_iSndCurrSeqNo, minlastack) < 0)
            {
                m_iSndCurrSeqNo = minlastack;
            }

I'm not understanding why m_iSndCurrSeqNo should be updated to m_iSndLastDataAck-1 rather than just m_iSndLastDataAck. Does it make sense for packets to be ACKed that haven't been sent? The comment also implies that it's fine for this function to drop packets that haven't yet been sent, but I couldn't find logic that actually drops packets from the sending buffer if they've been dropped by CUDT::checkNeedDrop.

[maxsharabayko]

Hi @CommonsNat
Thank you very much for your investigation! I can't express how helpful it is!

Based on your results it is very easy to reproduce the issue you are seeing.

1. Code Modification to Block Sending Packets (Sender Side)

I put return -1; in the very beginning of the CSndUList::pop(..) function, to avoid sending data packets and thus avoid incrementing m_iSndCurrSeqNo.

Initial values for all those variables the retransmission is based on are similar to the one you get after dropping all queued packets.

m_iSndLastAck     = isn;
m_iSndLastDataAck = isn;
m_iSndCurrSeqNo   = CSeqNo::decseq(isn);

Where isn is the initial sequence number.

2. Configuring the Receiver

A very important thing that can be observed from the code of the CUDT::checkRexmitTimer(..) is that in live mode packets are decided to be retransmitted only if the peer (receiver) does not send NAK reports periodically.

// If the receiver will send periodic NAK reports, then FASTREXMIT is inactive.
// MIND that probably some method of "blind rexmit" MUST BE DONE, when TLPKTDROP is off.
if (is_fastrexmit && m_bPeerNakReport)
    return;

Therefore, on the receiving side, there should be some old SRT library (as I remember prior to v1.3.0) or Periodic NAKs must be disabled explicitly. For example, using srt-live-transmit:

srt-live-transmit "srt://:4200?nakreport=0" udp://127.0.0.1:4500 -v

3. Run the Experiment

Running the sender the described code line is executed in a couple of seconds after the start. And the following operation is executed with m_iSndLastAck and m_iSndCurrSeqNo = m_iSndLastAck - 1

m_pSndLossList->insert(m_iSndLastAck, m_iSndCurrSeqNo)

As a result, CSndLossList::m_iLength = -2147483648, which makes the state of the CSndLossList invalid and unexpected.

Status

Working on a fix.

[CommonsNat]

This is awesome news. I'm glad to be of help. Let me know if there is anything else you could use from me.

[maxsharabayko]

The issue found by @CommonsNat was introduced in PR #1181 (Mar 17, 2020) by the introduction of the getFlightSpan() utility function. The function was later modified in #1190.

It means that only v1.4.2 is affected. The initial bug report was before this newly introduced error.

The condition in CUDT::checkRexmitTimer(..):

if (is_fastrexmit && (CSeqNo::seqoff(m_iSndLastAck, CSeqNo::incseq(m_iSndCurrSeqNo)) > 0))

was replaced by:

if (is_fastrexmit && getFlightSpan() != 0)

essentially identical to

CSeqNo::seqlen(m_iSndLastAck, CSeqNo::incseq(m_iSndCurrSeqNo));

CSeqNo::seqoff(A, A) = 0;
CSeqNo::seqlen(A, A) = 1;
CSeqNo::seqoff(A, A) != CSeqNo::seqlen(A, A);

[maxsharabayko]

Next Steps

• [ ] Fix the infinite while loop in the CSndLossList. See this unit test.

• [x] CSndLossList must not allow to add more entries than its size.

• [ ] CSndLossList::seqend could be equal to seqstart instead of -1 in case of one element. It could simplify insert operations.

[CommonsNat]

Hello, I just want to report that I've seen a unit in the field encounter the issue where a range of x to x-1 is inserted into the sndLossList even with #1613 applied. I don't have time to dedicate to debugging it further to see how this is occurring, I just wanted to mention that it still seems to be possible somehow.

[alexpokotilo]

@maxsharabayko
I've checked this hung test https://github.com/Haivision/srt/issues/1000#issuecomment-679008493 with latest fixes https://github.com/Haivision/srt/pull/1733 and the hung is not reproduced. This commit prevents hung https://github.com/Haivision/srt/pull/1733/commits/d5d656922a8a513336e9f4c8d1e4b99e13260fe6

I've analyzed issue reported by @CommonsNat https://github.com/Haivision/srt/issues/1000#issuecomment-742866546 and this commit https://github.com/Haivision/srt/pull/1733/commits/05e66fcd7a965324fd2ca2bde5290a5b9e641222 will prevent this issue as well.

We have a duplicate of this bug https://github.com/Haivision/srt/issues/1662 there we also have hung because of the same problem. It was fixed by:

• https://github.com/Haivision/srt/pull/1673 - we check incoming ACK here and hence prevent insertion of invalid values into CSndLossList
• https://github.com/Haivision/srt/pull/1733 - we made several checks in CSndLossList preventing from old value rewriting, invalid range insertions etc.

Again with https://github.com/Haivision/srt/pull/1733 integrated I don't see any issues described in this ticket reproducible including https://github.com/Haivision/srt/issues/1000#issuecomment-679008493.

Let me copy our TODO list for this task:

Fix the infinite while loop in the CSndLossList. See this unit test.

already fixed by https://github.com/Haivision/srt/pull/1733 so we just need to add https://github.com/Haivision/srt/issues/1000#issuecomment-679008493 test

CSndLossList must not allow to add more entries than its size.

already fixed by https://github.com/Haivision/srt/pull/1733

CSndLossList::seqend could be equal to seqstart instead of -1 in case of one element. It could simplify insert operations.

this is exactly how it's on the master.

I agree that CSndLossList is not "the list of my dreams" and should be probably refactored but I think we need to close this ticket as described issue has been already fixed and merged. and all we need is to add https://github.com/Haivision/srt/issues/1000#issuecomment-679008493 test for hung.

[maxsharabayko]

@alexpokotilo Thank you for your great analysis!

The updated TODO list:

• [x] There is one disabled test, but looks like it is also passing now. CSndLossListTest, DISABLED_InsertFullListNegativeOffset)

• [x] Add test from this comment to the code base.

• [ ] CSndLossList::seqend could be equal to seqstart instead of -1 in case of one element. It could simplify insert operations. (consider creating a separate maintenance issue or adding a TODO comment to the CSndLossList).

---

Regarding the last one, the list saves seqend only if it differs from seqstart (see this code line). Otherwise, seqend remains -1.

Anyway, this is mainly an idea for potential further improvement, not some bug. I was thinking that maybe an entry could be inserted with an offset from seqend, not seqstart. But probably this would also require some other logic to be changed.
So there seems to be not that much work remaining to close this issue.

[alexpokotilo]

So there seems to be not that much work remaining to close this issue.

@maxsharabayko let me know if you need my help with any further reviews of the list.

[maxsharabayko]

Thanks to everyone involved!
Closing this issue as CSndLossList bugs are expected to be fixed now.
Please do not hesitate to reopen if something else arises.