Fix async NSAPI race condition

Remove read and write mutexes since multiple calls to send or multiple
calls to recv on different threads is undefined behavior.
This is because the size of data sent or received by these calls is
undefined and could lead to the data being interleaved.  The code now
asserts that there are not multiple threads calling send at the same
or calling recv at the same time.  Note that calling send and recv from
different threads at the same time is still safe and well defined
behavior.

By removing the read and write mutexes and associated timeout it
guarantees that a stack call will always be made and thus the value
NSAPI_ERROR_TIMEOUT cannot get falsely returned.

Author: c1728p9

TCPSocket.cpp

@@ -16,21 +16,25 @@
 
 #include "TCPSocket.h"
 #include "Timer.h"
+#include "mbed_assert.h"
 
 TCPSocket::TCPSocket()
-    : _pending(0), _read_sem(0), _write_sem(0)
+    : _pending(0), _read_sem(0), _write_sem(0),
+      _read_in_progress(false), _write_in_progress(false)
 {
 }
 
 TCPSocket::TCPSocket(NetworkStack *iface)
-    : _pending(0), _read_sem(0), _write_sem(0)
+    : _pending(0), _read_sem(0), _write_sem(0),
+      _read_in_progress(false), _write_in_progress(false)
 {
     // TCPSocket::open is thread safe
     open(iface);
 }
 
 TCPSocket::TCPSocket(NetworkInterface *iface)
-    : _pending(0), _read_sem(0), _write_sem(0)
+    : _pending(0), _read_sem(0), _write_sem(0),
+      _read_in_progress(false), _write_in_progress(false)
 {
     // TCPSocket::open is thread safe
     open(iface->get_stack());
@@ -82,10 +86,12 @@ int TCPSocket::connect(const char *host, uint16_t port)
 
 int TCPSocket::send(const void *data, unsigned size)
 {
-    if (osOK != _write_lock.lock(_timeout)) {
-        return NSAPI_ERROR_WOULD_BLOCK;
-    }
     _lock.lock();
+    // If this assert is hit then there are two threads
+    // performing a send at the same time which is undefined
+    // behavior
+    MBED_ASSERT(!_write_in_progress);
+    _write_in_progress = true;
 
     int ret;
     while (true) {
@@ -116,17 +122,19 @@ int TCPSocket::send(const void *data, unsigned size)
         }
     }
 
+    _write_in_progress = false;
     _lock.unlock();
-    _write_lock.unlock();
     return ret;
 }
 
 int TCPSocket::recv(void *data, unsigned size)
 {
-    if (osOK != _read_lock.lock(_timeout)) {
-        return NSAPI_ERROR_WOULD_BLOCK;
-    }
     _lock.lock();
+    // If this assert is hit then there are two threads
+    // performing a recv at the same time which is undefined
+    // behavior
+    MBED_ASSERT(!_read_in_progress);
+    _read_in_progress = true;
 
     int ret;
     while (true) {
@@ -157,8 +165,8 @@ int TCPSocket::recv(void *data, unsigned size)
         }
     }
 
+    _read_in_progress = false;
     _lock.unlock();
-    _read_lock.unlock();
     return ret;
 }
 

TCPSocket.h

@@ -132,10 +132,10 @@ class TCPSocket : public Socket {
 protected:
     virtual void event();
     volatile unsigned _pending;
-    rtos::Mutex _read_lock;
     rtos::Semaphore _read_sem;
-    rtos::Mutex _write_lock;
     rtos::Semaphore _write_sem;
+    bool _read_in_progress;
+    bool _write_in_progress;
     friend class TCPServer;
 };
 

UDPSocket.cpp

@@ -16,20 +16,24 @@
 
 #include "UDPSocket.h"
 #include "Timer.h"
+#include "mbed_assert.h"
 
 UDPSocket::UDPSocket()
-    : _pending(0), _read_sem(0), _write_sem(0)
+    : _pending(0), _read_sem(0), _write_sem(0),
+      _read_in_progress(false), _write_in_progress(false)
 {
 }
 
 UDPSocket::UDPSocket(NetworkStack *iface)
-    : _pending(0), _read_sem(0), _write_sem(0)
+    : _pending(0), _read_sem(0), _write_sem(0),
+      _read_in_progress(false), _write_in_progress(false)
 {
     open(iface);
 }
 
 UDPSocket::UDPSocket(NetworkInterface *iface)
-    : _pending(0), _read_sem(0), _write_sem(0)
+    : _pending(0), _read_sem(0), _write_sem(0),
+      _read_in_progress(false), _write_in_progress(false)
 {
     open(iface->get_stack());
 }
@@ -64,10 +68,12 @@ int UDPSocket::sendto(const char *host, uint16_t port, const void *data, unsigne
 
 int UDPSocket::sendto(const SocketAddress &address, const void *data, unsigned size)
 {
-    if (osOK != _write_lock.lock(_timeout)) {
-        return NSAPI_ERROR_WOULD_BLOCK;
-    }
     _lock.lock();
+    // If this assert is hit then there are two threads
+    // performing a send at the same time which is undefined
+    // behavior
+    MBED_ASSERT(!_write_in_progress);
+    _write_in_progress = true;
 
     int ret;
     while (true) {
@@ -98,17 +104,19 @@ int UDPSocket::sendto(const SocketAddress &address, const void *data, unsigned s
         }
     }
 
+    _write_in_progress = false;
     _lock.unlock();
-    _write_lock.unlock();
     return ret;
 }
 
 int UDPSocket::recvfrom(SocketAddress *address, void *buffer, unsigned size)
 {
-    if (osOK != _read_lock.lock(_timeout)) {
-        return NSAPI_ERROR_WOULD_BLOCK;
-    }
     _lock.lock();
+    // If this assert is hit then there are two threads
+    // performing a recv at the same time which is undefined
+    // behavior
+    MBED_ASSERT(!_read_in_progress);
+    _read_in_progress = true;
 
     int ret;
     while (true) {
@@ -139,8 +147,8 @@ int UDPSocket::recvfrom(SocketAddress *address, void *buffer, unsigned size)
         }
     }
 
+    _read_in_progress = false;
     _lock.unlock();
-    _read_lock.unlock();
     return ret;
 }
 

UDPSocket.h

@@ -131,10 +131,10 @@ class UDPSocket : public Socket {
 protected:
     virtual void event();
     volatile unsigned _pending;
-    rtos::Mutex _read_lock;
     rtos::Semaphore _read_sem;
-    rtos::Mutex _write_lock;
     rtos::Semaphore _write_sem;
+    bool _read_in_progress;
+    bool _write_in_progress;
 };
 
 #endif