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

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1994 bytes --]



On 06/07/2017 14: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 <famz@redhat.com>
>> ---
>>  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?

It depends on whether the compiler handles a non-1, nonzero value as
true or false.  If it counts as true, using an uint32_t with a magic
value (0xacce55ed? ;)) would not be fooled by MALLOC_PERTURB_.  This
would be nice.

>> @@ -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?

It should be well predicted, but if it comes up in profiles we can make
it conditional on release versions (or maybe you should use a spinlock
instead).

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]