Cellular: fix ATHandler destructor possible crash on delete

In some multithread cases there is possibility that process_oob function
was called after ATHandler was deleted. Fix is to cancel eventqueue if it's
holding an event and wait if oob processing is ongoing.

Author: Teppo Järvelin

UNITTESTS/features/cellular/framework/AT/at_cellularbase/unittest.cmake

@@ -26,4 +26,6 @@ set(unittest-test-sources
   stubs/ATHandler_stub.cpp
   stubs/EventQueue_stub.cpp
   stubs/FileHandle_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

UNITTESTS/features/cellular/framework/AT/at_cellularcontext/unittest.cmake

@@ -42,4 +42,6 @@ set(unittest-test-sources
   stubs/UARTSerial_stub.cpp
   stubs/SerialBase_stub.cpp
   stubs/CellularContext_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

UNITTESTS/features/cellular/framework/AT/at_cellulardevice/unittest.cmake

@@ -44,6 +44,8 @@ set(unittest-test-sources
   stubs/SerialBase_stub.cpp
   stubs/CellularStateMachine_stub.cpp
   stubs/CellularContext_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )
 
 # defines

UNITTESTS/features/cellular/framework/AT/at_cellularinformation/unittest.cmake

@@ -25,4 +25,6 @@ set(unittest-test-sources
   stubs/EventQueue_stub.cpp
   stubs/FileHandle_stub.cpp
   stubs/mbed_assert_stub.c
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

UNITTESTS/features/cellular/framework/AT/at_cellularnetwork/unittest.cmake

@@ -28,4 +28,6 @@ set(unittest-test-sources
   stubs/mbed_assert_stub.c
   stubs/SocketAddress_stub.cpp
   stubs/randLIB_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

UNITTESTS/features/cellular/framework/AT/at_cellularsms/unittest.cmake

@@ -27,4 +27,6 @@ set(unittest-test-sources
   stubs/us_ticker_stub.cpp
   stubs/mbed_assert_stub.c
   stubs/mbed_wait_api_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

UNITTESTS/features/cellular/framework/AT/at_cellularstack/unittest.cmake

@@ -28,4 +28,6 @@ set(unittest-test-sources
   stubs/NetworkStack_stub.cpp
   stubs/SocketAddress_stub.cpp
   stubs/mbed_assert_stub.c
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

UNITTESTS/features/cellular/framework/AT/athandler/unittest.cmake

@@ -5,10 +5,12 @@
 
 # Add test specific include paths
 set(unittest-includes ${unittest-includes}
-  features/cellular/framework/common/util
+  ../platform
+  ../features/cellular/framework/common/util
   ../features/cellular/framework/common
   ../features/cellular/framework/AT
   ../features/frameworks/mbed-client-randlib/mbed-client-randlib
+  
 )
 
 # Source files
@@ -33,6 +35,8 @@ set(unittest-test-sources
   stubs/Kernel_stub.cpp
   stubs/ThisThread_stub.cpp
   stubs/randLIB_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )
 
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DMBED_CONF_CELLULAR_DEBUG_AT=true -DOS_STACK_SIZE=2048")

UNITTESTS/features/cellular/framework/device/cellulardevice/unittest.cmake

@@ -33,6 +33,8 @@ set(unittest-test-sources
   stubs/NetworkInterface_stub.cpp
   stubs/NetworkInterfaceDefaults_stub.cpp
   stubs/CellularContext_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )
 
 # defines

UNITTESTS/features/cellular/framework/device/cellularstatemachine/unittest.cmake

@@ -39,6 +39,8 @@ set(unittest-test-sources
   stubs/EventQueue_stub.cpp
   stubs/equeue_stub.c
   stubs/CellularContext_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )
 
 # defines

UNITTESTS/stubs/ATHandler_stub.cpp

@@ -81,6 +81,9 @@ void ATHandler_stub::debug_call_count_clear()
 
 ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay) :
     _nextATHandler(0),
+#ifdef AT_HANDLER_MUTEX
+    _oobCv(_fileHandleMutex),
+#endif
     _fileHandle(fh),
     _queue(queue),
     _ref_count(1),

UNITTESTS/stubs/ConditionVariable_stub.cpp

@@ -0,0 +1,54 @@
+/*
+ * Copyright (c) 2019, Arm Limited and affiliates.
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef MBED_OS_UNITTESTS_STUBS_CONDITIONVARIABLE_STUB_CPP_
+#define MBED_OS_UNITTESTS_STUBS_CONDITIONVARIABLE_STUB_CPP_
+
+#include "ConditionVariable_stub.h"
+#include "mbed_error.h"
+#include "mbed_assert.h"
+
+bool ConditionVariable_stub::time_out = false;
+using namespace rtos;
+
+ConditionVariable::ConditionVariable(Mutex &mutex): _mutex(mutex), _wait_list(0)
+{
+    // No initialization to do
+}
+
+ConditionVariable::~ConditionVariable()
+{
+}
+
+void ConditionVariable::notify_one()
+{
+
+}
+
+void ConditionVariable::wait()
+{
+}
+
+bool ConditionVariable::wait_for(uint32_t millisec)
+{
+    return ConditionVariable_stub::time_out;
+}
+
+void ConditionVariable::notify_all()
+{
+}
+
+#endif /* MBED_OS_UNITTESTS_STUBS_CONDITIONVARIABLE_STUB_CPP_ */

UNITTESTS/stubs/ConditionVariable_stub.h

@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2019, Arm Limited and affiliates.
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef MBED_OS_UNITTESTS_STUBS_CONDITIONVARIABLE_STUB_H_
+#define MBED_OS_UNITTESTS_STUBS_CONDITIONVARIABLE_STUB_H_
+
+#include "rtos/ConditionVariable.h"
+
+namespace ConditionVariable_stub {
+extern bool time_out;
+}
+
+
+#endif /* MBED_OS_UNITTESTS_STUBS_CONDITIONVARIABLE_STUB_H_ */

features/cellular/framework/AT/ATHandler.cpp

@@ -26,6 +26,7 @@
 #include "rtos/ThisThread.h"
 #include "Kernel.h"
 #include "CellularUtil.h"
+#include "ScopedLock.h"
 
 using namespace mbed;
 using namespace events;
@@ -63,6 +64,9 @@ static const uint8_t map_3gpp_errors[][2] =  {
 
 ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay) :
     _nextATHandler(0),
+#ifdef AT_HANDLER_MUTEX
+    _oobCv(_fileHandleMutex),
+#endif
     _fileHandle(NULL), // filehandle is set by set_file_handle()
     _queue(queue),
     _last_err(NSAPI_ERROR_OK),
@@ -85,7 +89,9 @@ ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, uint32_t timeout, const
     _debug_on(MBED_CONF_CELLULAR_DEBUG_AT),
     _cmd_start(false),
     _use_delimiter(true),
-    _start_time(0)
+    _start_time(0),
+    _event_id(-1),
+    _oob_pending(false)
 {
     clear_error();
 
@@ -120,8 +126,15 @@ bool ATHandler::get_debug() const
 
 ATHandler::~ATHandler()
 {
+    ScopedLock <ATHandler> lock(*this);
     set_file_handle(NULL);
 
+    // cancel possible oob event from queue and wait if oob is ongoing
+    _queue.cancel(_event_id);
+    while (_oob_pending) {
+        _oobCv.wait();
+    }
+
     while (_oobs) {
         struct oob_t *oob = _oobs;
         _oobs = oob->next;
@@ -154,6 +167,7 @@ FileHandle *ATHandler::get_file_handle()
 
 void ATHandler::set_file_handle(FileHandle *fh)
 {
+    ScopedLock<ATHandler> lock(*this);
     if (_fileHandle) {
         set_is_filehandle_usable(false);
     }
@@ -165,6 +179,7 @@ void ATHandler::set_file_handle(FileHandle *fh)
 
 void ATHandler::set_is_filehandle_usable(bool usable)
 {
+    ScopedLock<ATHandler> lock(*this);
     if (_fileHandle) {
         if (usable) {
             _fileHandle->set_blocking(false);
@@ -241,7 +256,7 @@ void ATHandler::event()
 {
     if (!_oob_queued) {
         _oob_queued = true;
-        (void) _queue.call(Callback<void(void)>(this, &ATHandler::process_oob));
+        _event_id = _queue.call(Callback<void(void)>(this, &ATHandler::process_oob));
     }
 }
 
@@ -256,12 +271,12 @@ void ATHandler::lock()
 
 void ATHandler::unlock()
 {
+    if (_is_fh_usable && (_fileHandle->readable() || (_recv_pos < _recv_len))) {
+        _event_id = _queue.call(Callback<void(void)>(this, &ATHandler::process_oob));
+    }
 #ifdef AT_HANDLER_MUTEX
     _fileHandleMutex.unlock();
 #endif
-    if (_fileHandle->readable() || (_recv_pos < _recv_len)) {
-        (void) _queue.call(Callback<void(void)>(this, &ATHandler::process_oob));
-    }
 }
 
 nsapi_error_t ATHandler::unlock_return_error()
@@ -295,12 +310,15 @@ void ATHandler::restore_at_timeout()
 
 void ATHandler::process_oob()
 {
+    ScopedLock<ATHandler> lock(*this);
+    _oob_pending = true;
+    _oob_queued = false;
     if (!_is_fh_usable) {
         tr_debug("process_oob, filehandle is not usable, return...");
+        _oob_pending = false;
+        _oobCv.notify_all();
         return;
     }
-    lock();
-    _oob_queued = false;
     if (_fileHandle->readable() || (_recv_pos < _recv_len)) {
         tr_debug("AT OoB readable %d, len %u", _fileHandle->readable(), _recv_len - _recv_pos);
         _current_scope = NotSet;
@@ -326,7 +344,8 @@ void ATHandler::process_oob()
         _at_timeout = timeout;
         tr_debug("AT OoB done");
     }
-    unlock();
+    _oob_pending = false;
+    _oobCv.notify_all();
 }
 
 void ATHandler::reset_buffer()

features/cellular/framework/AT/ATHandler.h

@@ -21,16 +21,9 @@
 #include "platform/mbed_retarget.h"
 
 #include "events/EventQueue.h"
-#include "PlatformMutex.h"
 #include "nsapi_types.h"
-
-#include "PlatformMutex.h"
 #include "Callback.h"
 
-namespace mbed {
-
-class FileHandle;
-
 /**
  * If application calls associated FileHandle only from single thread context
   * then locking between AT command and response is not needed. However,
@@ -39,6 +32,14 @@ class FileHandle;
   */
 #define AT_HANDLER_MUTEX
 
+#ifdef AT_HANDLER_MUTEX
+#include "ConditionVariable.h"
+#endif
+
+namespace mbed {
+
+class FileHandle;
+
 extern const char *OK;
 extern const char *CRLF;
 
@@ -212,7 +213,8 @@ class ATHandler {
 protected:
     void event();
 #ifdef AT_HANDLER_MUTEX
-    PlatformMutex _fileHandleMutex;
+    rtos::Mutex _fileHandleMutex;
+    rtos::ConditionVariable _oobCv;
 #endif
     FileHandle *_fileHandle;
 private:
@@ -496,6 +498,10 @@ class ATHandler {
 
     // time when a command or an URC processing was started
     uint64_t _start_time;
+    // eventqueue event id
+    int _event_id;
+    // used to check if oob processing is ongoing
+    bool _oob_pending;
 
     // Gets char from receiving buffer.
     // Resets and fills the buffer if all are already read (receiving position equals receiving length).

features/cellular/framework/AT/AT_CellularStack.h

@@ -20,6 +20,7 @@
 
 #include "AT_CellularBase.h"
 #include "NetworkStack.h"
+#include "PlatformMutex.h"
 
 namespace mbed {