Race condition in tdb_runtime_check_for_robust_mutexes()
Ralph Boehme
slow at samba.org
Tue Mar 29 12:05:20 UTC 2016
On Tue, Mar 29, 2016 at 01:43:08PM +0300, Uri Simchoni wrote:
> On 03/29/2016 01:41 PM, Ralph Boehme wrote:
> > On Tue, Mar 29, 2016 at 01:22:35PM +0300, Uri Simchoni wrote:
> >> On 03/29/2016 01:03 PM, Uri Simchoni wrote:
> >>> On 03/29/2016 12:52 PM, Uri Simchoni wrote:
> >>>> Pushed with my RB+
> >>>
> >>> ..once again with the removed unused status :)
> >>>
> >>>
> >> OK this is becoming embarrassing.
> >>
> >> Please review the attached - jumping to cleanup_m instead of cleanup_ma
> >> after pthread_sigmask().
> >
> > thanks for spotting this! Pushed.
> >
> > I was already seriously considering adding a second patch that changes
> > the cleanup logic to use state flags instead of gotos, eg
> >
> > bool cleanup_mutex = false;
> >
> > ret = pthread_mutex_init(...);
> > if (ret != 0) {
> > goto cleanup;
> > }
> > cleanup_mutex = true;
> >
> > ...
> >
> > cleanup:
> > if (cleanup_mutex) {
> > ...cleanup mutex...
> > }
> >
> > Cheerio!
> > -slow
>
> Seems like the thing we should do, considering the empirical evidence...
attached. Not sure if we really want this.
Cheerio!
-slow
-------------- next part --------------
From dfc8bc0957d3ec6c9e39dd6a3c7199b0fee90c73 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 29 Mar 2016 13:08:20 +0200
Subject: [PATCH] tdb: rework cleanup logic in
tdb_runtime_check_for_robust_mutexes()
The cleanup logic used six goto lables, at least I'm not able to make
sane modifications to such a beast.
By using state flags that track which objects are initialized and need
cleanup, we get rid of the goto labels. It comes at a cost though: you
have to be careful to correctly set the cleanup flags.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
lib/tdb/common/mutex.c | 101 +++++++++++++++++++++++++++++--------------------
1 file changed, 61 insertions(+), 40 deletions(-)
diff --git a/lib/tdb/common/mutex.c b/lib/tdb/common/mutex.c
index e57031d..03ee9bb 100644
--- a/lib/tdb/common/mutex.c
+++ b/lib/tdb/common/mutex.c
@@ -766,7 +766,7 @@ static void tdb_robust_mutex_handler(int sig)
_PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
{
- void *ptr;
+ void *ptr = NULL;
pthread_mutex_t *m;
pthread_mutexattr_t ma;
int ret = 1;
@@ -777,6 +777,11 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
bool ok;
static bool initialized;
sigset_t mask, old_mask, suspend_mask;
+ bool cleanup_ma = false;
+ bool cleanup_m = false;
+ bool cleanup_sigmask = false;
+ bool cleanup_sig_handler = false;
+ bool cleanup_child = false;
if (initialized) {
return tdb_mutex_locking_cached;
@@ -800,33 +805,37 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
ret = pipe(pipe_down);
if (ret != 0) {
- goto cleanup_mmap;
+ goto cleanup;
}
ret = pipe(pipe_up);
if (ret != 0) {
- goto cleanup_pipe;
+ goto cleanup;
}
ret = pthread_mutexattr_init(&ma);
if (ret != 0) {
- goto cleanup_pipe;
+ goto cleanup;
}
+ cleanup_ma = true;
+
ret = pthread_mutexattr_settype(&ma, PTHREAD_MUTEX_ERRORCHECK);
if (ret != 0) {
- goto cleanup_ma;
+ goto cleanup;
}
ret = pthread_mutexattr_setpshared(&ma, PTHREAD_PROCESS_SHARED);
if (ret != 0) {
- goto cleanup_ma;
+ goto cleanup;
}
ret = pthread_mutexattr_setrobust(&ma, PTHREAD_MUTEX_ROBUST);
if (ret != 0) {
- goto cleanup_ma;
+ goto cleanup;
}
+
ret = pthread_mutex_init(m, &ma);
if (ret != 0) {
- goto cleanup_ma;
+ goto cleanup;
}
+ cleanup_m = true;
/*
* Block SIGCHLD so we can atomically wait for it later with
@@ -836,15 +845,17 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
sigaddset(&mask, SIGCHLD);
ret = pthread_sigmask(SIG_BLOCK, &mask, &old_mask);
if (ret != 0) {
- goto cleanup_m;
+ 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_sigmask;
+ goto cleanup;
}
+ cleanup_sig_handler = true;
tdb_robust_mutex_pid = fork();
if (tdb_robust_mutex_pid == 0) {
@@ -867,8 +878,10 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
_exit(0);
}
if (tdb_robust_mutex_pid == -1) {
- goto cleanup_sig_child;
+ goto cleanup;
}
+ cleanup_child = true;
+
close(pipe_down[0]);
pipe_down[0] = -1;
close(pipe_up[1]);
@@ -876,7 +889,7 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
nread = read(pipe_up[0], &ret, sizeof(ret));
if (nread != sizeof(ret)) {
- goto cleanup_child;
+ goto cleanup;
}
ret = pthread_mutex_trylock(m);
@@ -884,16 +897,16 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
if (ret == 0) {
pthread_mutex_unlock(m);
}
- goto cleanup_child;
+ goto cleanup;
}
if (write(pipe_down[1], &c, 1) != 1) {
- goto cleanup_child;
+ goto cleanup;
}
nread = read(pipe_up[0], &c, 1);
if (nread != 0) {
- goto cleanup_child;
+ goto cleanup;
}
while (tdb_robust_mutex_pid > 0) {
@@ -902,56 +915,62 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
abort();
}
}
- tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
+ cleanup_child = false;
ret = pthread_mutex_trylock(m);
if (ret != EOWNERDEAD) {
if (ret == 0) {
pthread_mutex_unlock(m);
}
- goto cleanup_sigmask;
+ goto cleanup;
}
ret = pthread_mutex_consistent(m);
if (ret != 0) {
- goto cleanup_sigmask;
+ goto cleanup;
}
ret = pthread_mutex_trylock(m);
if (ret != EDEADLK) {
pthread_mutex_unlock(m);
- goto cleanup_sigmask;
+ goto cleanup;
}
ret = pthread_mutex_unlock(m);
if (ret != 0) {
- goto cleanup_sigmask;
+ goto cleanup;
}
tdb_mutex_locking_cached = true;
- goto cleanup_sigmask;
-cleanup_child:
- while (tdb_robust_mutex_pid > 0) {
- kill(tdb_robust_mutex_pid, SIGKILL);
- ret = sigsuspend(&suspend_mask);
- if (ret != -1 || errno != EINTR) {
- abort();
+cleanup:
+ if (cleanup_child) {
+ while (tdb_robust_mutex_pid > 0) {
+ kill(tdb_robust_mutex_pid, SIGKILL);
+ ret = sigsuspend(&suspend_mask);
+ if (ret != -1 || errno != EINTR) {
+ abort();
+ }
}
}
-cleanup_sig_child:
- tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
-cleanup_sigmask:
- ret = pthread_sigmask(SIG_SETMASK, &old_mask, NULL);
- if (ret != 0) {
- abort();
+ if (cleanup_sig_handler) {
+ tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler,
+ NULL);
}
-cleanup_m:
- pthread_mutex_destroy(m);
-cleanup_ma:
- pthread_mutexattr_destroy(&ma);
-cleanup_pipe:
+ if (cleanup_sigmask) {
+ ret = pthread_sigmask(SIG_SETMASK, &old_mask, NULL);
+ if (ret != 0) {
+ abort();
+ }
+ }
+ if (cleanup_m) {
+ pthread_mutex_destroy(m);
+ }
+ if (cleanup_ma) {
+ pthread_mutexattr_destroy(&ma);
+ }
+
if (pipe_down[0] != -1) {
close(pipe_down[0]);
}
@@ -964,8 +983,10 @@ cleanup_pipe:
if (pipe_up[1] != -1) {
close(pipe_up[1]);
}
-cleanup_mmap:
- munmap(ptr, sizeof(pthread_mutex_t));
+
+ if (ptr != NULL) {
+ munmap(ptr, sizeof(pthread_mutex_t));
+ }
return tdb_mutex_locking_cached;
}
--
2.5.0
More information about the samba-technical
mailing list