Crash in CRcvLossList::getFirstLostSeq()

[pcamina]

Bug description
My application crashes in the function CRcvLossList::getFirstLostSeq(). When the crash happens m_iHead is -1 whereas m_iLength is 1. This results in a EXC_BAD_ACCESS (SIGSEGV) exception since the index (m_iHead) into the array is -1. It does not happen very often and is difficult to reproduce, but since our application is used by many users there are many hundred SRT connections per day, it gets an issue.

I am wondering if it is a concurrency problem. I have seen that loss list accesses (remove and insert) are protected with the m_RcvLossLock lock. Since getFirstLostSeq() is not protected would it be possible that an ongoing remove operation (e.g. of the last element in the list) is interrupted at a point where m_iHead is already -1 but m_iLength is not yet 0?

To Reproduce
I am using SRT in SRTT_LIVE mode, TSBPD, non-blocking send / receive mode in sender and receiver. Up to nine connections per application.
As mentioned above it is very difficult to reproduce.

Desktop (please provide the following information):

• OS: iOS 13
• SRT Version / commit ID: 1.4.0

Additional context
Otherwise SRT works really well for me!

Cheers
Patrick Camina

[maxsharabayko]

Hi @pcamina
This sounds like something that was fixed recently in PR #1333 and #1335.
Could you please check the latest master?

[pcamina]

Hi @maxsharabayko,

I checked the latest master and getFirstLostSeq() is not protected. Actually, I don't see any changes that address this issue. It seems to me that PR #1333 and #1335 are not related.

Maybe, the m_RcvLossLock lock could be used as well for the getFirstLostSeq() access?

Cheers
Patrick

[maxsharabayko]

@pcamina You are right, it is a different issue. Sorry, read the description too fluently.

getFirstLostSeq(..) does not modify the state of CRcvLossList. But it accesses m_caSeq[m_iHead], and theoretically after it checks m_iLength != 0 the state could be changed by another thread so that m_iHead becomes -1

On the other hand, all the work with CRcvLossList should be performed from the same thread. So the problem might actually come from some place where m_iHead and m_iLength are updated.

[pcamina]

@maxsharabayko.

Actually, CRcvLossList::remove() is called in the CUDT::tsbpd thread, whereas the other CRcvLossListfunctions are performed in the CRcvQueue::worker thread.

So, my scenario is the following:
CRcvLossList::remove() is called in the CUDT::tsbpd thread. While remove(..) modifies the list, getFirstLostSeq(..) is called from the CRcvQueue::worker thread. This could, while removing the last entry, result in m_iLength still being 1 but m_iHeadbeing already -1 which would allow m_caSeq[m_iHead] to be accessed with a negative index.

What do you think?

Patrick

[maxsharabayko]

Right, I forgot about the TSBPD thread. That would explain this crash. Then the protection with m_RcvLossLock is needed.

[pcamina]

I had added the lock and the problem disappeared.