From 4e43bc06f7673597a99f61325543449e72070c8c Mon Sep 17 00:00:00 2001 From: =?utf8?q?=C4=8Cestm=C3=ADr=20Kalina?= Date: Tue, 18 Oct 2022 08:41:21 -0400 Subject: [PATCH] crypto: thread: serialize concurrent joins MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Multiple concurrent joins with a running thread suffer from a race condition that allows concurrent join calls to perform concurrent arch specific join calls, which is UB on POSIX, or to concurrently execute join and terminate calls. As soon as a thread T1 exists, one of the threads that joins with T1 is selected to perform the join, the remaining ones await completion. Once completed, the remaining calls immediately return. If the join failed, another thread is selected to attempt the join operation. Forcefully terminating a thread that is in the process of joining another thread is not supported. Common code from thread_posix and thread_win was refactored to use common wrapper that handles synchronization. Signed-off-by: Čestmír Kalina Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/19433) --- crypto/thread/arch.c | 66 +++++++++++++++++++++++++++++++ crypto/thread/arch/thread_none.c | 2 +- crypto/thread/arch/thread_posix.c | 48 ++++++---------------- crypto/thread/arch/thread_win.c | 36 +++-------------- include/internal/thread_arch.h | 5 ++- test/threadstest.c | 2 +- 6 files changed, 90 insertions(+), 69 deletions(-) diff --git a/crypto/thread/arch.c b/crypto/thread/arch.c index 565f87b93a2..72fddf5f84d 100644 --- a/crypto/thread/arch.c +++ b/crypto/thread/arch.c @@ -46,6 +46,72 @@ fail: return NULL; } +int ossl_crypto_thread_native_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval) +{ + uint64_t req_state_mask; + + if (thread == NULL) + return 0; + + ossl_crypto_mutex_lock(thread->statelock); + req_state_mask = CRYPTO_THREAD_TERMINATED | CRYPTO_THREAD_FINISHED \ + | CRYPTO_THREAD_JOINED; + while (!CRYPTO_THREAD_GET_STATE(thread, req_state_mask)) + ossl_crypto_condvar_wait(thread->condvar, thread->statelock); + + if (CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_TERMINATED)) { + ossl_crypto_mutex_unlock(thread->statelock); + return 0; + } + + if (CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_JOINED)) + goto pass; + + /* Await concurrent join completion, if any. */ + while (CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_JOIN_AWAIT)) { + if (!CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_JOINED)) + ossl_crypto_condvar_wait(thread->condvar, thread->statelock); + if (CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_JOINED)) + goto pass; + } + CRYPTO_THREAD_SET_STATE(thread, CRYPTO_THREAD_JOIN_AWAIT); + ossl_crypto_mutex_unlock(thread->statelock); + + if (ossl_crypto_thread_native_perform_join(thread, retval) == 0) + goto fail; + + ossl_crypto_mutex_lock(thread->statelock); +pass: + CRYPTO_THREAD_UNSET_ERROR(thread, CRYPTO_THREAD_JOINED); + CRYPTO_THREAD_SET_STATE(thread, CRYPTO_THREAD_JOINED); + + /* + * Broadcast join completion. It is important to broadcast even if + * we haven't performed an actual join. Multiple threads could be + * awaiting the CRYPTO_THREAD_JOIN_AWAIT -> CRYPTO_THREAD_JOINED + * transition, but broadcast on actual join would wake only one. + * Broadcasing here will always wake one. + */ + ossl_crypto_condvar_broadcast(thread->condvar); + ossl_crypto_mutex_unlock(thread->statelock); + + if (retval != NULL) + *retval = thread->retval; + return 1; + +fail: + ossl_crypto_mutex_lock(thread->statelock); + CRYPTO_THREAD_SET_ERROR(thread, CRYPTO_THREAD_JOINED); + + /* Have another thread that's awaiting join retry to avoid that + * thread deadlock. */ + CRYPTO_THREAD_UNSET_STATE(thread, CRYPTO_THREAD_JOIN_AWAIT); + ossl_crypto_condvar_broadcast(thread->condvar); + + ossl_crypto_mutex_unlock(thread->statelock); + return 0; +} + int ossl_crypto_thread_native_clean(CRYPTO_THREAD *handle) { uint64_t req_state_mask; diff --git a/crypto/thread/arch/thread_none.c b/crypto/thread/arch/thread_none.c index 8a0389f5cbf..1da736a7fb6 100644 --- a/crypto/thread/arch/thread_none.c +++ b/crypto/thread/arch/thread_none.c @@ -16,7 +16,7 @@ int ossl_crypto_thread_native_spawn(CRYPTO_THREAD *thread) return 0; } -int ossl_crypto_thread_native_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval) +int ossl_crypto_thread_native_perform_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval) { return 0; } diff --git a/crypto/thread/arch/thread_posix.c b/crypto/thread/arch/thread_posix.c index d74cfddab37..0504ac9f81f 100644 --- a/crypto/thread/arch/thread_posix.c +++ b/crypto/thread/arch/thread_posix.c @@ -63,55 +63,26 @@ fail: return 0; } -int ossl_crypto_thread_native_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval) +int ossl_crypto_thread_native_perform_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval) { void *thread_retval; pthread_t *handle; - uint64_t req_state_mask; - if (thread == NULL) + if (thread == NULL || thread->handle == NULL) return 0; - req_state_mask = CRYPTO_THREAD_TERMINATED | CRYPTO_THREAD_JOINED; - - ossl_crypto_mutex_lock(thread->statelock); - if (CRYPTO_THREAD_GET_STATE(thread, req_state_mask)) { - ossl_crypto_mutex_unlock(thread->statelock); - goto pass; - } - while (!CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_FINISHED)) - ossl_crypto_condvar_wait(thread->condvar, thread->statelock); - ossl_crypto_mutex_unlock(thread->statelock); - handle = (pthread_t *) thread->handle; - if (handle == NULL) - goto fail; - if (pthread_join(*handle, &thread_retval) != 0) - goto fail; + return 0; /* * Join return value may be non-NULL when the thread has been cancelled, * as indicated by thread_retval set to PTHREAD_CANCELLED. */ if (thread_retval != NULL) - goto fail; - -pass: - if (retval != NULL) - *retval = thread->retval; + return 0; - ossl_crypto_mutex_lock(thread->statelock); - CRYPTO_THREAD_UNSET_ERROR(thread, CRYPTO_THREAD_JOINED); - CRYPTO_THREAD_SET_STATE(thread, CRYPTO_THREAD_JOINED); - ossl_crypto_mutex_unlock(thread->statelock); return 1; - -fail: - ossl_crypto_mutex_lock(thread->statelock); - CRYPTO_THREAD_SET_ERROR(thread, CRYPTO_THREAD_JOINED); - ossl_crypto_mutex_unlock(thread->statelock); - return 0; } int ossl_crypto_thread_native_terminate(CRYPTO_THREAD *thread) @@ -130,14 +101,15 @@ int ossl_crypto_thread_native_terminate(CRYPTO_THREAD *thread) ossl_crypto_mutex_lock(thread->statelock); if (thread->handle == NULL || CRYPTO_THREAD_GET_STATE(thread, mask)) goto terminated; + /* Do not fail when there's a join in progress. Do not block. */ + if (CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_JOIN_AWAIT)) + goto fail; ossl_crypto_mutex_unlock(thread->statelock); handle = thread->handle; if (pthread_cancel(*handle) != 0) { ossl_crypto_mutex_lock(thread->statelock); - CRYPTO_THREAD_SET_ERROR(thread, CRYPTO_THREAD_TERMINATED); - ossl_crypto_mutex_unlock(thread->statelock); - return 0; + goto fail; } if (pthread_join(*handle, &res) != 0) return 0; @@ -153,6 +125,10 @@ terminated: CRYPTO_THREAD_SET_STATE(thread, CRYPTO_THREAD_TERMINATED); ossl_crypto_mutex_unlock(thread->statelock); return 1; +fail: + CRYPTO_THREAD_SET_ERROR(thread, CRYPTO_THREAD_TERMINATED); + ossl_crypto_mutex_unlock(thread->statelock); + return 0; } int ossl_crypto_thread_native_exit(void) diff --git a/crypto/thread/arch/thread_win.c b/crypto/thread/arch/thread_win.c index b71cda85ea8..7b63712d5b4 100644 --- a/crypto/thread/arch/thread_win.c +++ b/crypto/thread/arch/thread_win.c @@ -53,32 +53,20 @@ fail: return 0; } -int ossl_crypto_thread_native_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval) +int ossl_crypto_thread_native_perform_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval) { - int req_state_mask; DWORD thread_retval; HANDLE *handle; - if (thread == NULL) + if (thread == NULL || thread->handle == NULL) return 0; - req_state_mask = CRYPTO_THREAD_TERMINATED | CRYPTO_THREAD_JOINED; - - ossl_crypto_mutex_lock(thread->statelock); - if (CRYPTO_THREAD_GET_STATE(thread, req_state_mask)) - goto pass; - while (!CRYPTO_THREAD_GET_STATE(thread, CRYPTO_THREAD_FINISHED)) - ossl_crypto_condvar_wait(thread->condvar, thread->statelock); - handle = (HANDLE *) thread->handle; - if (handle == NULL) - goto fail; - if (WaitForSingleObject(*handle, INFINITE) != WAIT_OBJECT_0) - goto fail; + return 0; if (GetExitCodeThread(*handle, &thread_retval) == 0) - goto fail; + return 0; /* * GetExitCodeThread call followed by this check is to make sure that @@ -87,24 +75,12 @@ int ossl_crypto_thread_native_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL * * if the thread is still active (returns STILL_ACTIVE (259)). */ if (thread_retval != 0) - goto fail; + return 0; if (CloseHandle(*handle) == 0) - goto fail; - -pass: - if (retval != NULL) - *retval = thread->retval; + return 0; - CRYPTO_THREAD_UNSET_ERROR(thread, CRYPTO_THREAD_JOINED); - CRYPTO_THREAD_SET_STATE(thread, CRYPTO_THREAD_JOINED); - ossl_crypto_mutex_unlock(thread->statelock); return 1; - -fail: - CRYPTO_THREAD_SET_ERROR(thread, CRYPTO_THREAD_JOINED); - ossl_crypto_mutex_unlock(thread->statelock); - return 0; } int ossl_crypto_thread_native_terminate(CRYPTO_THREAD *thread) diff --git a/include/internal/thread_arch.h b/include/internal/thread_arch.h index fcf312ff144..142e77b7683 100644 --- a/include/internal/thread_arch.h +++ b/include/internal/thread_arch.h @@ -52,7 +52,8 @@ typedef CRYPTO_THREAD_RETVAL (*CRYPTO_THREAD_ROUTINE_CB)(void *, void **); # define CRYPTO_THREAD_NO_STATE 0UL -# define CRYPTO_THREAD_FINISHED (1UL << 1) +# define CRYPTO_THREAD_FINISHED (1UL << 0) +# define CRYPTO_THREAD_JOIN_AWAIT (1UL << 1) # define CRYPTO_THREAD_JOINED (1UL << 2) # define CRYPTO_THREAD_TERMINATED (1UL << 3) @@ -109,6 +110,8 @@ CRYPTO_THREAD * ossl_crypto_thread_native_start(CRYPTO_THREAD_ROUTINE routine, int ossl_crypto_thread_native_spawn(CRYPTO_THREAD *thread); int ossl_crypto_thread_native_join(CRYPTO_THREAD *thread, CRYPTO_THREAD_RETVAL *retval); +int ossl_crypto_thread_native_perform_join(CRYPTO_THREAD *thread, + CRYPTO_THREAD_RETVAL *retval); int ossl_crypto_thread_native_terminate(CRYPTO_THREAD *thread); int ossl_crypto_thread_native_exit(void); int ossl_crypto_thread_native_is_self(CRYPTO_THREAD *thread); diff --git a/test/threadstest.c b/test/threadstest.c index af1d78a3b7e..6ddb4bf96c1 100644 --- a/test/threadstest.c +++ b/test/threadstest.c @@ -833,7 +833,7 @@ static int test_thread_native(void) if (!TEST_int_eq(ossl_crypto_thread_native_terminate(t), 1)) return 0; - if (!TEST_int_eq(ossl_crypto_thread_native_join(t, &retval), 1)) + if (!TEST_int_eq(ossl_crypto_thread_native_join(t, &retval), 0)) return 0; if (!TEST_int_eq(ossl_crypto_thread_native_clean(t), 1)) -- 2.47.3