From 361ff90fadd2da4f55963385271a7541a397b141 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 16 Sep 2020 17:11:43 -0700 Subject: [PATCH 1/3] Fix a pthreads race condition --- .../musl/src/thread/pthread_mutex_timedlock.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c index 0a240e7913418..e8e73d3e64b3e 100644 --- a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c +++ b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c @@ -10,7 +10,22 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec r = pthread_mutex_trylock(m); if (r != EBUSY) return r; - + +#ifdef __EMSCRIPTEN__ + // If no time is given, do not sleep forever - there is a possible race + // condition here. In the below loop, we need to keep iterating, since _m_lock + // may be set to the value we want *just* before the __timedwait. __timedwait + // does not check if the value is what we want, it just checks for a wake on + // that address, so it would wait forever. + // FIXME waiting for 1 second avoids too busy a wait, but also means that we + // may wait up to 1 second in that rare race condition. A more complex + // exponential backoff may make more sense. + // TODO is this due to limitations of futexes on the Web platform, or our + // implementation of them, or is it a general issue in musl? + struct timespec default_at = {.tv_sec = 1, .tv_nsec = 0}; + if (!at) at = &default_at; +#endif + int spins = 100; while (spins-- && m->_m_lock && !m->_m_waiters) a_spin(); From bb08b54d775f42418a17eba4ab73c99a676e1b64 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 16 Sep 2020 18:22:53 -0700 Subject: [PATCH 2/3] shorter --- system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c index e8e73d3e64b3e..2627ba3e0b92e 100644 --- a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c +++ b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c @@ -17,12 +17,10 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec // may be set to the value we want *just* before the __timedwait. __timedwait // does not check if the value is what we want, it just checks for a wake on // that address, so it would wait forever. - // FIXME waiting for 1 second avoids too busy a wait, but also means that we - // may wait up to 1 second in that rare race condition. A more complex - // exponential backoff may make more sense. + // TODO what is a good amount of time to wait here? // TODO is this due to limitations of futexes on the Web platform, or our // implementation of them, or is it a general issue in musl? - struct timespec default_at = {.tv_sec = 1, .tv_nsec = 0}; + struct timespec default_at = {.tv_sec = 0, .tv_nsec = 1000}; if (!at) at = &default_at; #endif From c959116f6d48abc8fb4c0beb996b8b9e7072eeb7 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 16 Sep 2020 18:53:25 -0700 Subject: [PATCH 3/3] better --- .../musl/src/thread/pthread_mutex_timedlock.c | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c index 2627ba3e0b92e..fdaf9fca76fdf 100644 --- a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c +++ b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c @@ -10,20 +10,7 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec r = pthread_mutex_trylock(m); if (r != EBUSY) return r; - -#ifdef __EMSCRIPTEN__ - // If no time is given, do not sleep forever - there is a possible race - // condition here. In the below loop, we need to keep iterating, since _m_lock - // may be set to the value we want *just* before the __timedwait. __timedwait - // does not check if the value is what we want, it just checks for a wake on - // that address, so it would wait forever. - // TODO what is a good amount of time to wait here? - // TODO is this due to limitations of futexes on the Web platform, or our - // implementation of them, or is it a general issue in musl? - struct timespec default_at = {.tv_sec = 0, .tv_nsec = 1000}; - if (!at) at = &default_at; -#endif - + int spins = 100; while (spins-- && m->_m_lock && !m->_m_waiters) a_spin(); @@ -33,7 +20,19 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec if ((m->_m_type&3) == PTHREAD_MUTEX_ERRORCHECK && (r&0x7fffffff) == __pthread_self()->tid) return EDEADLK; - +#ifdef __EMSCRIPTEN__ + // If no time is given, do not sleep forever - there is a possible race + // condition here. We need to keep iterating in this loop, since _m_lock + // may be set to the value we want *just* before the __timedwait. __timedwait + // does not check if the value is what we want, it just checks for a wake on + // that address, so it would wait forever. + // Instead of picking some arbitrary time to wait, just keep busy-waiting. + if (!at) { + r = pthread_mutex_trylock(m); + if (r != EBUSY) return r; + } + continue; +#endif a_inc(&m->_m_waiters); t = r | 0x80000000; a_cas(&m->_m_lock, r, t);