Torvald Riegel wrote:
> The double-checked locking in glthread_once_multithreaded is broken. It
> has data races. Remember the conversation we had about atomics? Look
> at glibc's pthread_once to see what needs to be done.
>
> A similar problem exists regarding the (use of the) initialized fields
> in the custom locks you implemented. This is not a correct
> pthread_once, and it cannot reliably help you shield users from bad
> consequences due to not calling ..._init().
Good points, thanks a lot for these hints! Find attached the proposed fix.
It's gnulib's first use of <stdatomic.h>...
> I also think that glthread_rwlock_destroy_multithreaded should destroy
> the condvars first, then the mutex. That may not be strictly required
> depending on how one interprets the wording around references to mutexes
> held by condvars.
I don't think the order matters, since the condvars must be empty at this
point. It's surely undefined behaviour to destroy an rwlock that still has
readers or writers pending (although POSIX [1] does not say so).
Anyway, patch attached.
> And you should not skip calling pthread_cond_destroy() and other
> destruction functions. Doing so is not standards-conforming, in
> particular if you can ever reuse the memory for something else. Not
> calling pthread_cond_destroy() will be broken when using current glibc,
> for example.
Yeah, I was a bit lazy in the error-handling case. Patch attached.
Bruno
[1]
http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_destroy.html
>From 5614c5c9b577cb4c631401cb80ddafaea44a769e Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 6 Jan 2017 18:30:40 +0100
Subject: [PATCH 1/3] lock: Fix data races.
* m4/lock.m4 (gl_PREREQ_LOCK): Test for <stdatomic.h>.
* lib/glthread/lock.c (atomic_thread_fence): Define as a dummy if
<stdatomic.h> is not available.
(glthread_rwlock_init_multithreaded,
glthread_rwlock_rdlock_multithreaded,
glthread_rwlock_wrlock_multithreaded,
glthread_rwlock_wrlock_multithreaded,
glthread_rwlock_destroy_multithreaded): Use atomic_thread_fence to
guarantee memory reads actually see the effects of previous memory
writes in multiprocessor systems.
(glthread_recursive_lock_init_multithreaded,
glthread_recursive_lock_lock_multithreaded,
glthread_recursive_lock_unlock_multithreaded,
glthread_recursive_lock_destroy_multithreaded): Likewise.
(glthread_once_multithreaded): LIkewise.
Reported by Torvald Riegel <trie...@redhat.com>.
---
ChangeLog | 20 ++++++++++++++
lib/glthread/lock.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++
m4/lock.m4 | 6 ++--
3 files changed, 103 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index ebd491f..77f7c2f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,23 @@
+2017-01-06 Bruno Haible <br...@clisp.org>
+
+ lock: Fix data races.
+ * m4/lock.m4 (gl_PREREQ_LOCK): Test for <stdatomic.h>.
+ * lib/glthread/lock.c (atomic_thread_fence): Define as a dummy if
+ <stdatomic.h> is not available.
+ (glthread_rwlock_init_multithreaded,
+ glthread_rwlock_rdlock_multithreaded,
+ glthread_rwlock_wrlock_multithreaded,
+ glthread_rwlock_wrlock_multithreaded,
+ glthread_rwlock_destroy_multithreaded): Use atomic_thread_fence to
+ guarantee memory reads actually see the effects of previous memory
+ writes in multiprocessor systems.
+ (glthread_recursive_lock_init_multithreaded,
+ glthread_recursive_lock_lock_multithreaded,
+ glthread_recursive_lock_unlock_multithreaded,
+ glthread_recursive_lock_destroy_multithreaded): Likewise.
+ (glthread_once_multithreaded): LIkewise.
+ Reported by Torvald Riegel <trie...@redhat.com>.
+
2017-01-05 Pádraig Brady <p...@draigbrady.com>
parse-datetime: fix generated paths for coverage files
diff --git a/lib/glthread/lock.c b/lib/glthread/lock.c
index 061562b..eceb2c3 100644
--- a/lib/glthread/lock.c
+++ b/lib/glthread/lock.c
@@ -22,6 +22,16 @@
#include "glthread/lock.h"
+/* Use <stdatomic.h> to avoid data races in multiprocessor systems.
+ Does not affect x86 and x86_64 systems. */
+#if HAVE_STDATOMIC_H
+# include <stdatomic.h>
+#else
+/* On multiprocessor systems which don't have <stdatomic.h> and which are
+ not x86 or x86_64, you have to cross your fingers. */
+# define atomic_thread_fence(mo) /* nothing! */
+#endif
+
/* ========================================================================= */
#if USE_POSIX_THREADS
@@ -68,16 +78,26 @@ glthread_rwlock_init_multithreaded (gl_rwlock_t *lock)
{
int err;
+ /* Ensure changes to *lock by a previous glthread_rwlock_destroy_multithreaded
+ invocation on another processor have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
err = pthread_rwlock_init (&lock->rwlock, NULL);
if (err != 0)
return err;
lock->initialized = 1;
+ /* Ensure the changes to *lock are pushed to other processors in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_release);
return 0;
}
int
glthread_rwlock_rdlock_multithreaded (gl_rwlock_t *lock)
{
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
{
int err;
@@ -104,6 +124,9 @@ glthread_rwlock_rdlock_multithreaded (gl_rwlock_t *lock)
int
glthread_rwlock_wrlock_multithreaded (gl_rwlock_t *lock)
{
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
{
int err;
@@ -130,6 +153,9 @@ glthread_rwlock_wrlock_multithreaded (gl_rwlock_t *lock)
int
glthread_rwlock_unlock_multithreaded (gl_rwlock_t *lock)
{
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
return EINVAL;
return pthread_rwlock_unlock (&lock->rwlock);
@@ -140,12 +166,18 @@ glthread_rwlock_destroy_multithreaded (gl_rwlock_t *lock)
{
int err;
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
return EINVAL;
err = pthread_rwlock_destroy (&lock->rwlock);
if (err != 0)
return err;
lock->initialized = 0;
+ /* Ensure to not disturb future calls to glthread_rwlock_init_multithreaded
+ on other processors in a multiprocessor system. */
+ atomic_thread_fence (memory_order_release);
return 0;
}
@@ -342,6 +374,11 @@ glthread_recursive_lock_init_multithreaded (gl_recursive_lock_t *lock)
pthread_mutexattr_t attributes;
int err;
+ /* Ensure changes to *lock by a previous
+ glthread_recursive_lock_destroy_multithreaded invocation on another
+ processor have been received by this processor in a multiprocessor
+ system. */
+ atomic_thread_fence (memory_order_acquire);
err = pthread_mutexattr_init (&attributes);
if (err != 0)
return err;
@@ -361,12 +398,18 @@ glthread_recursive_lock_init_multithreaded (gl_recursive_lock_t *lock)
if (err != 0)
return err;
lock->initialized = 1;
+ /* Ensure the changes to *lock are pushed to other processors in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_release);
return 0;
}
int
glthread_recursive_lock_lock_multithreaded (gl_recursive_lock_t *lock)
{
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
{
int err;
@@ -393,6 +436,9 @@ glthread_recursive_lock_lock_multithreaded (gl_recursive_lock_t *lock)
int
glthread_recursive_lock_unlock_multithreaded (gl_recursive_lock_t *lock)
{
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
return EINVAL;
return pthread_mutex_unlock (&lock->recmutex);
@@ -403,12 +449,19 @@ glthread_recursive_lock_destroy_multithreaded (gl_recursive_lock_t *lock)
{
int err;
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
return EINVAL;
err = pthread_mutex_destroy (&lock->recmutex);
if (err != 0)
return err;
lock->initialized = 0;
+ /* Ensure to not disturb future calls to
+ glthread_recursive_lock_init_multithreaded on other processors in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_release);
return 0;
}
@@ -513,6 +566,10 @@ glthread_once_singlethreaded (pthread_once_t *once_control)
int
glthread_rwlock_init_multithreaded (gl_rwlock_t *lock)
{
+ /* Ensure changes to *lock by a previous glthread_rwlock_destroy_multithreaded
+ invocation on another processor have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!pth_mutex_init (&lock->lock))
return errno;
if (!pth_cond_init (&lock->waiting_readers))
@@ -522,12 +579,18 @@ glthread_rwlock_init_multithreaded (gl_rwlock_t *lock)
lock->waiting_writers_count = 0;
lock->runcount = 0;
lock->initialized = 1;
+ /* Ensure the changes to *lock are pushed to other processors in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_release);
return 0;
}
int
glthread_rwlock_rdlock_multithreaded (gl_rwlock_t *lock)
{
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
glthread_rwlock_init_multithreaded (lock);
if (!pth_mutex_acquire (&lock->lock, 0, NULL))
@@ -554,6 +617,9 @@ glthread_rwlock_rdlock_multithreaded (gl_rwlock_t *lock)
int
glthread_rwlock_wrlock_multithreaded (gl_rwlock_t *lock)
{
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
glthread_rwlock_init_multithreaded (lock);
if (!pth_mutex_acquire (&lock->lock, 0, NULL))
@@ -582,6 +648,9 @@ glthread_rwlock_unlock_multithreaded (gl_rwlock_t *lock)
{
int err;
+ /* Ensure other changes to *lock have been received by this processor in a
+ multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!lock->initialized)
return EINVAL;
if (!pth_mutex_acquire (&lock->lock, 0, NULL))
@@ -638,6 +707,9 @@ int
glthread_rwlock_destroy_multithreaded (gl_rwlock_t *lock)
{
lock->initialized = 0;
+ /* Ensure to not disturb future calls to glthread_rwlock_init_multithreaded
+ on other processors in a multiprocessor system. */
+ atomic_thread_fence (memory_order_release);
return 0;
}
@@ -753,6 +825,9 @@ glthread_recursive_lock_destroy_multithreaded (gl_recursive_lock_t *lock)
int
glthread_once_multithreaded (gl_once_t *once_control, void (*initfunction) (void))
{
+ /* Ensure other changes to *once_control have been received by this processor
+ in a multiprocessor system. */
+ atomic_thread_fence (memory_order_acquire);
if (!once_control->inited)
{
int err;
@@ -762,10 +837,14 @@ glthread_once_multithreaded (gl_once_t *once_control, void (*initfunction) (void
err = mutex_lock (&once_control->mutex);
if (err != 0)
return err;
+ atomic_thread_fence (memory_order_acquire);
if (!once_control->inited)
{
once_control->inited = 1;
initfunction ();
+ /* Ensure the changes to *once_control are pushed to other processors
+ in a multiprocessor system. */
+ atomic_thread_fence (memory_order_release);
}
return mutex_unlock (&once_control->mutex);
}
diff --git a/m4/lock.m4 b/m4/lock.m4
index cb04a67..720f9b2 100644
--- a/m4/lock.m4
+++ b/m4/lock.m4
@@ -1,4 +1,4 @@
-# lock.m4 serial 14
+# lock.m4 serial 15
dnl Copyright (C) 2005-2017 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -44,4 +44,6 @@ return !x;
])
# Prerequisites of lib/glthread/lock.c.
-AC_DEFUN([gl_PREREQ_LOCK], [:])
+AC_DEFUN([gl_PREREQ_LOCK], [
+ AC_CHECK_HEADERS_ONCE([stdatomic.h])
+])
--
2.6.4
>From eb451fc68ab7087c4eff1346656b8266ee911858 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 6 Jan 2017 18:35:05 +0100
Subject: [PATCH 2/3] lock: Improve destruction.
* lib/glthread/lock.c (glthread_rwlock_destroy_multithreaded): Destroy
the wait queues before the lock.
Suggested by Torvald Riegel <trie...@redhat.com>.
---
ChangeLog | 7 +++++++
lib/glthread/lock.c | 6 +++---
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 77f7c2f..c5f410f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
2017-01-06 Bruno Haible <br...@clisp.org>
+ lock: Improve destruction.
+ * lib/glthread/lock.c (glthread_rwlock_destroy_multithreaded): Destroy
+ the wait queues before the lock.
+ Suggested by Torvald Riegel <trie...@redhat.com>.
+
+2017-01-06 Bruno Haible <br...@clisp.org>
+
lock: Fix data races.
* m4/lock.m4 (gl_PREREQ_LOCK): Test for <stdatomic.h>.
* lib/glthread/lock.c (atomic_thread_fence): Define as a dummy if
diff --git a/lib/glthread/lock.c b/lib/glthread/lock.c
index eceb2c3..b0f78f4 100644
--- a/lib/glthread/lock.c
+++ b/lib/glthread/lock.c
@@ -319,15 +319,15 @@ glthread_rwlock_destroy_multithreaded (gl_rwlock_t *lock)
{
int err;
- err = pthread_mutex_destroy (&lock->lock);
- if (err != 0)
- return err;
err = pthread_cond_destroy (&lock->waiting_readers);
if (err != 0)
return err;
err = pthread_cond_destroy (&lock->waiting_writers);
if (err != 0)
return err;
+ err = pthread_mutex_destroy (&lock->lock);
+ if (err != 0)
+ return err;
return 0;
}
--
2.6.4
>From 025fe623db26a1c963e8fbbbf3f8a2434861f101 Mon Sep 17 00:00:00 2001
From: Bruno Haible <br...@clisp.org>
Date: Fri, 6 Jan 2017 18:49:54 +0100
Subject: [PATCH 3/3] lock: Fix resource deallocation upon failure.
* lib/glthread/lock.c (glthread_rwlock_init_multithreaded): Destroy
already allocated objects upon failure.
(glthread_rwlock_unlock_multithreaded): Attempt to destroy all
subobjects even if not all destructions succeed.
Reported by Torvald Riegel <trie...@redhat.com>.
---
ChangeLog | 9 +++++++++
lib/glthread/lock.c | 26 +++++++++++++++++---------
2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index c5f410f..1c0f60a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
2017-01-06 Bruno Haible <br...@clisp.org>
+ lock: Fix resource deallocation upon failure.
+ * lib/glthread/lock.c (glthread_rwlock_init_multithreaded): Destroy
+ already allocated objects upon failure.
+ (glthread_rwlock_unlock_multithreaded): Attempt to destroy all
+ subobjects even if not all destructions succeed.
+ Reported by Torvald Riegel <trie...@redhat.com>.
+
+2017-01-06 Bruno Haible <br...@clisp.org>
+
lock: Improve destruction.
* lib/glthread/lock.c (glthread_rwlock_destroy_multithreaded): Destroy
the wait queues before the lock.
diff --git a/lib/glthread/lock.c b/lib/glthread/lock.c
index b0f78f4..d765cd3 100644
--- a/lib/glthread/lock.c
+++ b/lib/glthread/lock.c
@@ -195,10 +195,17 @@ glthread_rwlock_init_multithreaded (gl_rwlock_t *lock)
return err;
err = pthread_cond_init (&lock->waiting_readers, NULL);
if (err != 0)
- return err;
+ {
+ pthread_mutex_destroy (&lock->lock);
+ return err;
+ }
err = pthread_cond_init (&lock->waiting_writers, NULL);
if (err != 0)
- return err;
+ {
+ pthread_cond_destroy (&lock->waiting_readers);
+ pthread_mutex_destroy (&lock->lock);
+ return err;
+ }
lock->waiting_writers_count = 0;
lock->runcount = 0;
return 0;
@@ -317,18 +324,19 @@ glthread_rwlock_unlock_multithreaded (gl_rwlock_t *lock)
int
glthread_rwlock_destroy_multithreaded (gl_rwlock_t *lock)
{
+ int ret = 0;
int err;
err = pthread_cond_destroy (&lock->waiting_readers);
- if (err != 0)
- return err;
+ if (err != 0 && ret == 0)
+ ret = err;
err = pthread_cond_destroy (&lock->waiting_writers);
- if (err != 0)
- return err;
+ if (err != 0 && ret == 0)
+ ret = err;
err = pthread_mutex_destroy (&lock->lock);
- if (err != 0)
- return err;
- return 0;
+ if (err != 0 && ret == 0)
+ ret = err;
+ return ret;
}
# endif
--
2.6.4
