crypto: thread: serialize concurrent joins

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 <ckalina@redhat.com>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19433)

diff --git a/crypto/thread/arch.c b/crypto/thread/arch.c
index 565f87b93a2bd15a08bc223d6a1e858021cd3abd..72fddf5f84d03362a00fdbf850462cbf9b15e8d3 100644 (file)
--- 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 8a0389f5cbfd5802fda8d4e564a11578dc1195a0..1da736a7fb679b8bbe3b0fdaf5b1dcbbaccff921 100644 (file)
--- 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 d74cfddab3792ef05993635abdb8fe43595d3278..0504ac9f81ff1de5995b60e430bc4f0eb78c0051 100644 (file)
--- 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 b71cda85ea8ddf0ae1c75a21f0e007f38e4e3e79..7b63712d5b4c45789fe26d7ca8a680c5822a80aa 100644 (file)
--- 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 fcf312ff144fe40cc6c8ea0466106490ea793d2f..142e77b76833d4c95f2779e687db0abbbe3cd647 100644 (file)
--- 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 af1d78a3b7e2e124070fd7f7f917944f3dbfe4ab..6ddb4bf96c1512965c210c80877eb8ab4d8918c0 100644 (file)
--- 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))