From 5b165864836e2eac44922a574e05c5b971f401bb Mon Sep 17 00:00:00 2001 From: David Garske Date: Tue, 8 Aug 2023 11:59:43 -0700 Subject: [PATCH] Fixes for wolfSSL conditional porting. Can cause deadlock in high usage situations. Added better signal support on MacOS. Issue created in PR #6437. --- examples/benchmark/tls_bench.c | 9 ++-- examples/echoserver/echoserver.c | 5 -- src/crl.c | 24 +++------ tests/utils.c | 26 ++++------ wolfcrypt/src/wc_port.c | 84 +++++++++++++++++++++++++------- wolfssl/test.h | 54 +++++--------------- wolfssl/wolfcrypt/types.h | 20 +++++--- 7 files changed, 112 insertions(+), 110 deletions(-) diff --git a/examples/benchmark/tls_bench.c b/examples/benchmark/tls_bench.c index 2d114e8d8..0417f1b18 100644 --- a/examples/benchmark/tls_bench.c +++ b/examples/benchmark/tls_bench.c @@ -436,8 +436,7 @@ static int ServerMemRecv(info_t* info, char* buf, int sz) #ifndef BENCH_USE_NONBLOCK while (info->to_server.write_idx - info->to_server.read_idx < sz && !info->to_client.done) { - THREAD_CHECK_RET(wolfSSL_CondWait(&info->to_server.cond, - &info->to_server.mutex)); + THREAD_CHECK_RET(wolfSSL_CondWait(&info->to_server.cond)); } #else if (info->to_server.write_idx - info->to_server.read_idx < sz) { @@ -511,8 +510,7 @@ static int ClientMemRecv(info_t* info, char* buf, int sz) #ifndef BENCH_USE_NONBLOCK while (info->to_client.write_idx - info->to_client.read_idx < sz && !info->to_server.done) { - THREAD_CHECK_RET(wolfSSL_CondWait(&info->to_client.cond, - &info->to_client.mutex)); + THREAD_CHECK_RET(wolfSSL_CondWait(&info->to_client.cond)); } #else if (info->to_client.write_idx - info->to_client.read_idx < sz) { @@ -1054,8 +1052,7 @@ static int bench_tls_client(info_t* info) if (info->doDTLS && !info->clientOrserverOnly) { THREAD_CHECK_RET(wc_LockMutex(&info->dtls_mutex)); if (info->serverReady != 1) { - THREAD_CHECK_RET(wolfSSL_CondWait(&info->dtls_cond, - &info->dtls_mutex)); + THREAD_CHECK_RET(wolfSSL_CondWait(&info->dtls_cond)); } /* for next loop */ info->serverReady = 0; diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index a392fe18b..ae5e8dbbc 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -74,13 +74,8 @@ static void SignalReady(void* args, word16 port) THREAD_CHECK_RET(wc_LockMutex(&ready->mutex)); ready->ready = 1; ready->port = port; -#ifdef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif THREAD_CHECK_RET(wolfSSL_CondSignal(&ready->cond)); -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif #endif /* NO_MAIN_DRIVER && WOLFSSL_COND */ (void)args; (void)port; diff --git a/src/crl.c b/src/crl.c index b167914c0..540f785f8 100644 --- a/src/crl.c +++ b/src/crl.c @@ -931,20 +931,14 @@ static int SignalSetup(WOLFSSL_CRL* crl, int status) int ret; /* signal to calling thread we're setup */ -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX if (wc_LockMutex(&crl->crlLock) != 0) { WOLFSSL_MSG("wc_LockMutex crlLock failed"); return BAD_MUTEX_E; } -#endif - crl->setup = status; - ret = wolfSSL_CondSignal(&crl->cond); - -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX wc_UnLockMutex(&crl->crlLock); -#endif + ret = wolfSSL_CondSignal(&crl->cond); if (ret != 0) return BAD_COND_E; @@ -1491,26 +1485,24 @@ static int StartMonitorCRL(WOLFSSL_CRL* crl) return THREAD_CREATE_E; } -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX /* wait for setup to complete */ if (wc_LockMutex(&crl->crlLock) != 0) { - WOLFSSL_MSG("wc_LockMutex crlLock error"); + WOLFSSL_MSG("wc_LockMutex crlLock failed"); return BAD_MUTEX_E; } -#endif - while (crl->setup == 0) { - if (wolfSSL_CondWait(&crl->cond, &crl->crlLock) != 0) { + int condRet; + wc_UnLockMutex(&crl->crlLock); + condRet = wolfSSL_CondWait(&crl->cond); + wc_LockMutex(&crl->crlLock); + if (condRet != 0) { ret = BAD_COND_E; break; } } - if (crl->setup < 0) + if (ret >= 0 && crl->setup < 0) ret = crl->setup; /* store setup error */ - -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX wc_UnLockMutex(&crl->crlLock); -#endif if (ret < 0) { WOLFSSL_MSG("DoMonitor setup failure"); diff --git a/tests/utils.c b/tests/utils.c index 368c7f44a..14480855c 100644 --- a/tests/utils.c +++ b/tests/utils.c @@ -322,31 +322,23 @@ void signal_ready(tcp_ready* ready) { THREAD_CHECK_RET(wc_LockMutex(&ready->mutex)); ready->ready = 1; -#ifdef COND_NO_REQUIRE_LOCKED_MUTEX THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif THREAD_CHECK_RET(wolfSSL_CondSignal(&ready->cond)); -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif } #endif void wait_tcp_ready(func_args* args) { #if !defined(SINGLE_THREADED) && defined(WOLFSSL_COND) - if (!args->signal->ready) { -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_LockMutex(&args->signal->mutex)); -#endif - THREAD_CHECK_RET(wolfSSL_CondWait(&args->signal->cond, - &args->signal->mutex)); -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_UnLockMutex(&args->signal->mutex)); -#endif + tcp_ready* ready = args->signal; + THREAD_CHECK_RET(wc_LockMutex(&ready->mutex)); + if (!ready->ready) { + THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); + THREAD_CHECK_RET(wolfSSL_CondWait(&ready->cond)); + THREAD_CHECK_RET(wc_LockMutex(&ready->mutex)); } - args->signal->ready = 0; /* reset */ - + ready->ready = 0; /* reset */ + THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); #else /* no threading wait or single threaded */ (void)args; @@ -356,7 +348,7 @@ void wait_tcp_ready(func_args* args) #ifndef SINGLE_THREADED /* Start a thread. * - * @param [in] fun Function to executre in thread. + * @param [in] fun Function to execute in thread. * @param [in] args Object to send to function in thread. * @param [out] thread Handle to thread. */ diff --git a/wolfcrypt/src/wc_port.c b/wolfcrypt/src/wc_port.c index 61104b99a..c2169ec09 100644 --- a/wolfcrypt/src/wc_port.c +++ b/wolfcrypt/src/wc_port.c @@ -3491,11 +3491,8 @@ char* mystrnstr(const char* s1, const char* s2, unsigned int n) return 0; } - int wolfSSL_CondWait(COND_TYPE* cond, - wolfSSL_Mutex* mutex) + int wolfSSL_CondWait(COND_TYPE* cond) { - (void)mutex; - if (cond == NULL) return BAD_FUNC_ARG; @@ -3698,14 +3695,20 @@ char* mystrnstr(const char* s1, const char* s2, unsigned int n) } #ifdef WOLFSSL_COND + #ifndef __MACH__ + /* Generic POSIX conditional */ int wolfSSL_CondInit(COND_TYPE* cond) { if (cond == NULL) return BAD_FUNC_ARG; - if (pthread_cond_init(cond, NULL) != 0) + cond->lockCount = 1; /* behave as signal, by defaulting to locked */ + if (pthread_mutex_init(&cond->mutex, NULL) != 0) return MEMORY_E; - + if (pthread_cond_init(&cond->cond, NULL) != 0) { + pthread_mutex_destroy(&cond->mutex); + return MEMORY_E; + } return 0; } @@ -3714,9 +3717,8 @@ char* mystrnstr(const char* s1, const char* s2, unsigned int n) if (cond == NULL) return BAD_FUNC_ARG; - if (pthread_cond_destroy(cond) != 0) - return MEMORY_E; - + pthread_mutex_destroy(&cond->mutex); + pthread_cond_destroy(&cond->cond); return 0; } @@ -3725,26 +3727,72 @@ char* mystrnstr(const char* s1, const char* s2, unsigned int n) if (cond == NULL) return BAD_FUNC_ARG; - if (pthread_cond_signal(cond) != 0) - return MEMORY_E; - + pthread_mutex_lock(&cond->mutex); + if (cond->lockCount > 0) + cond->lockCount--; + pthread_cond_signal(&cond->cond); + pthread_mutex_unlock(&cond->mutex); return 0; } - int wolfSSL_CondWait(COND_TYPE* cond, - wolfSSL_Mutex* mutex) + int wolfSSL_CondWait(COND_TYPE* cond) { - if (cond == NULL || mutex == NULL) + if (cond == NULL) return BAD_FUNC_ARG; - /* mutex has to be locked on entry so we can't touch */ + pthread_mutex_lock(&cond->mutex); + while (cond->lockCount > 0) + pthread_cond_wait(&cond->cond, &cond->mutex); + cond->lockCount++; + pthread_mutex_unlock(&cond->mutex); + return 0; + } + #else + /* Apple style dispatch semaphore */ + int wolfSSL_CondInit(COND_TYPE* cond) + { + if (cond == NULL) + return BAD_FUNC_ARG; - if (pthread_cond_wait(cond, mutex) != 0) + /* dispatch_release() fails hard, with Trace/BPT trap signal, if the + * sem's internal count is less than the value passed in with + * dispatch_semaphore_create(). work around this by initing + * with 0, then incrementing it afterwards. + */ + cond->sem = dispatch_semaphore_create(0); + if (cond->sem == NULL) return MEMORY_E; - return 0; } + int wolfSSL_CondFree(COND_TYPE* cond) + { + if (cond == NULL) + return BAD_FUNC_ARG; + + dispatch_release(cond->sem); + cond->sem = NULL; + return 0; + } + + int wolfSSL_CondSignal(COND_TYPE* cond) + { + if (cond == NULL) + return BAD_FUNC_ARG; + + dispatch_semaphore_signal(cond->sem); + return 0; + } + + int wolfSSL_CondWait(COND_TYPE* cond) + { + if (cond == NULL) + return BAD_FUNC_ARG; + + dispatch_semaphore_wait(cond->sem, DISPATCH_TIME_FOREVER); + return 0; + } + #endif /* __MACH__ */ #endif /* WOLFSSL_COND */ #endif diff --git a/wolfssl/test.h b/wolfssl/test.h index fb82a7034..f19cfbd3f 100644 --- a/wolfssl/test.h +++ b/wolfssl/test.h @@ -551,12 +551,10 @@ static WC_INLINE void InitTcpReady(tcp_ready* ready) ready->srfName = NULL; #ifndef SINGLE_THREADED -#ifdef WOLFSSL_COND THREAD_CHECK_RET(wc_InitMutex(&ready->mutex)); + #ifdef WOLFSSL_COND THREAD_CHECK_RET(wolfSSL_CondInit(&ready->cond)); -#else /* No signaling available, rely only on the mutex */ - THREAD_CHECK_RET(wc_InitMutex(&ready->mutex)); -#endif + #endif #endif } @@ -567,11 +565,9 @@ static WC_INLINE void InitTcpReady(tcp_ready* ready) static WC_INLINE void FreeTcpReady(tcp_ready* ready) { #ifndef SINGLE_THREADED + THREAD_CHECK_RET(wc_FreeMutex(&ready->mutex)); #ifdef WOLFSSL_COND - THREAD_CHECK_RET(wc_FreeMutex(&ready->mutex)); THREAD_CHECK_RET(wolfSSL_CondFree(&ready->cond)); -#else /* No signaling available, rely only on the mutex */ - THREAD_CHECK_RET(wc_FreeMutex(&ready->mutex)); #endif #else (void)ready; @@ -695,24 +691,16 @@ static WC_INLINE void srtp_helper_init(srtp_test_helper *srtp) static WC_INLINE void srtp_helper_get_ekm(srtp_test_helper *srtp, uint8_t **ekm, size_t *size) { -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_LockMutex(&srtp->mutex)); -#endif - if (srtp->server_srtp_ekm == NULL) THREAD_CHECK_RET(wolfSSL_CondWait(&srtp->cond, &srtp->mutex)); -#ifdef COND_NO_REQUIRE_LOCKED_MUTEX THREAD_CHECK_RET(wc_LockMutex(&srtp->mutex)); -#endif - *ekm = srtp->server_srtp_ekm; *size = srtp->server_srtp_ekm_size; /* reset */ srtp->server_srtp_ekm = NULL; srtp->server_srtp_ekm_size = 0; - THREAD_CHECK_RET(wc_UnLockMutex(&srtp->mutex)); } @@ -731,17 +719,10 @@ static WC_INLINE void srtp_helper_set_ekm(srtp_test_helper *srtp, uint8_t *ekm, size_t size) { THREAD_CHECK_RET(wc_LockMutex(&srtp->mutex)); - srtp->server_srtp_ekm_size = size; srtp->server_srtp_ekm = ekm; -#ifdef COND_NO_REQUIRE_LOCKED_MUTEX THREAD_CHECK_RET(wc_UnLockMutex(&srtp->mutex)); -#endif THREAD_CHECK_RET(wolfSSL_CondSignal(&srtp->cond)); - -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_UnLockMutex(&srtp->mutex)); -#endif } static WC_INLINE void srtp_helper_free(srtp_test_helper *srtp) @@ -750,7 +731,8 @@ static WC_INLINE void srtp_helper_free(srtp_test_helper *srtp) THREAD_CHECK_RET(wolfSSL_CondFree(&srtp->cond)); } -#endif /* WOLFSSL_SRTP && !SINGLE_THREADED && POSIX_THREADS */ +#endif /* WOLFSSL_SRTP && WOLFSSL_COND */ + /** * @@ -2237,19 +2219,12 @@ static WC_INLINE void udp_accept(SOCKET_T* sockfd, SOCKET_T* clientfd, THREAD_CHECK_RET(wc_LockMutex(&ready->mutex)); ready->ready = 1; ready->port = port; -#ifdef WOLFSSL_COND + THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); + #ifdef WOLFSSL_COND /* signal ready to accept data */ -#ifdef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif THREAD_CHECK_RET(wolfSSL_CondSignal(&ready->cond)); -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif -#else - THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif -#endif + #endif +#endif /* !SINGLE_THREADED */ } else { fprintf(stderr, "args or args->signal was NULL. Not setting ready info."); @@ -2283,17 +2258,12 @@ static WC_INLINE void tcp_accept(SOCKET_T* sockfd, SOCKET_T* clientfd, THREAD_CHECK_RET(wc_LockMutex(&ready->mutex)); ready->ready = 1; ready->port = port; -#ifdef WOLFSSL_COND -#ifdef COND_NO_REQUIRE_LOCKED_MUTEX THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif + #ifdef WOLFSSL_COND THREAD_CHECK_RET(wolfSSL_CondSignal(&ready->cond)); -#ifndef COND_NO_REQUIRE_LOCKED_MUTEX - THREAD_CHECK_RET(wc_UnLockMutex(&ready->mutex)); -#endif -#endif + #endif } -#endif +#endif /* !SINGLE_THREADED */ if (ready_file) { #if !defined(NO_FILESYSTEM) || defined(FORCE_BUFFER_TEST) && \ diff --git a/wolfssl/wolfcrypt/types.h b/wolfssl/wolfcrypt/types.h index 93413f3da..a46970ecf 100644 --- a/wolfssl/wolfcrypt/types.h +++ b/wolfssl/wolfcrypt/types.h @@ -1380,9 +1380,21 @@ typedef struct w64wrapper { #define WOLFSSL_THREAD #elif (defined(_POSIX_THREADS) || defined(HAVE_PTHREAD)) && \ !defined(__MINGW32__) + #ifdef __MACH__ + #include + typedef struct COND_TYPE { + dispatch_semaphore_t sem; + } COND_TYPE; + #else + #include + typedef struct COND_TYPE { + volatile int lockCount; + pthread_mutex_t mutex; + pthread_cond_t cond; + } COND_TYPE; + #endif typedef void* THREAD_RETURN; typedef pthread_t THREAD_TYPE; - typedef pthread_cond_t COND_TYPE; #define WOLFSSL_COND #define WOLFSSL_THREAD #ifndef HAVE_SELFTEST @@ -1398,7 +1410,6 @@ typedef struct w64wrapper { typedef HANDLE COND_TYPE; #define WOLFSSL_COND #define INVALID_THREAD_VAL ((THREAD_TYPE)(INVALID_HANDLE_VALUE)) - #define COND_NO_REQUIRE_LOCKED_MUTEX #define WOLFSSL_THREAD __stdcall #define WOLFSSL_THREAD_NO_JOIN __cdecl #else @@ -1433,8 +1444,6 @@ typedef struct w64wrapper { * thread callbacks that don't require cleanup * WOLFSSL_COND - defined if this system suports signaling * COND_TYPE - type that should be passed into the signaling API - * COND_NO_REQUIRE_LOCKED_MUTEX - defined if the signaling doesn't - * require locking a mutex * WOLFSSL_THREAD_VOID_RETURN - defined if the thread callback has a * void return * WOLFSSL_RETURN_FROM_THREAD - define used to correctly return from a @@ -1475,8 +1484,7 @@ typedef struct w64wrapper { WOLFSSL_API int wolfSSL_CondInit(COND_TYPE* cond); WOLFSSL_API int wolfSSL_CondFree(COND_TYPE* cond); WOLFSSL_API int wolfSSL_CondSignal(COND_TYPE* cond); - WOLFSSL_API int wolfSSL_CondWait(COND_TYPE* cond, - wolfSSL_Mutex* mutex); + WOLFSSL_API int wolfSSL_CondWait(COND_TYPE* cond); #endif #else #define WOLFSSL_RETURN_FROM_THREAD(x) return (THREAD_RETURN)(x)