From d2096679d5b5d76a167d038a3a2aa570e4ce37f3 Mon Sep 17 00:00:00 2001
From: Anton Khirnov <anton@khirnov.net>
Date: Thu, 12 Dec 2024 16:04:44 +0100
Subject: [PATCH] compat/w32pthreads: change pthread_t into pointer to malloced
 struct
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

pthread_t is currently defined as a struct, which gets placed into
caller's memory and filled by pthread_create() (which accepts a
pthread_t*).

The problem with this approach is that pthread_join() accepts pthread_t
itself rather than a pointer to it, so it gets a _copy_ of this
structure. This causes non-deterministic failures of pthread_join() to
produce the correct return value - depending on whether the thread
already finished before pthread_join() is called (and thus the copy
contains the correct value), or not (then it contains 0).

Change the definition of pthread_t into a pointer to a struct, that gets
malloced by pthread_create() and freed by pthread_join().

Fixes random failures of fate-ffmpeg-error-rate-fail on Windows after
433cf391f58210432be907d817654929a66e80ba.

See also [1] for an alternative approach that does not require dynamic
allocation, but relies on an assumption that the pthread_t value
remains in a fixed memory location.

[1] https://code.videolan.org/videolan/x264/-/commit/23829dd2b2c909855481f46cc884b3c25d92c2d7

Reviewed-By: Martin Storsjö <martin@martin.st>
---
 compat/w32pthreads.h | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/compat/w32pthreads.h b/compat/w32pthreads.h
index 2ff9735227..fd6428e29f 100644
--- a/compat/w32pthreads.h
+++ b/compat/w32pthreads.h
@@ -50,7 +50,7 @@ typedef struct pthread_t {
     void *(*func)(void* arg);
     void *arg;
     void *ret;
-} pthread_t;
+} *pthread_t;
 
 /* use light weight mutex/condition variable API for Windows Vista and later */
 typedef SRWLOCK pthread_mutex_t;
@@ -74,7 +74,7 @@ typedef CONDITION_VARIABLE pthread_cond_t;
 static av_unused THREADFUNC_RETTYPE
 __stdcall attribute_align_arg win32thread_worker(void *arg)
 {
-    pthread_t *h = (pthread_t*)arg;
+    pthread_t h = (pthread_t)arg;
     h->ret = h->func(h->arg);
     return 0;
 }
@@ -82,21 +82,35 @@ __stdcall attribute_align_arg win32thread_worker(void *arg)
 static av_unused int pthread_create(pthread_t *thread, const void *unused_attr,
                                     void *(*start_routine)(void*), void *arg)
 {
-    thread->func   = start_routine;
-    thread->arg    = arg;
+    pthread_t ret;
+
+    ret = av_mallocz(sizeof(*ret));
+    if (!ret)
+        return EAGAIN;
+
+    ret->func   = start_routine;
+    ret->arg    = arg;
 #if HAVE_WINRT
-    thread->handle = (void*)CreateThread(NULL, 0, win32thread_worker, thread,
-                                           0, NULL);
+    ret->handle = (void*)CreateThread(NULL, 0, win32thread_worker, ret,
+                                      0, NULL);
 #else
-    thread->handle = (void*)_beginthreadex(NULL, 0, win32thread_worker, thread,
-                                           0, NULL);
+    ret->handle = (void*)_beginthreadex(NULL, 0, win32thread_worker, ret,
+                                        0, NULL);
 #endif
-    return !thread->handle;
+
+    if (!ret->handle) {
+        av_free(ret);
+        return EAGAIN;
+    }
+
+    *thread = ret;
+
+    return 0;
 }
 
 static av_unused int pthread_join(pthread_t thread, void **value_ptr)
 {
-    DWORD ret = WaitForSingleObject(thread.handle, INFINITE);
+    DWORD ret = WaitForSingleObject(thread->handle, INFINITE);
     if (ret != WAIT_OBJECT_0) {
         if (ret == WAIT_ABANDONED)
             return EINVAL;
@@ -104,8 +118,9 @@ static av_unused int pthread_join(pthread_t thread, void **value_ptr)
             return EDEADLK;
     }
     if (value_ptr)
-        *value_ptr = thread.ret;
-    CloseHandle(thread.handle);
+        *value_ptr = thread->ret;
+    CloseHandle(thread->handle);
+    av_free(thread);
     return 0;
 }