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)

4 files changed, 85 insertions, 67 deletions

diff --git a/crypto/thread/arch.c b/crypto/thread/arch.c
index 565f87b93a..72fddf5f84 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 8a0389f5cb..1da736a7fb 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 d74cfddab3..0504ac9f81 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 b71cda85ea..7b63712d5b 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)