From fc3e674615f7643f5a6b08e92243a6add3c1c472 Mon Sep 17 00:00:00 2001 From: Dan Mihai Date: Wed, 1 Mar 2017 18:14:57 -0800 Subject: [PATCH] [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 Reviewed-on: https://gerrit.iotivity.org/gerrit/17605 Reviewed-by: Kevin Kane Tested-by: jenkins-iotivity Reviewed-by: Greg Zaverucha --- resource/c_common/octhread/include/octhread.h | 17 +++++++ resource/c_common/octhread/src/posix/octhread.c | 55 ++++++++++++++++++++++ resource/c_common/octhread/src/windows/octhread.c | 55 ++++++++++++++++++++++ .../src/adapter_util/ca_adapter_net_ssl.c | 8 ++-- 4 files changed, 132 insertions(+), 3 deletions(-) diff --git a/resource/c_common/octhread/include/octhread.h b/resource/c_common/octhread/include/octhread.h index c9a6062..713b975 100644 --- 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 --- 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 --- 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 --- 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; } -- 2.7.4