[PATCH] tdb: runtime check for robust mutexes may hang
Ralph Böhme
slow at samba.org
Tue Mar 14 15:33:00 UTC 2017
On Mon, Mar 13, 2017 at 10:57:50AM +0100, Ralph Böhme wrote:
> On Mon, Mar 13, 2017 at 06:49:00AM +1300, Andrew Bartlett wrote:
> > On Sun, 2017-03-12 at 14:14 +0100, Ralph Böhme via samba-technical
> > wrote:
> > >
> > > As there's no POSIX function to change the signal mask of all threads
> > > in a
> > > process, this problem can't be solved by a change to
> > > tdb_runtime_check_for_robust_mutexes().
> > >
> > > Attached patch *moves* the runtime check to a library initializer
> > > which
> > > guarantees that only one thread exists in the process when the
> > > function is run.
> >
> > Why can we assert this is true?
> >
> > Surely if tdb is loaded into a program by being linked to a plugin, eg
> > dlopen(), the library initializer will run then, with threads already
> > running?
>
> golly, you're right. I missed this case.
>
> Back to the drawing board I guess, unless we decide we don't care about this use
> case. We probably should care, might actually be used by the Gnome VFS, who
> knows.
attached patch uses a different approach, essentially going back to the previous
version of using waitpid() to rendezvous with the child, but fixing the toctou
race(s).
Thoughts? Uri (or whoever's interested), can you please take a look? I'd like as
many eyes as possible on this one. Thanks!
Cheerio!
-slow
-------------- next part --------------
From dda0423662daaa2e49796cf5ed2a556b7460bc9a Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 14 Mar 2017 14:24:18 +0100
Subject: [PATCH] lib/tdb: runtime check for robust mutexes may hang in
threaded programs
The current runtime check for robust mutexes in
tdb_runtime_check_for_robust_mutexes() is not thread-safe.
When called in a multi-threaded program where any another thread doesn't
have SIGCHLD blocked, we may end up hung in sigsuspend() waiting for a
SIGCHLD of a child procecss and the signal was delivered to another
thread.
Revert to the previous behaviour of waiting for the child instead of
waiting for the SIGCHLD signal.
Ensure the pid we wait for is not reset to -1 in a toctou race with the
signal handler.
Check whether waitpid() returns ECHILD which can happen if the signal
handler is run by more then one thread in parallel (yes, this can
happen) or if tdb_robust_mutex_wait_for_child() and the signal handler
are racing.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=12593
Pair-programmed-with: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
lib/tdb/common/mutex.c | 116 +++++++++++++++++++++++++++++--------------------
1 file changed, 70 insertions(+), 46 deletions(-)
diff --git a/lib/tdb/common/mutex.c b/lib/tdb/common/mutex.c
index cac3916..8a122d5 100644
--- a/lib/tdb/common/mutex.c
+++ b/lib/tdb/common/mutex.c
@@ -752,12 +752,23 @@ static bool tdb_robust_mutex_setup_sigchild(void (*handler)(int),
static void tdb_robust_mutex_handler(int sig)
{
- if (tdb_robust_mutex_pid != -1) {
+ pid_t child_pid = tdb_robust_mutex_pid;
+
+ if (child_pid != -1) {
pid_t pid;
- int status;
- pid = waitpid(tdb_robust_mutex_pid, &status, WNOHANG);
- if (pid == tdb_robust_mutex_pid) {
+ pid = waitpid(child_pid, NULL, WNOHANG);
+ if (pid == -1) {
+ switch (errno) {
+ case ECHILD:
+ tdb_robust_mutex_pid = -1;
+ return;
+
+ default:
+ return;
+ }
+ }
+ if (pid == child_pid) {
tdb_robust_mutex_pid = -1;
return;
}
@@ -776,6 +787,44 @@ static void tdb_robust_mutex_handler(int sig)
tdb_robust_mutext_old_handler(sig);
}
+static void tdb_robust_mutex_wait_for_child(pid_t *child_pid)
+{
+ int options = WNOHANG;
+
+ if (*child_pid == -1) {
+ return;
+ }
+
+ while (tdb_robust_mutex_pid > 0) {
+ pid_t pid;
+
+ /*
+ * First we try with WNOHANG, as the process might not exist
+ * anymore. Once we've sent SIGKILL we block waiting for the
+ * exit.
+ */
+ pid = waitpid(*child_pid, NULL, options);
+ if (pid == -1) {
+ if (errno == EINTR) {
+ continue;
+ } else if (errno == ECHILD) {
+ break;
+ } else {
+ abort();
+ }
+ }
+ if (pid == *child_pid) {
+ break;
+ }
+
+ kill(*child_pid, SIGKILL);
+ options = 0;
+ }
+
+ tdb_robust_mutex_pid = -1;
+ *child_pid = -1;
+}
+
_PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
{
void *ptr = NULL;
@@ -788,9 +837,8 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
char c = 0;
bool ok;
static bool initialized;
- sigset_t mask, old_mask, suspend_mask;
+ pid_t saved_child_pid = -1;
bool cleanup_ma = false;
- bool cleanup_sigmask = false;
if (initialized) {
return tdb_mutex_locking_cached;
@@ -798,8 +846,6 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
initialized = true;
- sigemptyset(&suspend_mask);
-
ok = tdb_mutex_locking_supported();
if (!ok) {
return false;
@@ -845,26 +891,13 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
}
m = (pthread_mutex_t *)ptr;
- /*
- * Block SIGCHLD so we can atomically wait for it later with
- * sigsuspend()
- */
- sigemptyset(&mask);
- sigaddset(&mask, SIGCHLD);
- ret = pthread_sigmask(SIG_BLOCK, &mask, &old_mask);
- if (ret != 0) {
- goto cleanup;
- }
- cleanup_sigmask = true;
- suspend_mask = old_mask;
- sigdelset(&suspend_mask, SIGCHLD);
-
if (tdb_robust_mutex_setup_sigchild(tdb_robust_mutex_handler,
&tdb_robust_mutext_old_handler) == false) {
goto cleanup;
}
tdb_robust_mutex_pid = fork();
+ saved_child_pid = tdb_robust_mutex_pid;
if (tdb_robust_mutex_pid == 0) {
size_t nwritten;
close(pipe_down[1]);
@@ -914,14 +947,7 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
goto cleanup;
}
- while (tdb_robust_mutex_pid > 0) {
- ret = sigsuspend(&suspend_mask);
- if (ret != -1 || errno != EINTR) {
- abort();
- }
- }
- tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
- tdb_robust_mutext_old_handler = SIG_ERR;
+ tdb_robust_mutex_wait_for_child(&saved_child_pid);
ret = pthread_mutex_trylock(m);
if (ret != EOWNERDEAD) {
@@ -950,23 +976,21 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
tdb_mutex_locking_cached = true;
cleanup:
- while (tdb_robust_mutex_pid > 0) {
- kill(tdb_robust_mutex_pid, SIGKILL);
- ret = sigsuspend(&suspend_mask);
- if (ret != -1 || errno != EINTR) {
- abort();
- }
- }
+ /*
+ * Note that we don't reset the signal handler we just reset
+ * tdb_robust_mutex_pid to -1. This is ok as this code path is only
+ * called once per process.
+ *
+ * Leaving our signal handler avoids races with other threads potentialy
+ * setting up their SIGCHLD handlers.
+ *
+ * The worst thing that can happen is that the other newer signal
+ * handler will get the SIGCHLD signal for our child and/or reap the
+ * child with a wait() function. tdb_robust_mutex_wait_for_child()
+ * handles the case where waitpid returns ECHILD.
+ */
+ tdb_robust_mutex_wait_for_child(&saved_child_pid);
- if (tdb_robust_mutext_old_handler != SIG_ERR) {
- tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
- }
- if (cleanup_sigmask) {
- ret = pthread_sigmask(SIG_SETMASK, &old_mask, NULL);
- if (ret != 0) {
- abort();
- }
- }
if (m != NULL) {
pthread_mutex_destroy(m);
}
--
2.9.3
More information about the samba-technical
mailing list