[IOT-1861] Fix self-deadlock during OT

Avoid acquiring g_sslContextMutex recursively during Ownership
Transfer. Instead, make it a requirement that the caller must
own this lock.

Finding a better solution for this old problem is now tracked
by IOT-1876.

Change-Id: I25ce19a5c48c5adab70e2287288833e40c4db213
Signed-off-by: Dan Mihai <Daniel.Mihai@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/17605
Reviewed-by: Kevin Kane <kkane@microsoft.com>
Tested-by: jenkins-iotivity <jenkins@iotivity.org>
Reviewed-by: Greg Zaverucha <gregz@microsoft.com>

diff --git a/resource/c_common/octhread/include/octhread.h b/resource/c_common/octhread/include/octhread.h
index c9a6062..713b975 100644 (file)
--- a/resource/c_common/octhread/include/octhread.h
+++ b/resource/c_common/octhread/include/octhread.h
@@ -37,6 +37,11 @@ extern "C"
 {
 #endif /* __cplusplus */
 
+/**
+ * Value used for the owner field of an oc_mutex that doesn't have an owner.
+ */
+#define OC_INVALID_THREAD_ID    0
+
 typedef struct oc_mutex_internal *oc_mutex;
 typedef struct oc_cond_internal *oc_cond;
 typedef struct oc_thread_internal *oc_thread;
@@ -133,6 +138,18 @@ void oc_mutex_unlock(oc_mutex mutex);
 bool oc_mutex_free(oc_mutex mutex);
 
 /**
+ * On Debug builds, assert that the current thread owns or does not own a mutex.
+ *
+ * This function has no effect on Release builds.
+ *
+ * @param[in] mutex  The mutex to assert on.
+ * @param[in] currentThreadIsOwner  true if the current thread is expected to
+ *                  be the mutex owner, false otherwise.
+ *
+ */
+void oc_mutex_assert_owner(const oc_mutex mutex, bool currentThreadIsOwner);
+
+/**
  * Creates new condition.
  *
  * @return  Reference to newly created oc_cond, otherwise NULL.

diff --git a/resource/c_common/octhread/src/posix/octhread.c b/resource/c_common/octhread/src/posix/octhread.c
index 0dacb0c..cd0282c 100644 (file)
--- a/resource/c_common/octhread/src/posix/octhread.c
+++ b/resource/c_common/octhread/src/posix/octhread.c
@@ -88,6 +88,14 @@ static const uint64_t NANOSECS_PER_SEC      = 1000000000L;
 typedef struct _tagMutexInfo_t
 {
     pthread_mutex_t mutex;
+
+    /**
+     * Catch some of the incorrect mutex usage, by tracking the mutex owner,
+     * on Debug builds.
+     */
+#ifndef NDEBUG
+    pthread_t owner;
+#endif
 } oc_mutex_internal;
 
 typedef struct _tagEventInfo_t
@@ -102,6 +110,13 @@ typedef struct _tagThreadInfo_t
     pthread_attr_t  threadattr;
 } oc_thread_internal;
 
+static pthread_t oc_get_current_thread_id()
+{
+    pthread_t id = pthread_self();
+    assert(OC_INVALID_THREAD_ID != id);
+    return id;
+}
+
 OCThreadResult_t oc_thread_new(oc_thread *t, void *(*start_routine)(void *), void *arg)
 {
     OCThreadResult_t res = OC_THREAD_SUCCESS;
@@ -170,6 +185,9 @@ oc_mutex oc_mutex_new(void)
         int ret=pthread_mutex_init(&(mutexInfo->mutex), PTHREAD_MUTEX_DEFAULT);
         if (0 == ret)
         {
+#ifndef NDEBUG
+            mutexInfo->owner = OC_INVALID_THREAD_ID;
+#endif
             retVal = (oc_mutex) mutexInfo;
         }
         else
@@ -222,6 +240,14 @@ void oc_mutex_lock(oc_mutex mutex)
             OIC_LOG_V(ERROR, TAG, "Pthread Mutex lock failed: %d", ret);
             exit(ret);
         }
+
+#ifndef NDEBUG
+        /**
+         * Updating the owner field must be performed while owning the lock,
+         * to solve race conditions with other threads using the same lock.
+         */
+        mutexInfo->owner = oc_get_current_thread_id();
+#endif
     }
     else
     {
@@ -234,6 +260,14 @@ void oc_mutex_unlock(oc_mutex mutex)
     oc_mutex_internal *mutexInfo = (oc_mutex_internal*) mutex;
     if (mutexInfo)
     {
+#ifndef NDEBUG
+        /**
+         * Updating the owner field must be performed while owning the lock,
+         * to solve race conditions with other threads using the same lock.
+         */
+        mutexInfo->owner = OC_INVALID_THREAD_ID;
+#endif
+
         int ret = pthread_mutex_unlock(&mutexInfo->mutex);
         if(ret != 0)
         {
@@ -248,6 +282,27 @@ void oc_mutex_unlock(oc_mutex mutex)
     }
 }
 
+void oc_mutex_assert_owner(const oc_mutex mutex, bool currentThreadIsOwner)
+{
+#ifdef NDEBUG
+    (void)mutex;
+    (void)currentThreadIsOwner;
+#else
+    assert(NULL != mutex);
+    const oc_mutex_internal *mutexInfo = (const oc_mutex_internal*) mutex;
+
+    pthread_t currentThreadID = oc_get_current_thread_id();
+    if (currentThreadIsOwner)
+    {
+        assert(pthread_equal(mutexInfo->owner, currentThreadID));
+    }
+    else
+    {
+        assert(!pthread_equal(mutexInfo->owner, currentThreadID));
+    }
+#endif
+}
+
 oc_cond oc_cond_new(void)
 {
     oc_cond retVal = NULL;

diff --git a/resource/c_common/octhread/src/windows/octhread.c b/resource/c_common/octhread/src/windows/octhread.c
index 3888a85..9cc5447 100644 (file)
--- a/resource/c_common/octhread/src/windows/octhread.c
+++ b/resource/c_common/octhread/src/windows/octhread.c
@@ -41,8 +41,23 @@ static const uint64_t USECS_PER_MSEC = 1000;
 typedef struct _tagMutexInfo_t
 {
     CRITICAL_SECTION mutex;
+
+    /**
+     * Catch some of the incorrect mutex usage, by tracking the mutex owner,
+     * on Debug builds.
+     */
+#ifndef NDEBUG
+    DWORD owner;
+#endif
 } oc_mutex_internal;
 
+static DWORD oc_get_current_thread_id()
+{
+    DWORD id = GetCurrentThreadId();
+    assert(OC_INVALID_THREAD_ID != id);
+    return id;
+}
+
 typedef struct _tagEventInfo_t
 {
     CONDITION_VARIABLE cond;
@@ -119,6 +134,9 @@ oc_mutex oc_mutex_new(void)
     oc_mutex_internal *mutexInfo = (oc_mutex_internal*) OICMalloc(sizeof(oc_mutex_internal));
     if (NULL != mutexInfo)
     {
+#ifndef NDEBUG
+        mutexInfo->owner = OC_INVALID_THREAD_ID;
+#endif
         InitializeCriticalSection(&mutexInfo->mutex);
         retVal = (oc_mutex)mutexInfo;
     }
@@ -154,6 +172,14 @@ void oc_mutex_lock(oc_mutex mutex)
     if (mutexInfo)
     {
         EnterCriticalSection(&mutexInfo->mutex);
+
+#ifndef NDEBUG
+        /**
+         * Updating the owner field must be performed while owning the lock,
+         * to solve race conditions with other threads using the same lock.
+         */
+        mutexInfo->owner = oc_get_current_thread_id();
+#endif
     }
     else
     {
@@ -166,6 +192,14 @@ void oc_mutex_unlock(oc_mutex mutex)
     oc_mutex_internal *mutexInfo = (oc_mutex_internal*) mutex;
     if (mutexInfo)
     {
+#ifndef NDEBUG
+        /**
+         * Updating the owner field must be performed while owning the lock,
+         * to solve race conditions with other threads using the same lock.
+         */
+        mutexInfo->owner = OC_INVALID_THREAD_ID;
+#endif
+
         LeaveCriticalSection(&mutexInfo->mutex);
     }
     else
@@ -174,6 +208,27 @@ void oc_mutex_unlock(oc_mutex mutex)
     }
 }
 
+void oc_mutex_assert_owner(const oc_mutex mutex, bool currentThreadIsOwner)
+{
+#ifdef NDEBUG
+    OC_UNUSED(mutex);
+    OC_UNUSED(currentThreadIsOwner);
+#else
+    assert(NULL != mutex);
+    const oc_mutex_internal *mutexInfo = (const oc_mutex_internal*) mutex;
+
+    DWORD currentThreadID = oc_get_current_thread_id();
+    if (currentThreadIsOwner)
+    {
+        assert(mutexInfo->owner == currentThreadID);
+    }
+    else
+    {
+        assert(mutexInfo->owner != currentThreadID);
+    }
+#endif
+}
+
 oc_cond oc_cond_new(void)
 {
     oc_cond retVal = NULL;

diff --git a/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c b/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c
index b92b210..ac375fb 100755 (executable)
--- a/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c
+++ b/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c
@@ -953,7 +953,11 @@ bool SetCASecureEndpointAttribute(const CAEndpoint_t* peer, uint32_t newAttribut
     OIC_LOG_V(DEBUG, NET_SSL_TAG, "In %s(peer = %s:%u, attribute = %#x)", __func__,
         peer->addr, (uint32_t)peer->port, newAttribute);
 
-    oc_mutex_lock(g_sslContextMutex);
+    // Acquiring g_sslContextMutex recursively here is not supported, so assert
+    // that the caller already owns this mutex. IOT-1876 tracks a possible
+    // refactoring of the code that is using g_sslContextMutex, to address these
+    // API quirks.
+    oc_mutex_assert_owner(g_sslContextMutex, true);
 
     if (NULL == g_caSslContext)
     {
@@ -974,8 +978,6 @@ bool SetCASecureEndpointAttribute(const CAEndpoint_t* peer, uint32_t newAttribut
         }
     }
 
-    oc_mutex_unlock(g_sslContextMutex);
-
     OIC_LOG_V(DEBUG, NET_SSL_TAG, "Out %s -> %s", __func__, result ? "success" : "failed");
     return result;
 }