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

[Anton Khirnov]

Quoting Martin Storsjö (2024-12-12 18:40:27)
> 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.

Sure.

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

POSIX specifies EAGAIN for "Insufficient resources to create another
thread.", so...

-- 
Anton Khirnov