Bug in pthread_cond_wait...

Bug in pthread_cond_wait...

[Lorin Hochstein]

As mentioned in my previous post, I believe there is a bug in the
pthread_cond_wait code. If the thread is cancelled while blocked in a
pthread_cond_wait, it does not properly restore the condition variable's
state, which can cause a subsequent call to pthread_cond_broadcast to
hang (broadcast will wait forever for the terminated thread to respond).
This is a big problem if you have one producer and multiple consumers,
and one of the consumers gets cancelled.

I don't know what the correct protocol is for submitting bugfixes.
However, for the impatient ones that are encountering the same problem
that I am attaching a patch with this e-mail.

Lorin Hochstein
478a479,489
> 
> static int
> _restore_cond(void *cond)
> {
>   pthread_cond_t cv;
>   cv = *(pthread_cond_t*)cond;
>   cv->waiters--;
>   return 0;
> }
> 
> 
536a548,550
>        *
>        *      We must also ensure that the condition variable is 
>        *      restored to the correct state.
538a553
>       pthread_cleanup_push (_restore_cond, cond);
544a560
>       pthread_cleanup_pop (0);

Re: Bug in pthread_cond_wait...

[Ross Johnson]

Dear all,

I've been collecting all of the messages on this subject and will
hopefully get time in the next few days to sort out any changes that
need to be made to the library. In particular, looking at Peter
Slacik's patches against Douglas Schmit's CV paper as suggested.

Thanks.
Ross Johnson

+----------------------+---+
| Ross Johnson         |   | E-Mail: rpj@ise.canberra.edu.au
| Info Sciences and Eng|___|
| University of Canberra   | FAX:    +61 6 2015227
| PO Box 1                 |
| Belconnen  ACT    2616   | WWW:    http://willow.canberra.edu.au/~rpj/
| AUSTRALIA                |
+--------------------------+

Re: Bug in pthread_cond_wait...

[Peter Slacik]

Lorin Hochstein wrote:

> As mentioned in my previous post, I believe there is a bug in the
> pthread_cond_wait code. If the thread is cancelled while blocked in a
> pthread_cond_wait, it does not properly restore the condition variable's
> state, which can cause a subsequent call to pthread_cond_broadcast to
> hang (broadcast will wait forever for the terminated thread to respond).
> This is a big problem if you have one producer and multiple consumers,
> and one of the consumers gets cancelled.
>
> I don't know what the correct protocol is for submitting bugfixes.
> However, for the impatient ones that are encountering the same problem
> that I am attaching a patch with this e-mail.

This is true, but not enough. Cleanup handler _restore_cond() have to
SetEvent(cv->waitersDone) in the thread is the last waiter (as done in
cond_timedwait() after pthread_cleanup_pop()). Missing to do so might cause the
broadcasting thread to check that waiters count is > 0 and then wait for
waitersDone event to be set.

According to my previously posted changes, _restore_cond() must protect it's
work with cv->waitersLock. Additionally, I forgot to do this in
pthread_cond_signal(), something like:

Index: condvar.c
===================================================================
RCS file: pthread/condvar.c,v
retrieving revision 1.5
diff -c -r1.5 condvar.c
*** condvar.c   1999/07/01 07:20:51     1.5
--- condvar.c   1999/07/01 08:22:28
***************
*** 783,792 ****
--- 783,798 ----
    /*
     * If there aren't any waiters, then this is a no-op.
     */
+   if (pthread_mutex_lock(&(cv->waitersLock)) == EINVAL)
+       return EINVAL;
+
    if (cv->waiters > 0)
      {
        result = sem_post (&(cv->sema));
      }
+
+   if (pthread_mutex_unlock(&(cv->waitersLock)) == EINVAL)
+       return EINVAL;

    return (result);

--
Peter