[PATCH] Teach pthread checker about double unlocks

[Daniel Fahlgren]

Hi,

On Wed, 2014-03-19 at 20:15 -0700, Jordan Rose wrote:

> Right, at which point it's really just a set.

Yes. I think I already had implemented tracking of calls to init/destroy
in my mind when I wrote that code.
> > 
> > I'll go back to the drawing board and try to think of a way to
> > address
> > the recursive locks while at it. What is your opinion about default
> > behavior regarding recursive locks?
> > 
> > int main(int argc, char *argv[])
> > {
> >  pthread_mutex_t mtx = PTHREAD_MUTEX_INITIALIZER;
> >  pthread_mutex_lock(&mtx);
> >  pthread_mutex_lock(&mtx);
> >  return 0;
> > }
> > 
> > will behave differently depending on the OS. On FreeBSD and Solaris
> > the process will terminate while Linux and AIX will deadlock. How OS
> > X behaves I do not know. Should the default be to allow recursive
> > locking, not allow it, or depend on the current OS?
> > 
> > Btw, what do you mean with "to support an unbalanced unlock"?

> void dispose(struct resource_t *rsrc) {
>   free(rsrc->buffer);
>   pthread_mutex_unlock(rsrc->buffer);
> }
> 
> This kind of thing. In this case, we're trying to unlock something
> even though we've never seen it get locked. We shouldn't warn on this,
> just like we shouldn't warn on the free() here.

I understand. Yes, that must be supported. Otherwise the checker will
produce false positives and be much less useful. However, that exact
example should trigger other warnings in the analyzer ;)
> 
> I'm not sure what to do about recursive locks. I can think of a few
> choices for how to control it:
> 
> 
> 1. Use an -analyzer-config option. Downside: a bit annoying to access
> from checkers, not necessarily something we want users to think about
> (at least not right now).
> 2. Have two variants of the checker, and allow the user to override
> the default by asking for the other one.
> 3. Don't allow the user to override our default.
> 
> 
> ...and then choices for our default:
> 
> 
> 1. Allow recursive locks (severely reduces checking).
> 2. Disallow recursive locks (false positives sometimes).
> 3. Disallow recursive locks on platforms where that's not the default.
> 
> 
> (FWIW it looks like OS X also uses non-recursive locks by default. I
> didn't test it but I assume iOS does as well.)
> 
> 
> I'm inclined to say that for now we should just continue pretending
> all locks are non-recursive, but possibly separate out the lock/unlock
> checking from the lock order checking, so that if your application
> makes heavy use of recursive locks you could just disable that part of
> the checker. What do you think?

I agree that non-recursive locks is a sane default. On FreeBSD the false
positives will serve as a warning that the code is non-portable. We
might need to do more than just disabling that part to properly support
recursive locks. For example:

void foo() {
  pthread_mutex_init(&mtx, NULL);
  pthread_mutex_lock(&mtx);
  pthread_mutex_unlock(&mtx);
  pthread_mutex_unlock(&mtx);
}

should still yield a warning. However, if the call to pthread_mutex_init
isn't present the checker shouldn't warn about the double unlock. I'll
need to think more about this. 

> Jordan

> P.S. If we can see where a mutex is created, we could also record if
> it was created with the "recursive" flag. But I think most uses of
> locks aren't right next to the creation point.
> 
That would definitely be nice. One sad thing is that
PTHREAD_MUTEX_INITIALIZER is totally different on different OSes (E.g.
NULL on FreeBSD and a struct on Linux). Also, pthread_mutex_init(&mtx,
NULL) doesn't contain information if the mutex is recursive or not.

Daniel