[core] Fixed lock around ackmessage (#1849)

Also fixed wrong log FA qualification basing on the call stack
Haivision · Mar 9, 2021 · d1d351d · d1d351d
1 parent db3db78
commit d1d351d
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 4 deletions.
diff --git a/srtcore/core.cpp b/srtcore/core.cpp
@@ -6006,11 +6006,12 @@ void CUDT::checkNeedDrop(bool& w_bCongestion)
             // from the API call directly. This should be extra verified, if that
             // changes in the future.
             //
-            // What's important is that the lock on GroupLock cannot be applied
-            // here, both because it might be applied already, and because the
-            // locks on the later lock ordered mutexes are already set.
             if (m_parent->m_GroupOf)
             {
+                // What's important is that the lock on GroupLock cannot be applied
+                // here, both because it might be applied already, that is, according
+                // to the condition defined at this function's header, it is applied
+                // under this condition. Hence ackMessage can be defined as 100% locked.
                 m_parent->m_GroupOf->ackMessage(first_msgno);
             }
 #endif
@@ -7741,7 +7742,15 @@ void CUDT::updateSndLossListOnACK(int32_t ackdata_seqno)
         ScopedLock glock (s_UDTUnited.m_GlobControlLock);
         if (m_parent->m_GroupOf)
         {
-            HLOGC(xtlog.Debug, log << "ACK: acking group sender buffer for #" << msgno_at_last_acked_seq);
+            HLOGC(inlog.Debug, log << "ACK: acking group sender buffer for #" << msgno_at_last_acked_seq);
+
+            // Guard access to m_iSndAckedMsgNo field
+            // Note: This can't be done inside CUDTGroup::ackMessage
+            // because this function is also called from CUDT::checkNeedDrop
+            // called from CUDT::sendmsg2 called from CUDTGroup::send, which
+            // applies the lock on m_GroupLock already.
+            ScopedLock glk (*m_parent->m_GroupOf->exp_groupLock());
+
             // NOTE: ackMessage also accepts and ignores the trap representation
             // which is SRT_MSGNO_CONTROL.
             m_parent->m_GroupOf->ackMessage(msgno_at_last_acked_seq);

diff --git a/srtcore/group.cpp b/srtcore/group.cpp
@@ -4216,6 +4216,7 @@ int32_t CUDTGroup::addMessageToBuffer(const char* buf, size_t len, SRT_MSGCTRL&
         // Very first packet, just set the msgno.
         m_iSndAckedMsgNo  = w_mc.msgno;
         m_iSndOldestMsgNo = w_mc.msgno;
+        HLOGC(gslog.Debug, log << "addMessageToBuffer: initial message no #" << w_mc.msgno);
     }
     else if (m_iSndOldestMsgNo != m_iSndAckedMsgNo)
     {
@@ -4241,6 +4242,8 @@ int32_t CUDTGroup::addMessageToBuffer(const char* buf, size_t len, SRT_MSGCTRL&
 
         // Position at offset is not included
         m_iSndOldestMsgNo = m_iSndAckedMsgNo;
+        HLOGC(gslog.Debug,
+              log << "addMessageToBuffer: ... after: oldest #" << m_iSndOldestMsgNo);
     }
 
     m_SenderBuffer.resize(m_SenderBuffer.size() + 1);
@@ -4338,6 +4341,7 @@ int CUDTGroup::sendBackupRexmit(CUDT& core, SRT_MSGCTRL& w_mc)
     return stat;
 }
 
+// [[using locked(CUDTGroup::m_GroupLock)]];
 void CUDTGroup::ackMessage(int32_t msgno)
 {
     // The message id could not be identified, skip.