In do_fork() and clone_func() we currently use a pthread_mutex_t and a pthread_cond_t to allow the parent thread to wait until the child thread has started and e.g. set fields like info->tid, before the parent reads those fields and otherwise proceeds.
Coverity points out that this use of a pthread condition variable isn't reliable: pthread_cond_wait() may wake up spuriously, so the idiom for "wait on a condition" should be "loop around calling pthread_cond_wait() until a predicate that was updated by the caller of pthread_cond_broadcast() is true". This is quite old code; rather than fixing up its use of raw pthread condition variables, we can use a QemuSemaphore. This gives us the "thread A waits until thread B signals it" semantics we want. (Under the hood it is implemented with a mutex and condition variable.) Cc: [email protected] Resolves: CID 1527417 Signed-off-by: Peter Maydell <[email protected]> --- cc stable because in theory this could result in the parent thread waking up too early and weird stuff happening; however my guess is that in practice with one waiter and one signaller that the "spurious wakeup" case won't happen. At any rate this code has been like this for a very long time. Disclaimer: tested with "make check" only, but that does include various linux-user tests that use multiple threads. --- linux-user/syscall.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 0380691a8b1..9d85a193efe 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -6932,8 +6932,7 @@ static abi_long do_map_shadow_stack(CPUArchState *env, abi_ulong addr, static pthread_mutex_t clone_lock = PTHREAD_MUTEX_INITIALIZER; typedef struct { CPUArchState *env; - pthread_mutex_t mutex; - pthread_cond_t cond; + QemuSemaphore sem; pthread_t thread; uint32_t tid; abi_ulong child_tidptr; @@ -6964,9 +6963,7 @@ static void *clone_func(void *arg) /* Enable signals. */ sigprocmask(SIG_SETMASK, &info->sigmask, NULL); /* Signal to the parent that we're ready. */ - pthread_mutex_lock(&info->mutex); - pthread_cond_broadcast(&info->cond); - pthread_mutex_unlock(&info->mutex); + qemu_sem_post(&info->sem); /* Wait until the parent has finished initializing the tls state. */ pthread_mutex_lock(&clone_lock); pthread_mutex_unlock(&clone_lock); @@ -7071,9 +7068,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp, } memset(&info, 0, sizeof(info)); - pthread_mutex_init(&info.mutex, NULL); - pthread_mutex_lock(&info.mutex); - pthread_cond_init(&info.cond, NULL); + qemu_sem_init(&info.sem, 0); info.env = new_env; if (flags & CLONE_CHILD_SETTID) { info.child_tidptr = child_tidptr; @@ -7097,7 +7092,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp, pthread_attr_destroy(&attr); if (ret == 0) { /* Wait for the child to initialize. */ - pthread_cond_wait(&info.cond, &info.mutex); + qemu_sem_wait(&info.sem); ret = info.tid; } else { errno = ret; @@ -7111,9 +7106,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp, #endif g_free(ts); } - pthread_mutex_unlock(&info.mutex); - pthread_cond_destroy(&info.cond); - pthread_mutex_destroy(&info.mutex); + qemu_sem_destroy(&info.sem); pthread_mutex_unlock(&clone_lock); } else { /* if no CLONE_VM, we consider it is a fork */ -- 2.43.0
