Re: [Qemu-devel] [PATCH] qemu-thread: Assert locks are initialized before using

[Fam Zheng]

On Thu, 07/06 07:16, Eric Blake wrote:
> On 07/04/2017 07:23 AM, Fam Zheng wrote:
> > Not all platforms check whether a lock is initialized before used.  In
> > particular Linux seems to be more permissive than OSX.
> > 
> > Check initialization state explicitly in our code to catch such bugs
> > earlier.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  include/qemu/thread-posix.h |  4 ++++
> >  include/qemu/thread-win32.h |  5 +++++
> >  util/qemu-thread-posix.c    | 27 +++++++++++++++++++++++++++
> >  util/qemu-thread-win32.c    | 34 +++++++++++++++++++++++++++++++++-
> >  4 files changed, 69 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
> > index 09d1e15..e5e3a0f 100644
> > --- a/include/qemu/thread-posix.h
> > +++ b/include/qemu/thread-posix.h
> > @@ -12,10 +12,12 @@ typedef QemuMutex QemuRecMutex;
> >  
> >  struct QemuMutex {
> >      pthread_mutex_t lock;
> > +    bool initialized;
> >  };
> 
> Are we worried about an object living on the stack and inheriting bit
> values that make the object already appear initialized?  Would a magic
> number a little less likely than '1' reduce the risk of inherited stack
> garbage throwing us off?
> 
> Then again, several years ago, the Cygwin project quit using a magic
> number cookie to track if synchronization objects were initialized, as
> it ran into issues where repeated calls to a function that allocates an
> object would cause the second allocation to fail because it saw leftover
> stack contents from the first time through, so even with it's use of
> something a little less likely than a bool '1', it still became a problem.

I don't know the answer to your question about magic number problem, but more
often than not a lock is heap allocated, so I'm not worried.

> 
> 
> > @@ -58,6 +61,7 @@ void qemu_mutex_lock(QemuMutex *mutex)
> >  {
> >      int err;
> >  
> > +    assert(mutex->initialized);
> >      err = pthread_mutex_lock(&mutex->lock);
> >      if (err)
> >          error_exit(err, __func__);
> 
> Are we sure this isn't going to penalize our code speed, by adding a
> conditional on every lock/unlock?

We already have assertions everywhere, and I've never worried about its
computation cost.  Hot paths should try not to contend on locks anyway.

(This patch is already in master, if there is a problem, it will need a follow
up patch.)

Fam