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.

Author: Teppo Järvelin

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/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,8 @@ set(unittest-test-sources
   stubs/ThisThread_stub.cpp
   stubs/randLIB_stub.cpp
   stubs/CellularUtil_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

@@ -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/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),
+#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 "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),
+#ifdef AT_HANDLER_MUTEX
+    _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(-1)
 {
     clear_error();
 
@@ -218,8 +222,17 @@ bool ATHandler::get_debug() const
 
 ATHandler::~ATHandler()
 {
+    ScopedLock <ATHandler> lock(*this);
     set_file_handle(NULL);
 
+    while (_event_id > 0) {
+#ifdef AT_HANDLER_MUTEX
+        _oobCv.wait();
+#else
+        ThisThread::sleep_for(PROCESS_URC_TIME);
+#endif // AT_HANDLER_MUTEX
+    }
+
     while (_oobs) {
         struct oob_t *oob = _oobs;
         _oobs = oob->next;
@@ -252,6 +265,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 +277,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,9 +352,8 @@ bool ATHandler::find_urc_handler(const char *prefix)
 
 void ATHandler::event()
 {
-    if (!_oob_queued) {
-        _oob_queued = true;
-        (void) _queue.call(Callback<void(void)>(this, &ATHandler::process_oob));
+    if (_event_id <= 0) {
+        _event_id = _queue.call(Callback<void(void)>(this, &ATHandler::process_oob));
     }
 }
 
@@ -354,12 +368,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()
@@ -393,12 +407,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 = -1;
+#ifdef AT_HANDLER_MUTEX
+        _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 +441,10 @@ void ATHandler::process_oob()
         _at_timeout = timeout;
         tr_debug("AT OoB done");
     }
-    unlock();
+    _event_id = -1;
+#ifdef AT_HANDLER_MUTEX
+    _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
 
+#ifdef AT_HANDLER_MUTEX
+#include "ConditionVariable.h"
+#endif
+
+namespace mbed {
+
+class FileHandle;
+
 extern const char *OK;
 extern const char *CRLF;
 
@@ -219,7 +222,8 @@ class ATHandler {
 protected:
     void event();
 #ifdef AT_HANDLER_MUTEX
-    PlatformMutex _fileHandleMutex;
+    rtos::Mutex _fileHandleMutex;
+    rtos::ConditionVariable _oobCv;
 #endif
     FileHandle *_fileHandle;
 private:
@@ -251,7 +255,6 @@ class ATHandler {
     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 @@ class ATHandler {
 
     // 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 {