Thanks for taking a look at this Tobias, and I'm sorry for the messy PoC. It's actually intended for a fallback implementation of pthread_barrier_wait from libuv, and that is where the code comes from. The same issue exists there. The code is still functionally equivalent to the rthread implementation (as far as I can tell).
I will try your patch and post back here. > On 15 Mar 2016, at 01:30, Tobias Ulmer <[email protected]> wrote: > > On Tue, Mar 15, 2016 at 12:51:45AM +0100, Tobias Ulmer wrote: >> On Thu, Mar 10, 2016 at 10:23:17PM +0000, Kári Tristan Helgason wrote: >>> I think I may have come across a race in the implementation of >>> `pthread_barrier_wait` in librthread. > > I'm pretty tired but I think I know what you're talking about. Can you > try this diff? > > We set 'sofar' to 0 first thing when the condition is met, allowing > threads to destroy a barrier that's still in use. > > Index: rthread_barrier.c > =================================================================== > RCS file: /home/vcs/cvs/openbsd/src/lib/librthread/rthread_barrier.c,v > retrieving revision 1.2 > diff -u -p -r1.2 rthread_barrier.c > --- rthread_barrier.c 23 Apr 2012 08:30:33 -0000 1.2 > +++ rthread_barrier.c 15 Mar 2016 01:10:25 -0000 > @@ -70,16 +70,23 @@ int > pthread_barrier_destroy(pthread_barrier_t *barrier) > { > pthread_barrier_t b; > + int rc; > > if (barrier == NULL || *barrier == NULL) > return (EINVAL); > > b = *barrier; > > - if (b->sofar > 0) > + if ((rc = pthread_mutex_trylock(&b->mutex))) > + return rc; > + > + if (b->sofar > 0) { > + pthread_mutex_unlock(&b->mutex); > return (EBUSY); > + } > > *barrier = NULL; > + pthread_mutex_unlock(&b->mutex); > pthread_mutex_destroy(&b->mutex); > pthread_cond_destroy(&b->cond); > free(b); > > > --8<-- > /* modified test program */ > #include <stdio.h> > #include <pthread.h> > #include <stdlib.h> > #include <unistd.h> > > pthread_barrier_t barrier; > > void *threadfn(void *arg) { > int num = (int)arg; > fprintf(stderr, "Thread %d started\n", num); > srandom(num); > sleep(random() % 5 + 1); > pthread_barrier_wait(&barrier); > fprintf(stderr, "Thread %d ended\n", num); > } > > > int main(int argc, char *argv[]) { > int numThreads = 4; > int i, rc; > pthread_barrier_init(&barrier, NULL, numThreads+1); > pthread_t *thread = malloc(sizeof(pthread_t) * numThreads); > for(i = 0; i < numThreads; i++) { > pthread_create(&thread[i], NULL, threadfn, (void*)i); > } > fprintf(stderr, "Main thread done spawning threads\n"); > pthread_barrier_wait(&barrier); > fprintf(stderr, "Main thread done waiting\n"); > > do { > rc = pthread_barrier_destroy(&barrier); > fprintf(stderr, "pthread_barrier_destroy returned %d\n", rc); > } while (rc != 0); > return 0; > } > --8<--
