From 2503b0fffc19268396dbf4fa60071e1e04ef9908 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 21 Jan 2025 18:44:42 +0000 Subject: [PATCH 1/4] Replace data race with use of pthread_once (ftpl_init) At the cost of no longer nicely detecting recursive initialisation problems. Fixes Debian bug #1093599 --- src/libfaketime.c | 50 +++++++---------------------------------------- 1 file changed, 7 insertions(+), 43 deletions(-) diff --git a/src/libfaketime.c b/src/libfaketime.c index c59c122..f3e4690 100644 --- a/src/libfaketime.c +++ b/src/libfaketime.c @@ -304,7 +304,7 @@ static bool check_missing_real(const char *name, bool missing) #define CHECK_MISSING_REAL(name) \ check_missing_real(#name, (NULL == real_##name)) -static int initialized = 0; +static pthread_once_t initialized_once_control = PTHREAD_ONCE_INIT; /* prototypes */ static int fake_gettimeofday(struct timeval *tv); @@ -2346,44 +2346,12 @@ int clock_gettime(clockid_t clk_id, struct timespec *tp) #endif { int result; - static int recursion_depth = 0; - if (!initialized) - { - recursion_depth++; -#ifdef FAIL_PRE_INIT_CALLS - fprintf(stderr, "libfaketime: clock_gettime() was called before initialization.\n"); - fprintf(stderr, "libfaketime: Returning -1 on clock_gettime().\n"); - if (tp != NULL) - { - tp->tv_sec = 0; - tp->tv_nsec = 0; - } - return -1; -#else - if (recursion_depth == 2) - { - fprintf(stderr, "libfaketime: Unexpected recursive calls to clock_gettime() without proper initialization. Trying alternative.\n"); - DONT_FAKE_TIME(ftpl_init()) ; - } - else if (recursion_depth == 3) - { - fprintf(stderr, "libfaketime: Cannot recover from unexpected recursive calls to clock_gettime().\n"); - fprintf(stderr, "libfaketime: Please check whether any other libraries are in use that clash with libfaketime.\n"); - fprintf(stderr, "libfaketime: Returning -1 on clock_gettime() to break recursion now... if that does not work, please check other libraries' error handling.\n"); - if (tp != NULL) - { - tp->tv_sec = 0; - tp->tv_nsec = 0; - } - return -1; - } - else { - ftpl_init(); - } -#endif - recursion_depth--; - } + ftpl_init(); + // If ftpl_init ends up recursing, pthread_once will deadlock. + // (Previously we attempted to detect this situation, and bomb out, + // but the approach taken wasn't thread-safe and broke in practice.) + /* sanity check */ if (tp == NULL) { @@ -2795,7 +2763,6 @@ static void ftpl_really_init(void) #undef dlsym #undef dlvsym - initialized = 1; #ifdef FAKE_STATELESS if (0) ft_shm_init(); @@ -3046,10 +3013,7 @@ static void ftpl_really_init(void) } inline static void ftpl_init(void) { - if (!initialized) - { - ftpl_really_init(); - } + pthread_once(&initialized_once_control, ftpl_really_init); } void *ft_dlvsym(void *handle, const char *symbol, const char *version, From d9ba684b18176f3522b0d66739de2a6185256ed7 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 21 Jan 2025 18:50:07 +0000 Subject: [PATCH 2/4] Replace data race with use of pthread_once (ft_shm_init) --- src/libfaketime.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libfaketime.c b/src/libfaketime.c index f3e4690..4ba5165 100644 --- a/src/libfaketime.c +++ b/src/libfaketime.c @@ -521,16 +521,20 @@ static void ft_shm_destroy(void) } } +static pthread_once_t ft_shm_initialized_once_control = PTHREAD_ONCE_INIT; + +static void ft_shm_really_init (void); static void ft_shm_init (void) +{ + pthread_once(&ft_shm_initialized_once_control, ft_shm_really_init); +} + +static void ft_shm_really_init (void) { int ticks_shm_fd; char sem_name[256], shm_name[256], *ft_shared_env = getenv("FAKETIME_SHARED"); sem_t *shared_semR = NULL; static int nt=1; - static int ft_shm_initialized = 0; - - /* do all of this once only */ - if (ft_shm_initialized > 0) return; /* create semaphore and shared memory locally unless it has been passed along */ if (ft_shared_env == NULL) @@ -610,8 +614,6 @@ static void ft_shm_init (void) { /* force the deletion of the shm sync env variable */ unsetenv("FAKETIME_SHARED"); } - - ft_shm_initialized = 1; } static void ft_cleanup (void) From b6e87c6f26d64accc06b000b07ad9e92fed3e024 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 22 Jan 2025 09:11:46 +0000 Subject: [PATCH 3/4] Call ftpl_init before using monotonic_conds_lock Otherwise we can use this in an uninitialised state, which is not allowed. We call ftpl_init in pthread_cond_init_232, but the application might not have called that. For example, it might have a static condition variable set up with PTHREAD_COND_INITIALIZER. --- src/libfaketime.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libfaketime.c b/src/libfaketime.c index 4ba5165..7f4d0e6 100644 --- a/src/libfaketime.c +++ b/src/libfaketime.c @@ -3705,6 +3705,8 @@ int pthread_cond_destroy_232(pthread_cond_t *cond) { struct pthread_cond_monotonic* e; + ftpl_init(); + if (pthread_rwlock_trywrlock(&monotonic_conds_lock) != 0) { sched_yield(); return EBUSY; @@ -3787,6 +3789,8 @@ int pthread_cond_timedwait_common(pthread_cond_t *cond, pthread_mutex_t *mutex, clockid_t clk_id; int result = 0; + ftpl_init(); + if (abstime != NULL) { if (pthread_rwlock_tryrdlock(&monotonic_conds_lock) != 0) { From 50e2c56914413c8f93b4b8cba9e86a25c60dd184 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 22 Jan 2025 09:29:58 +0000 Subject: [PATCH 4/4] 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);