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

[Philippe Mathieu-Daudé]

On Tue, Jul 4, 2017 at 9:23 AM, Fam Zheng <address@hidden> 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>

Reviewed-by: Philippe Mathieu-Daudé <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;
>  };
>
>  struct QemuCond {
>      pthread_cond_t cond;
> +    bool initialized;
>  };
>
>  struct QemuSemaphore {
> @@ -26,6 +28,7 @@ struct QemuSemaphore {
>  #else
>      sem_t sem;
>  #endif
> +    bool initialized;
>  };
>
>  struct QemuEvent {
> @@ -34,6 +37,7 @@ struct QemuEvent {
>      pthread_cond_t cond;
>  #endif
>      unsigned value;
> +    bool initialized;
>  };
>
>  struct QemuThread {
> diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
> index 4c4a261..3a05e3b 100644
> --- a/include/qemu/thread-win32.h
> +++ b/include/qemu/thread-win32.h
> @@ -5,11 +5,13 @@
>
>  struct QemuMutex {
>      SRWLOCK lock;
> +    bool initialized;
>  };
>
>  typedef struct QemuRecMutex QemuRecMutex;
>  struct QemuRecMutex {
>      CRITICAL_SECTION lock;
> +    bool initialized;
>  };
>
>  void qemu_rec_mutex_destroy(QemuRecMutex *mutex);
> @@ -19,15 +21,18 @@ void qemu_rec_mutex_unlock(QemuRecMutex *mutex);
>
>  struct QemuCond {
>      CONDITION_VARIABLE var;
> +    bool initialized;
>  };
>
>  struct QemuSemaphore {
>      HANDLE sema;
> +    bool initialized;
>  };
>
>  struct QemuEvent {
>      int value;
>      HANDLE event;
> +    bool initialized;
>  };
>
>  typedef struct QemuThreadData QemuThreadData;
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index eacd99e..4e95d27 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -43,12 +43,15 @@ void qemu_mutex_init(QemuMutex *mutex)
>      err = pthread_mutex_init(&mutex->lock, NULL);
>      if (err)
>          error_exit(err, __func__);
> +    mutex->initialized = true;
>  }
>
>  void qemu_mutex_destroy(QemuMutex *mutex)
>  {
>      int err;
>
> +    assert(mutex->initialized);
> +    mutex->initialized = false;
>      err = pthread_mutex_destroy(&mutex->lock);
>      if (err)
>          error_exit(err, __func__);
> @@ -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__);
> @@ -69,6 +73,7 @@ int qemu_mutex_trylock(QemuMutex *mutex)
>  {
>      int err;
>
> +    assert(mutex->initialized);
>      err = pthread_mutex_trylock(&mutex->lock);
>      if (err == 0) {
>          trace_qemu_mutex_locked(mutex);
> @@ -84,6 +89,7 @@ void qemu_mutex_unlock(QemuMutex *mutex)
>  {
>      int err;
>
> +    assert(mutex->initialized);
>      trace_qemu_mutex_unlocked(mutex);
>      err = pthread_mutex_unlock(&mutex->lock);
>      if (err)
> @@ -102,6 +108,7 @@ void qemu_rec_mutex_init(QemuRecMutex *mutex)
>      if (err) {
>          error_exit(err, __func__);
>      }
> +    mutex->initialized = true;
>  }
>
>  void qemu_cond_init(QemuCond *cond)
> @@ -111,12 +118,15 @@ void qemu_cond_init(QemuCond *cond)
>      err = pthread_cond_init(&cond->cond, NULL);
>      if (err)
>          error_exit(err, __func__);
> +    cond->initialized = true;
>  }
>
>  void qemu_cond_destroy(QemuCond *cond)
>  {
>      int err;
>
> +    assert(cond->initialized);
> +    cond->initialized = false;
>      err = pthread_cond_destroy(&cond->cond);
>      if (err)
>          error_exit(err, __func__);
> @@ -126,6 +136,7 @@ void qemu_cond_signal(QemuCond *cond)
>  {
>      int err;
>
> +    assert(cond->initialized);
>      err = pthread_cond_signal(&cond->cond);
>      if (err)
>          error_exit(err, __func__);
> @@ -135,6 +146,7 @@ void qemu_cond_broadcast(QemuCond *cond)
>  {
>      int err;
>
> +    assert(cond->initialized);
>      err = pthread_cond_broadcast(&cond->cond);
>      if (err)
>          error_exit(err, __func__);
> @@ -144,6 +156,7 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
>  {
>      int err;
>
> +    assert(cond->initialized);
>      trace_qemu_mutex_unlocked(mutex);
>      err = pthread_cond_wait(&cond->cond, &mutex->lock);
>      trace_qemu_mutex_locked(mutex);
> @@ -174,12 +187,15 @@ void qemu_sem_init(QemuSemaphore *sem, int init)
>          error_exit(errno, __func__);
>      }
>  #endif
> +    sem->initialized = true;
>  }
>
>  void qemu_sem_destroy(QemuSemaphore *sem)
>  {
>      int rc;
>
> +    assert(sem->initialized);
> +    sem->initialized = false;
>  #if defined(__APPLE__) || defined(__NetBSD__)
>      rc = pthread_cond_destroy(&sem->cond);
>      if (rc < 0) {
> @@ -201,6 +217,7 @@ void qemu_sem_post(QemuSemaphore *sem)
>  {
>      int rc;
>
> +    assert(sem->initialized);
>  #if defined(__APPLE__) || defined(__NetBSD__)
>      pthread_mutex_lock(&sem->lock);
>      if (sem->count == UINT_MAX) {
> @@ -238,6 +255,7 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
>      int rc;
>      struct timespec ts;
>
> +    assert(sem->initialized);
>  #if defined(__APPLE__) || defined(__NetBSD__)
>      rc = 0;
>      compute_abs_deadline(&ts, ms);
> @@ -285,6 +303,7 @@ void qemu_sem_wait(QemuSemaphore *sem)
>  {
>      int rc;
>
> +    assert(sem->initialized);
>  #if defined(__APPLE__) || defined(__NetBSD__)
>      pthread_mutex_lock(&sem->lock);
>      while (sem->count == 0) {
> @@ -310,6 +329,7 @@ void qemu_sem_wait(QemuSemaphore *sem)
>  #else
>  static inline void qemu_futex_wake(QemuEvent *ev, int n)
>  {
> +    assert(ev->initialized);
>      pthread_mutex_lock(&ev->lock);
>      if (n == 1) {
>          pthread_cond_signal(&ev->cond);
> @@ -321,6 +341,7 @@ static inline void qemu_futex_wake(QemuEvent *ev, int n)
>
>  static inline void qemu_futex_wait(QemuEvent *ev, unsigned val)
>  {
> +    assert(ev->initialized);
>      pthread_mutex_lock(&ev->lock);
>      if (ev->value == val) {
>          pthread_cond_wait(&ev->cond, &ev->lock);
> @@ -355,10 +376,13 @@ void qemu_event_init(QemuEvent *ev, bool init)
>  #endif
>
>      ev->value = (init ? EV_SET : EV_FREE);
> +    ev->initialized = true;
>  }
>
>  void qemu_event_destroy(QemuEvent *ev)
>  {
> +    assert(ev->initialized);
> +    ev->initialized = false;
>  #ifndef __linux__
>      pthread_mutex_destroy(&ev->lock);
>      pthread_cond_destroy(&ev->cond);
> @@ -370,6 +394,7 @@ void qemu_event_set(QemuEvent *ev)
>      /* qemu_event_set has release semantics, but because it *loads*
>       * ev->value we need a full memory barrier here.
>       */
> +    assert(ev->initialized);
>      smp_mb();
>      if (atomic_read(&ev->value) != EV_SET) {
>          if (atomic_xchg(&ev->value, EV_SET) == EV_BUSY) {
> @@ -383,6 +408,7 @@ void qemu_event_reset(QemuEvent *ev)
>  {
>      unsigned value;
>
> +    assert(ev->initialized);
>      value = atomic_read(&ev->value);
>      smp_mb_acquire();
>      if (value == EV_SET) {
> @@ -398,6 +424,7 @@ void qemu_event_wait(QemuEvent *ev)
>  {
>      unsigned value;
>
> +    assert(ev->initialized);
>      value = atomic_read(&ev->value);
>      smp_mb_acquire();
>      if (value != EV_SET) {
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 653f29f..94f3491 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -46,15 +46,19 @@ static void error_exit(int err, const char *msg)
>  void qemu_mutex_init(QemuMutex *mutex)
>  {
>      InitializeSRWLock(&mutex->lock);
> +    mutex->initialized = true;
>  }
>
>  void qemu_mutex_destroy(QemuMutex *mutex)
>  {
> +    assert(mutex->initialized);
> +    mutex->initialized = false;
>      InitializeSRWLock(&mutex->lock);
>  }
>
>  void qemu_mutex_lock(QemuMutex *mutex)
>  {
> +    assert(mutex->initialized);
>      AcquireSRWLockExclusive(&mutex->lock);
>      trace_qemu_mutex_locked(mutex);
>  }
> @@ -63,6 +67,7 @@ int qemu_mutex_trylock(QemuMutex *mutex)
>  {
>      int owned;
>
> +    assert(mutex->initialized);
>      owned = TryAcquireSRWLockExclusive(&mutex->lock);
>      if (owned) {
>          trace_qemu_mutex_locked(mutex);
> @@ -73,6 +78,7 @@ int qemu_mutex_trylock(QemuMutex *mutex)
>
>  void qemu_mutex_unlock(QemuMutex *mutex)
>  {
> +    assert(mutex->initialized);
>      trace_qemu_mutex_unlocked(mutex);
>      ReleaseSRWLockExclusive(&mutex->lock);
>  }
> @@ -80,25 +86,31 @@ void qemu_mutex_unlock(QemuMutex *mutex)
>  void qemu_rec_mutex_init(QemuRecMutex *mutex)
>  {
>      InitializeCriticalSection(&mutex->lock);
> +    mutex->initialized = true;
>  }
>
>  void qemu_rec_mutex_destroy(QemuRecMutex *mutex)
>  {
> +    assert(mutex->initialized);
> +    mutex->initialized = false;
>      DeleteCriticalSection(&mutex->lock);
>  }
>
>  void qemu_rec_mutex_lock(QemuRecMutex *mutex)
>  {
> +    assert(mutex->initialized);
>      EnterCriticalSection(&mutex->lock);
>  }
>
>  int qemu_rec_mutex_trylock(QemuRecMutex *mutex)
>  {
> +    assert(mutex->initialized);
>      return !TryEnterCriticalSection(&mutex->lock);
>  }
>
>  void qemu_rec_mutex_unlock(QemuRecMutex *mutex)
>  {
> +    assert(mutex->initialized);
>      LeaveCriticalSection(&mutex->lock);
>  }
>
> @@ -106,25 +118,31 @@ void qemu_cond_init(QemuCond *cond)
>  {
>      memset(cond, 0, sizeof(*cond));
>      InitializeConditionVariable(&cond->var);
> +    cond->initialized = true;
>  }
>
>  void qemu_cond_destroy(QemuCond *cond)
>  {
> +    assert(cond->initialized);
> +    cond->initialized = false;
>      InitializeConditionVariable(&cond->var);
>  }
>
>  void qemu_cond_signal(QemuCond *cond)
>  {
> +    assert(cond->initialized);
>      WakeConditionVariable(&cond->var);
>  }
>
>  void qemu_cond_broadcast(QemuCond *cond)
>  {
> +    assert(cond->initialized);
>      WakeAllConditionVariable(&cond->var);
>  }
>
>  void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
>  {
> +    assert(cond->initialized);
>      trace_qemu_mutex_unlocked(mutex);
>      SleepConditionVariableSRW(&cond->var, &mutex->lock, INFINITE, 0);
>      trace_qemu_mutex_locked(mutex);
> @@ -134,21 +152,28 @@ void qemu_sem_init(QemuSemaphore *sem, int init)
>  {
>      /* Manual reset.  */
>      sem->sema = CreateSemaphore(NULL, init, LONG_MAX, NULL);
> +    sem->initialized = true;
>  }
>
>  void qemu_sem_destroy(QemuSemaphore *sem)
>  {
> +    assert(sem->initialized);
> +    sem->initialized = false;
>      CloseHandle(sem->sema);
>  }
>
>  void qemu_sem_post(QemuSemaphore *sem)
>  {
> +    assert(sem->initialized);
>      ReleaseSemaphore(sem->sema, 1, NULL);
>  }
>
>  int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
>  {
> -    int rc = WaitForSingleObject(sem->sema, ms);
> +    int rc;
> +
> +    assert(sem->initialized);
> +    rc = WaitForSingleObject(sem->sema, ms);
>      if (rc == WAIT_OBJECT_0) {
>          return 0;
>      }
> @@ -160,6 +185,7 @@ int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
>
>  void qemu_sem_wait(QemuSemaphore *sem)
>  {
> +    assert(sem->initialized);
>      if (WaitForSingleObject(sem->sema, INFINITE) != WAIT_OBJECT_0) {
>          error_exit(GetLastError(), __func__);
>      }
> @@ -193,15 +219,19 @@ void qemu_event_init(QemuEvent *ev, bool init)
>      /* Manual reset.  */
>      ev->event = CreateEvent(NULL, TRUE, TRUE, NULL);
>      ev->value = (init ? EV_SET : EV_FREE);
> +    ev->initialized = true;
>  }
>
>  void qemu_event_destroy(QemuEvent *ev)
>  {
> +    assert(ev->initialized);
> +    ev->initialized = false;
>      CloseHandle(ev->event);
>  }
>
>  void qemu_event_set(QemuEvent *ev)
>  {
> +    assert(ev->initialized);
>      /* qemu_event_set has release semantics, but because it *loads*
>       * ev->value we need a full memory barrier here.
>       */
> @@ -218,6 +248,7 @@ void qemu_event_reset(QemuEvent *ev)
>  {
>      unsigned value;
>
> +    assert(ev->initialized);
>      value = atomic_read(&ev->value);
>      smp_mb_acquire();
>      if (value == EV_SET) {
> @@ -232,6 +263,7 @@ void qemu_event_wait(QemuEvent *ev)
>  {
>      unsigned value;
>
> +    assert(ev->initialized);
>      value = atomic_read(&ev->value);
>      smp_mb_acquire();
>      if (value != EV_SET) {
> --
> 2.9.4
>
>