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 <tobi...@tmux.org> 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<--
