Use locks to prevent race conditions in HCICordioTransport

Author: swnf

src/utility/HCI.cpp

@@ -136,20 +136,24 @@ void HCIClass::poll(unsigned long timeout)
     HCITransport.wait(timeout);
   }
 
+  HCITransport.lockForRead();
   while (HCITransport.available()) {
     byte b = HCITransport.read();
 
     if (_recvIndex >= (int)sizeof(_recvBuffer)) {
         _recvIndex = 0;
         if (_debug) {
+            HCITransport.unlockForRead();
             _debug->println("_recvBuffer overflow");
+            HCITransport.lockForRead();
         }
     }
 
     _recvBuffer[_recvIndex++] = b;
 
     if (_recvBuffer[0] == HCI_ACLDATA_PKT) {
       if (_recvIndex > 5 && _recvIndex >= (5 + (_recvBuffer[3] + (_recvBuffer[4] << 8)))) {
+        HCITransport.unlockForRead();
         if (_debug) {
           dumpPkt("HCI ACLDATA RX <- ", _recvIndex, _recvBuffer);
         }
@@ -164,9 +168,11 @@ void HCIClass::poll(unsigned long timeout)
 #ifdef ARDUINO_AVR_UNO_WIFI_REV2
         digitalWrite(NINA_RTS, LOW);
 #endif
+        HCITransport.lockForRead();
       }
     } else if (_recvBuffer[0] == HCI_EVENT_PKT) {
       if (_recvIndex > 3 && _recvIndex >= (3 + _recvBuffer[2])) {
+        HCITransport.unlockForRead();
         if (_debug) {
           dumpPkt("HCI EVENT RX <- ", _recvIndex, _recvBuffer);
         }
@@ -182,19 +188,23 @@ void HCIClass::poll(unsigned long timeout)
 #ifdef ARDUINO_AVR_UNO_WIFI_REV2
         digitalWrite(NINA_RTS, LOW);
 #endif
+        HCITransport.lockForRead();
       }
     } else {
       _recvIndex = 0;
 
       if (_debug) {
+        HCITransport.unlockForRead();
         _debug->println(b, HEX);
+        HCITransport.lockForRead();
       }
     }
   }
 
 #ifdef ARDUINO_AVR_UNO_WIFI_REV2
   digitalWrite(NINA_RTS, HIGH);
 #endif
+  HCITransport.unlockForRead();
 }
 
 int HCIClass::reset()

src/utility/HCICordioTransport.cpp

@@ -254,8 +254,11 @@ void HCICordioTransportClass::end()
 
 void HCICordioTransportClass::wait(unsigned long timeout)
 {
-  if (available()) {
-    return;
+  {
+    mbed::CriticalSectionLock critical_section;
+    if (available()) {
+      return;
+    }
   }
 
   // wait for handleRxData to signal
@@ -277,6 +280,14 @@ int HCICordioTransportClass::read()
   return _rxBuf.read_char();
 }
 
+void HCICordioTransportClass::lockForRead() {
+  mbed::CriticalSectionLock::enable();
+}
+
+void HCICordioTransportClass::unlockForRead() {
+  mbed::CriticalSectionLock::disable();
+}
+
 size_t HCICordioTransportClass::write(const uint8_t* data, size_t length)
 {
   if (!_begun) {
@@ -299,13 +310,16 @@ size_t HCICordioTransportClass::write(const uint8_t* data, size_t length)
 
 void HCICordioTransportClass::handleRxData(uint8_t* data, uint8_t len)
 {
-  if (_rxBuf.availableForStore() < len) {
-    // drop!
-    return;
-  }
+  {
+    mbed::CriticalSectionLock critical_section;
+    if (_rxBuf.availableForStore() < len) {
+      // drop!
+      return;
+    }
 
-  for (int i = 0; i < len; i++) {
-    _rxBuf.store_char(data[i]);
+    for (int i = 0; i < len; i++) {
+      _rxBuf.store_char(data[i]);
+    }
   }
 
   bleEventFlags.set(0x01);

src/utility/HCICordioTransport.h

@@ -40,6 +40,9 @@ class HCICordioTransportClass : public HCITransportInterface {
   virtual int peek();
   virtual int read();
 
+  virtual void lockForRead() override;
+  virtual void unlockForRead() override;
+
   virtual size_t write(const uint8_t* data, size_t length);
 
 private:

src/utility/HCITransport.h

@@ -33,6 +33,12 @@ class HCITransportInterface {
   virtual int peek() = 0;
   virtual int read() = 0;
 
+  // Some transports require a lock to use available/peek/read
+  // These methods allow to keep the lock while reading an unknown number of bytes
+  // These methods might disable interrupts. Only keep the lock as long as necessary.
+  virtual void lockForRead() {}
+  virtual void unlockForRead() {}
+
   virtual size_t write(const uint8_t* data, size_t length) = 0;
 };