You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Joe Orton <jo...@redhat.com> on 2021/07/02 13:19:30 UTC
thread mutex double-locking w/o _timedlock
If pthread_mutex_timedlock() is not supported, apr_thread_mutex_unlock()
will take a locked mutex and immediately lock it again:
https://github.com/apache/apr/blob/trunk/locks/unix/thread_mutex.c#L297
APR_DECLARE(apr_status_t) apr_thread_mutex_unlock(apr_thread_mutex_t *mutex)
{
apr_status_t status;
if (mutex->cond) {
status = pthread_mutex_lock(&mutex->mutex);
This is undefined behaviour unless APR ensures that mutex is recursive,
which it doesn't AFAICT.
Current versions of Coverity are smart enough to detect double-locking
and hence this code triggers a scanner warning for every single call to
apr_thread_mutex_unlock() within APR.
I'm not sure how to fix the problem, maybe force mutexes to be
recursive? But isn't it also true that all this code can be
#if'ed out for platforms which do support pthread_mutex_timedlock()?
e.g. as attached.
Regards, Joe
Re: thread mutex double-locking w/o _timedlock
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Jul 02, 2021 at 04:10:08PM +0200, Yann Ylavic wrote:
> On Fri, Jul 2, 2021 at 3:51 PM Yann Ylavic <yl...@gmail.com> wrote:
> >
> > On Fri, Jul 2, 2021 at 3:19 PM Joe Orton <jo...@redhat.com> wrote:
> > >
> > > If pthread_mutex_timedlock() is not supported, apr_thread_mutex_unlock()
> > > will take a locked mutex and immediately lock it again:
> > >
> > > https://github.com/apache/apr/blob/trunk/locks/unix/thread_mutex.c#L297
> > >
> > > APR_DECLARE(apr_status_t) apr_thread_mutex_unlock(apr_thread_mutex_t *mutex)
> > > {
> > > apr_status_t status;
> > >
> > > if (mutex->cond) {
> > > status = pthread_mutex_lock(&mutex->mutex);
> > >
> > > This is undefined behaviour unless APR ensures that mutex is recursive,
> > > which it doesn't AFAICT.
> >
> > When mutex->cond != NULL (ie !HAVE_PTHREAD_MUTEX_TIMEDLOCK),
> > apr_thread_mutex_lock() and co don't leave mutex->mutex locked (the
> > actual locking happens thanks to the ->cond wait on the ->locked
> > flag).
> >
> > So apr_thread_mutex_unlock() can (and actually must) lock ->mutex to
> > signal the ->cond and clear the ->locked.
> > What am I missing?
Ahhhh, that makes sense. Sorry, I missed that, you're not missing
anything, tracing through pages of Coverity output dulls my remaining
brain capacity...
> Coverity likely can't figure out without the #ifdefs, so both your
> patch and the code look good to me :)
Great, thanks Yann. I removed the other fields too and dropped the
bogus comment, -> r1891204.
Regards, Joe
Re: thread mutex double-locking w/o _timedlock
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jul 2, 2021 at 3:51 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Jul 2, 2021 at 3:19 PM Joe Orton <jo...@redhat.com> wrote:
> >
> > If pthread_mutex_timedlock() is not supported, apr_thread_mutex_unlock()
> > will take a locked mutex and immediately lock it again:
> >
> > https://github.com/apache/apr/blob/trunk/locks/unix/thread_mutex.c#L297
> >
> > APR_DECLARE(apr_status_t) apr_thread_mutex_unlock(apr_thread_mutex_t *mutex)
> > {
> > apr_status_t status;
> >
> > if (mutex->cond) {
> > status = pthread_mutex_lock(&mutex->mutex);
> >
> > This is undefined behaviour unless APR ensures that mutex is recursive,
> > which it doesn't AFAICT.
>
> When mutex->cond != NULL (ie !HAVE_PTHREAD_MUTEX_TIMEDLOCK),
> apr_thread_mutex_lock() and co don't leave mutex->mutex locked (the
> actual locking happens thanks to the ->cond wait on the ->locked
> flag).
>
> So apr_thread_mutex_unlock() can (and actually must) lock ->mutex to
> signal the ->cond and clear the ->locked.
> What am I missing?
Coverity likely can't figure out without the #ifdefs, so both your
patch and the code look good to me :)
Re: thread mutex double-locking w/o _timedlock
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jul 2, 2021 at 3:19 PM Joe Orton <jo...@redhat.com> wrote:
>
> If pthread_mutex_timedlock() is not supported, apr_thread_mutex_unlock()
> will take a locked mutex and immediately lock it again:
>
> https://github.com/apache/apr/blob/trunk/locks/unix/thread_mutex.c#L297
>
> APR_DECLARE(apr_status_t) apr_thread_mutex_unlock(apr_thread_mutex_t *mutex)
> {
> apr_status_t status;
>
> if (mutex->cond) {
> status = pthread_mutex_lock(&mutex->mutex);
>
> This is undefined behaviour unless APR ensures that mutex is recursive,
> which it doesn't AFAICT.
When mutex->cond != NULL (ie !HAVE_PTHREAD_MUTEX_TIMEDLOCK),
apr_thread_mutex_lock() and co don't leave mutex->mutex locked (the
actual locking happens thanks to the ->cond wait on the ->locked
flag).
So apr_thread_mutex_unlock() can (and actually must) lock ->mutex to
signal the ->cond and clear the ->locked.
What am I missing?
Regards;
Yann.