From 50e2c56914413c8f93b4b8cba9e86a25c60dd184 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 22 Jan 2025 09:29:58 +0000 Subject: [PATCH] Don't use _try_ locking calls for monotonic_conds_lock This reverts commit 8ef74e33b636a53a757a945d8ebc51d0986f0d81 "Swapped out pthread_rwlock_xxlock() ..." This could result in concurrent uses of pthread_cond_* erroneously returning EAGAIN, which is not permitted by the spec and which the application way well treat as a bug. This seems to be happening in gem2deb in ci.debian.net. The commit message in 8ef74e33b636 says (rewrapped) Swapped out pthread_rwlock_xxlock(), which doesn't return if it can't obtain the lock, with pthread_rwlock_xxtrylock() followed by sched yield and error code return. The issue is sometimes a thread calling pthread_cond_init() or pthread_cond_destroy() can't acquire the lock when another thread is waiting on a condition variable notification via pthread_cond_timedwait(), and thus the thread calling pthread_cond_init() or pthread_cond_destroy() end up hanging indefinitely. I don't think this is true. The things that are done with monotonic_conds_lock held are HASH_ADD_PTR HASH_FIND_PTR etc. on monotonic_conds, which should all be fast and AFAICT don't in turn take any locks. So it shouldn't deadlock. I conjecture that the underlying bug being experienced by the author of "Swapped out pthread_rwlock_xxlock" was the lack of ftpl_init - ie, access to an uninitialised pthread_rwlock_t. That might result in a hang. --- src/libfaketime.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libfaketime.c b/src/libfaketime.c index 7f4d0e6..93da650 100644 --- a/src/libfaketime.c +++ b/src/libfaketime.c @@ -3690,9 +3690,9 @@ int pthread_cond_init_232(pthread_cond_t *restrict cond, const pthread_condattr_ struct pthread_cond_monotonic *e = (struct pthread_cond_monotonic*)malloc(sizeof(struct pthread_cond_monotonic)); e->ptr = cond; - if (pthread_rwlock_trywrlock(&monotonic_conds_lock) != 0) { - sched_yield(); - return EAGAIN; + if (pthread_rwlock_wrlock(&monotonic_conds_lock) != 0) { + fprintf(stderr,"can't acquire write monotonic_conds_lock\n"); + exit(-1); } HASH_ADD_PTR(monotonic_conds, ptr, e); pthread_rwlock_unlock(&monotonic_conds_lock); @@ -3707,9 +3707,9 @@ int pthread_cond_destroy_232(pthread_cond_t *cond) ftpl_init(); - if (pthread_rwlock_trywrlock(&monotonic_conds_lock) != 0) { - sched_yield(); - return EBUSY; + if (pthread_rwlock_wrlock(&monotonic_conds_lock) != 0) { + fprintf(stderr,"can't acquire write monotonic_conds_lock\n"); + exit(-1); } HASH_FIND_PTR(monotonic_conds, &cond, e); if (e) { @@ -3793,9 +3793,9 @@ int pthread_cond_timedwait_common(pthread_cond_t *cond, pthread_mutex_t *mutex, if (abstime != NULL) { - if (pthread_rwlock_tryrdlock(&monotonic_conds_lock) != 0) { - sched_yield(); - return EAGAIN; + if (pthread_rwlock_rdlock(&monotonic_conds_lock) != 0) { + fprintf(stderr,"can't acquire read monotonic_conds_lock\n"); + exit(-1); } HASH_FIND_PTR(monotonic_conds, &cond, e); pthread_rwlock_unlock(&monotonic_conds_lock);