Change in osmo-trx[master]: Transceiver: Fix race condition obtaining Dl burst from Upper layer

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

pespin gerrit-no-reply at lists.osmocom.org
Tue Jul 14 09:43:07 UTC 2020


pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/19206 )

Change subject: Transceiver: Fix race condition obtaining Dl burst from Upper layer
......................................................................

Transceiver: Fix race condition obtaining Dl burst from Upper layer

The queue was being accessed sequentially obtaining and releasing the
mutual exclusion zone twice. First in getStaleBurst() dropping all
FN<currTime, then in getCurrentBurst() trying to obtain FN=currTime.

However, since in between the mutex is released, it could happen that
for instance upper layer would introduce currTime-1 in the queue, which
would make then getCurrentBurst() detect that one instead of potential
currTime in the queue and return NULL.

By holding the mutex during the call to both functions we make sure the
state is kept during the whole transaction.

Related: OS#4487 (comment #7)
Change-Id: If1fd8d7fc5f21ee2894192ef1ac2a3cdda6bbb98
---
M Transceiver52M/Transceiver.cpp
M Transceiver52M/radioVector.cpp
M Transceiver52M/radioVector.h
3 files changed, 17 insertions(+), 23 deletions(-)

Approvals:
  fixeria: Looks good to me, but someone else must approve
  laforge: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/Transceiver52M/Transceiver.cpp b/Transceiver52M/Transceiver.cpp
index d45a28b..7aaf1d4 100644
--- a/Transceiver52M/Transceiver.cpp
+++ b/Transceiver52M/Transceiver.cpp
@@ -433,9 +433,15 @@
   std::vector<bool> filler(mChans, true);
   bool stale_bursts_changed;
 
+  TN = nowTime.TN();
+
   for (size_t i = 0; i < mChans; i ++) {
     state = &mStates[i];
     stale_bursts_changed = false;
+    zeros[i] = state->chanType[TN] == NONE;
+
+    Mutex *mtx = mTxPriorityQueues[i].getMutex();
+    mtx->lock();
 
     while ((burst = mTxPriorityQueues[i].getStaleBurst(nowTime))) {
       LOGCHAN(i, DTRXDDL, NOTICE) << "dumping STALE burst in TRX->SDR interface ("
@@ -447,15 +453,6 @@
       delete burst;
     }
 
-    if (stale_bursts_changed)
-      dispatch_trx_rate_ctr_change(state, i);
-
-    TN = nowTime.TN();
-    modFN = nowTime.FN() % state->fillerModulus[TN];
-
-    bursts[i] = state->fillerTable[modFN][TN];
-    zeros[i] = state->chanType[TN] == NONE;
-
     if ((burst = mTxPriorityQueues[i].getCurrentBurst(nowTime))) {
       bursts[i] = burst->getVector();
 
@@ -467,7 +464,15 @@
       }
 
       delete burst;
+    } else {
+      modFN = nowTime.FN() % state->fillerModulus[TN];
+      bursts[i] = state->fillerTable[modFN][TN];
     }
+
+    mtx->unlock();
+
+    if (stale_bursts_changed)
+      dispatch_trx_rate_ctr_change(state, i);
   }
 
   mRadioInterface->driveTransmitRadio(bursts, zeros);
diff --git a/Transceiver52M/radioVector.cpp b/Transceiver52M/radioVector.cpp
index ad40a11..68e42c5 100644
--- a/Transceiver52M/radioVector.cpp
+++ b/Transceiver52M/radioVector.cpp
@@ -120,38 +120,26 @@
 
 radioVector* VectorQueue::getStaleBurst(const GSM::Time& targTime)
 {
-	mLock.lock();
-	if ((mQ.size()==0)) {
-		mLock.unlock();
+	if ((mQ.size()==0))
 		return NULL;
-	}
 
 	if (mQ.top()->getTime() < targTime) {
 		radioVector* retVal = mQ.top();
 		mQ.pop();
-		mLock.unlock();
 		return retVal;
 	}
-	mLock.unlock();
-
 	return NULL;
 }
 
 radioVector* VectorQueue::getCurrentBurst(const GSM::Time& targTime)
 {
-	mLock.lock();
-	if ((mQ.size()==0)) {
-		mLock.unlock();
+	if ((mQ.size()==0))
 		return NULL;
-	}
 
 	if (mQ.top()->getTime() == targTime) {
 		radioVector* retVal = mQ.top();
 		mQ.pop();
-		mLock.unlock();
 		return retVal;
 	}
-	mLock.unlock();
-
 	return NULL;
 }
diff --git a/Transceiver52M/radioVector.h b/Transceiver52M/radioVector.h
index 0a14a4d..84e3987 100644
--- a/Transceiver52M/radioVector.h
+++ b/Transceiver52M/radioVector.h
@@ -65,6 +65,7 @@
 	GSM::Time nextTime() const;
 	radioVector* getStaleBurst(const GSM::Time& targTime);
 	radioVector* getCurrentBurst(const GSM::Time& targTime);
+	Mutex *getMutex() const { return &mLock; };
 };
 
 #endif /* RADIOVECTOR_H */

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/19206
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: If1fd8d7fc5f21ee2894192ef1ac2a3cdda6bbb98
Gerrit-Change-Number: 19206
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200714/14cb8414/attachment.htm>


More information about the gerrit-log mailing list