Commit fc3e6746 authored by Dan Mihai's avatar Dan Mihai Committed by Greg Zaverucha

[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: default avatarDan Mihai <Daniel.Mihai@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/17605Reviewed-by: default avatarKevin Kane <kkane@microsoft.com>
Tested-by: default avatarjenkins-iotivity <jenkins@iotivity.org>
Reviewed-by: default avatarGreg Zaverucha <gregz@microsoft.com>
parent 941926f5
......@@ -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;
......@@ -132,6 +137,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.
*
......
......@@ -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;
......
......@@ -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;
......
......@@ -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;
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment