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 wait if oob processing
is ongoing.

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

@@ -25,4 +25,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/CellularContext_stub.cpp
   stubs/CellularUtil_stub.cpp
   stubs/SocketAddress_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

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

@@ -43,6 +43,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,7 @@ set(unittest-test-sources
   stubs/us_ticker_stub.cpp
   stubs/mbed_assert_stub.c
   stubs/ThisThread_stub.cpp
+  stubs/mbed_wait_api_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )

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

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

UNITTESTS/features/cellular/framework/AT/athandler/athandlertest.cpp

@@ -51,6 +51,7 @@ protected:
         urc_callback_count = 0;
         CellularUtil_stub::char_ptr = NULL;
         CellularUtil_stub::char_pos = 0;
+        filehandle_stub_short_value_counter = 0;
     }
 
     void TearDown()
@@ -352,6 +353,7 @@ TEST_F(TestATHandler, test_ATHandler_cmd_start)
 {
     EventQueue que;
     FileHandle_stub fh1;
+    fh1.short_value = 0;
 
     ATHandler at(&fh1, que, 0, ",");
     mbed_poll_stub::revents_value = POLLOUT;

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
@@ -31,6 +33,10 @@ set(unittest-test-sources
   stubs/ThisThread_stub.cpp
   stubs/randLIB_stub.cpp
   stubs/CellularUtil_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
+  stubs/mbed_rtos_rtx_stub.c
+  stubs/rtx_mutex_stub.c
 )
 
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DMBED_CONF_CELLULAR_DEBUG_AT=true -DOS_STACK_SIZE=2048")

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

@@ -35,6 +35,8 @@ set(unittest-test-sources
   stubs/CellularDevice_stub.cpp
   stubs/equeue_stub.c
   stubs/ThisThread_stub.cpp
+  stubs/ConditionVariable_stub.cpp
+  stubs/Mutex_stub.cpp
 )
 
 # defines

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

@@ -38,6 +38,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/features/lorawan/loramac/unittest.cmake

@@ -40,10 +40,13 @@ set(unittest-test-sources
   stubs/LoRaWANTimer_stub.cpp
   stubs/LoRaMacCommand_stub.cpp
   stubs/EventQueue_stub.cpp
-
+  stubs/Mutex_stub.cpp
 )
 
 # defines
+#set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DMBED_CONF_RTOS_PRESENT=1")
+#set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DMBED_CONF_RTOS_PRESENT=1")
+
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DMBED_CONF_LORA_ADR_ON=true -DMBED_CONF_LORA_PUBLIC_NETWORK=true -DMBED_CONF_LORA_NB_TRIALS=2 -DMBED_CONF_LORA_DOWNLINK_PREAMBLE_LENGTH=5")
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DMBED_CONF_LORA_ADR_ON=true -DMBED_CONF_LORA_PUBLIC_NETWORK=true -DMBED_CONF_LORA_NB_TRIALS=2 -DMBED_CONF_LORA_DOWNLINK_PREAMBLE_LENGTH=5")
 

UNITTESTS/features/lorawan/lorawaninterface/unittest.cmake

@@ -41,6 +41,7 @@ set(unittest-test-sources
   stubs/LoRaWANTimer_stub.cpp
   stubs/LoRaMacCommand_stub.cpp
   stubs/LoRaPHYEU868_stub.cpp
+  stubs/Mutex_stub.cpp
 )
 
 # defines

UNITTESTS/features/lorawan/lorawanstack/unittest.cmake

@@ -42,7 +42,7 @@ set(unittest-test-sources
   stubs/LoRaMacCommand_stub.cpp
   stubs/EventQueue_stub.cpp
   stubs/equeue_stub.c
-
+  stubs/Mutex_stub.cpp
 )
 
 # defines

UNITTESTS/stubs/ATHandler_stub.cpp

@@ -83,6 +83,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),
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
+    _oobCv(_fileHandleMutex),
+#endif
     _fileHandle(fh),
     _queue(queue),
     _ref_count(1),

UNITTESTS/stubs/ConditionVariable_stub.cpp

@@ -0,0 +1,55 @@
+/*
+ * 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;
+#ifdef MBED_CONF_RTOS_PRESENT
+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
+#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_ */

UNITTESTS/stubs/mbed_rtos_rtx_stub.c

@@ -0,0 +1,20 @@
+/*
+ * 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.
+ */
+
+#include "cmsis_os2.h"
+
+osMutexId_t               singleton_mutex_id;

UNITTESTS/stubs/rtx_mutex_stub.c

@@ -0,0 +1,28 @@
+/*
+ * 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.
+ */
+
+#include "cmsis_os2.h"
+
+osStatus_t osMutexAcquire(osMutexId_t mutex_id, uint32_t timeout)
+{
+    return osOK;
+}
+
+osStatus_t osMutexRelease(osMutexId_t mutex_id)
+{
+    return osOK;
+}

UNITTESTS/target_h/cmsis_os2.h

@@ -18,14 +18,16 @@
 #ifndef __CMSIS_OS2_H__
 #define __CMSIS_OS2_H__
 
+#ifdef  __cplusplus
+extern "C"
+{
+#endif
+
 #include <inttypes.h>
 
 //If conflicts, then remove these, copied from cmsis_os.h
 typedef int32_t                  osStatus;
 
-#define osOK 0
-#define osErrorNoMemory -5
-
 
 //These are from cmsis_os2.h
 typedef void *osSemaphoreId_t;
@@ -37,6 +39,18 @@ typedef struct {
     uint32_t                   cb_size;   ///< size of provided memory for control block
 } osSemaphoreAttr_t;
 
+/// Status code values returned by CMSIS-RTOS functions.
+typedef enum {
+    osOK                      =  0,         ///< Operation completed successfully.
+    osError                   = -1,         ///< Unspecified RTOS error: run-time error but no other error message fits.
+    osErrorTimeout            = -2,         ///< Operation not completed within the timeout period.
+    osErrorResource           = -3,         ///< Resource not available.
+    osErrorParameter          = -4,         ///< Parameter error.
+    osErrorNoMemory           = -5,         ///< System is out of memory: it was impossible to allocate or reserve memory for the operation.
+    osErrorISR                = -6,         ///< Not allowed in ISR context: the function cannot be called from interrupt service routines.
+    osStatusReserved          = 0x7FFFFFFF  ///< Prevents enum down-size compiler optimization.
+} osStatus_t;
+
 //Thread
 typedef enum {
     osPriorityNormal        = 24       ///< Priority: normal
@@ -44,6 +58,8 @@ typedef enum {
 
 typedef void *osThreadId_t;
 
+typedef void *osMutexId_t;
+
 typedef void *osEventFlagsId_t;
 
 /// Attributes structure for thread.
@@ -74,5 +90,19 @@ typedef struct {
 #define osMutexPrioInherit    0x00000002U ///< Priority inherit protocol.
 #define osMutexRobust         0x00000008U ///< Robust mutex.
 
+/// Acquire a Mutex or timeout if it is locked.
+/// \param[in]     mutex_id      mutex ID obtained by \ref osMutexNew.
+/// \param[in]     timeout       \ref CMSIS_RTOS_TimeOutValue or 0 in case of no time-out.
+/// \return status code that indicates the execution status of the function.
+osStatus_t osMutexAcquire(osMutexId_t mutex_id, uint32_t timeout);
+
+/// Release a Mutex that was acquired by \ref osMutexAcquire.
+/// \param[in]     mutex_id      mutex ID obtained by \ref osMutexNew.
+/// \return status code that indicates the execution status of the function.
+osStatus_t osMutexRelease(osMutexId_t mutex_id);
+
+#ifdef  __cplusplus
+}
+#endif
 
 #endif

features/cellular/framework/AT/ATHandler.cpp

@@ -26,6 +26,7 @@
 #include "Kernel.h"
 #include "CellularUtil.h"
 #include "SingletonPtr.h"
+#include "ScopedLock.h"
 
 using namespace mbed;
 using namespace events;
@@ -161,6 +162,9 @@ bool ATHandler::ok_to_proceed()
 
 ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, uint32_t timeout, const char *output_delimiter, uint16_t send_delay) :
     _nextATHandler(0),
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
+    _oobCv(_fileHandleMutex),
+#endif
     _fileHandle(NULL), // filehandle is set by set_file_handle()
     _queue(queue),
     _last_err(NSAPI_ERROR_OK),
@@ -171,7 +175,6 @@ ATHandler::ATHandler(FileHandle *fh, EventQueue &queue, uint32_t timeout, const
     _previous_at_timeout(timeout),
     _at_send_delay(send_delay),
     _last_response_stop(0),
-    _oob_queued(false),
     _ref_count(1),
     _is_fh_usable(false),
     _stop_tag(NULL),
@@ -183,7 +186,8 @@ 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(0)
 {
     clear_error();
 
@@ -218,8 +222,22 @@ bool ATHandler::get_debug() const
 
 ATHandler::~ATHandler()
 {
+    ScopedLock <ATHandler> lock(*this);
     set_file_handle(NULL);
 
+    if (_event_id != 0 && _queue.cancel(_event_id)) {
+        _event_id = 0;
+    }
+
+    while (_event_id != 0) {
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
+        _oobCv.wait();
+#else
+        // Cancel will always work in a single threaded environment
+        MBED_ASSERT(false);
+#endif // AT_HANDLER_MUTEX
+    }
+
     while (_oobs) {
         struct oob_t *oob = _oobs;
         _oobs = oob->next;
@@ -252,6 +270,7 @@ FileHandle *ATHandler::get_file_handle()
 
 void ATHandler::set_file_handle(FileHandle *fh)
 {
+    ScopedLock<ATHandler> lock(*this);
     if (_fileHandle) {
         set_is_filehandle_usable(false);
     }
@@ -263,6 +282,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);
@@ -337,15 +357,14 @@ bool ATHandler::find_urc_handler(const char *prefix)
 
 void ATHandler::event()
 {
-    if (!_oob_queued) {
+    if (_event_id == 0) {
-        _oob_queued = true;
+        _event_id = _queue.call(callback(this, &ATHandler::process_oob));
-        (void) _queue.call(Callback<void(void)>(this, &ATHandler::process_oob));
     }
 }
 
 void ATHandler::lock()
 {
-#ifdef AT_HANDLER_MUTEX
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
     _fileHandleMutex.lock();
 #endif
     clear_error();
@@ -354,12 +373,12 @@ void ATHandler::lock()
 
 void ATHandler::unlock()
 {
-#ifdef AT_HANDLER_MUTEX
+    if (_is_fh_usable && (_fileHandle->readable() || (_recv_pos < _recv_len))) {
+        _event_id = _queue.call(callback(this, &ATHandler::process_oob));
+    }
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
     _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()
@@ -393,12 +412,15 @@ void ATHandler::restore_at_timeout()
 
 void ATHandler::process_oob()
 {
+    ScopedLock<ATHandler> lock(*this);
     if (!_is_fh_usable) {
         tr_debug("process_oob, filehandle is not usable, return...");
+        _event_id = 0;
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
+        _oobCv.notify_all();
+#endif
         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;
@@ -424,7 +446,10 @@ void ATHandler::process_oob()
         _at_timeout = timeout;
         tr_debug("AT OoB done");
     }
-    unlock();
+    _event_id = 0;
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
+    _oobCv.notify_all();
+#endif
 }
 
 void ATHandler::reset_buffer()

features/cellular/framework/AT/ATHandler.h

@@ -21,17 +21,12 @@
 #include "platform/mbed_retarget.h"
 
 #include "events/EventQueue.h"
-#include "PlatformMutex.h"
 #include "nsapi_types.h"
 
 #include "Callback.h"
 
 #include <cstdarg>
 
-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,
@@ -40,6 +35,14 @@ class FileHandle;
   */
 #define AT_HANDLER_MUTEX
 
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
+#include "ConditionVariable.h"
+#endif
+
+namespace mbed {
+
+class FileHandle;
+
 extern const char *OK;
 extern const char *CRLF;
 
@@ -218,8 +221,9 @@ public:
 
 protected:
     void event();
-#ifdef AT_HANDLER_MUTEX
+#if defined AT_HANDLER_MUTEX && defined MBED_CONF_RTOS_PRESENT
-    PlatformMutex _fileHandleMutex;
+    rtos::Mutex _fileHandleMutex;
+    rtos::ConditionVariable _oobCv;
 #endif
     FileHandle *_fileHandle;
 private:
@@ -251,7 +255,6 @@ private:
     uint16_t _at_send_delay;
     uint64_t _last_response_stop;
 
-    bool _oob_queued;
     int32_t _ref_count;
     bool _is_fh_usable;
 
@@ -550,6 +553,8 @@ private:
 
     // time when a command or an URC processing was started
     uint64_t _start_time;
+    // eventqueue event id
+    int _event_id;
 
     char _cmd_buffer[BUFF_SIZE];
 

features/cellular/framework/AT/AT_CellularStack.h

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