[FFmpeg-devel] [PATCH] compat/w32pthreads: change pthread_t into pointer to malloced struct

[Martin Storsjö]

On Thu, 12 Dec 2024, Anton Khirnov wrote:

> 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.
> ---
> compat/w32pthreads.h | 39 +++++++++++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 12 deletions(-)

As you've pinpointed the commit that made this issue appear much more 
frequently, it'd be nice to include that info.

> 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;

ENOMEM?

> +
> +    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;

Hmm, not sure about this error code either?

> +    }
> +
> +    *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;
> }

Other than that, LGTM, thanks!

// Martin