[SSSD] [PATCH] Release mutex if a thread gets cancelled

Jakub Hrozek jhrozek at redhat.com
Fri Aug 10 16:40:35 UTC 2012


https://fedorahosted.org/sssd/ticket/1460

Please see the commit. I'm wondering if there is still a (small) race
condition between the call to pthread_cleanup_pop() and unlocking the
mutex. Would it be better to i.e. always call the cleanup handler with
pthread_cleanup_pop(1) and disconnect from the fd based on some other
condition?
-------------- next part --------------
>From 460aa7faebb60b5c7d5f3d78f90640b39f999756 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Fri, 10 Aug 2012 17:02:26 +0200
Subject: [PATCH] Release mutex if a thread gets cancelled

Our code in the client calls several thread cancellation points after a
mutex is locked. If the thread gets cancelled after the mutex has been
acquired, it is never released, causing a deadlock.

This patch adds a cleanup routine registered with pthread_cleanup_push,
which would be invoked on thread cancelation. If the thread is canceled,
the cleanup routine releases the mutex and closes the connection to the
responder.

https://fedorahosted.org/sssd/ticket/1460
---
 Makefile.am              |  8 ++++++-
 src/sss_client/common.c  | 26 ++++++++++++++++-------
 src/sss_client/sss_cli.h | 54 +++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index e89938ecaff3d40faa50ffafcf88ea3dda1bacca..e218b440628d08fa743eeba290b63d6dc9a14a81 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -738,6 +738,7 @@ sss_sudo_cli_SOURCES = \
     src/sss_client/common.c \
     src/sss_client/sudo_testcli/sudo_testcli.c
 sss_sudo_cli_CFLAGS = $(AM_CFLAGS)
+sss_sudo_cli_LDFLAGS = -lpthread
 sss_sudo_cli_LDADD = \
     libsss_sudo.la
 endif
@@ -1121,7 +1122,7 @@ autofs_test_client_SOURCES = src/sss_client/autofs/autofs_test_client.c \
 			     src/sss_client/autofs/sss_autofs.c \
 			     src/sss_client/common.c
 autofs_test_client_CFLAGS = $(AM_CFLAGS)
-autofs_test_client_LDFLAGS = -lpopt
+autofs_test_client_LDFLAGS = -lpopt -lpthread
 endif
 
 ####################
@@ -1143,6 +1144,7 @@ libnss_sss_la_SOURCES = \
     src/sss_client/nss_mc_group.c \
     src/sss_client/nss_mc.h
 libnss_sss_la_LDFLAGS = \
+    -lpthread \
     -module \
     -version-info 2:0:0 \
     -Wl,--version-script,$(srcdir)/src/sss_client/sss_nss.exports
@@ -1156,6 +1158,7 @@ pam_sss_la_SOURCES = \
     src/sss_client/sss_pam_macros.h
 
 pam_sss_la_LDFLAGS = \
+    -lpthread \
     -lpam \
     -module \
     -avoid-version \
@@ -1171,6 +1174,7 @@ libsss_sudo_la_SOURCES = \
     src/sss_client/sudo/sss_sudo.h \
     src/sss_client/sudo/sss_sudo_private.h
 libsss_sudo_la_LDFLAGS = \
+    -lpthread \
     -Wl,--version-script,$(srcdir)/src/sss_client/sss_sudo.exports \
     -version-info 2:0:1
 
@@ -1190,6 +1194,7 @@ libsss_autofs_la_SOURCES = \
     src/sss_client/autofs/sss_autofs_private.h
 
 libsss_autofs_la_LDFLAGS = \
+    -lpthread \
     -module \
     -avoid-version \
     -Wl,--version-script,$(srcdir)/src/sss_client/autofs/sss_autofs.exports
@@ -1508,6 +1513,7 @@ sssd_pac_plugin_la_CFLAGS = \
     $(AM_CFLAGS) \
     $(KRB5_CFLAGS)
 sssd_pac_plugin_la_LDFLAGS = \
+    -lpthread \
     -lkrb5 \
     -avoid-version \
     -module
diff --git a/src/sss_client/common.c b/src/sss_client/common.c
index 59eae3b1fe4101383de96f680458467a962c2248..870122e1bbe0f22af55cc7897367ea97b6a42711 100644
--- a/src/sss_client/common.c
+++ b/src/sss_client/common.c
@@ -979,19 +979,29 @@ errno_t sss_strnlen(const char *str, size_t maxlen, size_t *len)
 static pthread_mutex_t sss_nss_mutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_mutex_t sss_pam_mutex = PTHREAD_MUTEX_INITIALIZER;
 
-void sss_nss_lock(void)
+void sss_nss_cleanup(void *pvt)
+{
+    sss_cli_close_socket();
+    _sss_nss_unlock();
+}
+void _sss_nss_lock(void)
 {
     pthread_mutex_lock(&sss_nss_mutex);
 }
-void sss_nss_unlock(void)
+void _sss_nss_unlock(void)
 {
     pthread_mutex_unlock(&sss_nss_mutex);
 }
-void sss_pam_lock(void)
+void sss_pam_cleanup(void *pvt)
+{
+    sss_cli_close_socket();
+    _sss_pam_unlock();
+}
+void _sss_pam_lock(void)
 {
     pthread_mutex_lock(&sss_pam_mutex);
 }
-void sss_pam_unlock(void)
+void _sss_pam_unlock(void)
 {
     pthread_mutex_unlock(&sss_pam_mutex);
 }
@@ -999,10 +1009,10 @@ void sss_pam_unlock(void)
 #else
 
 /* sorry no mutexes available */
-void sss_nss_lock(void) { return; }
-void sss_nss_unlock(void) { return; }
-void sss_pam_lock(void) { return; }
-void sss_pam_unlock(void) { return; }
+void _sss_nss_lock(void) { return; }
+void _sss_nss_unlock(void) { return; }
+void _sss_pam_lock(void) { return; }
+void _sss_pam_unlock(void) { return; }
 #endif
 
 
diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h
index f60bd99121dd45f7811699da64eec8b106b20bf3..f6e2b8a4b922c4081f97edca1e0add6ce916a80a 100644
--- a/src/sss_client/sss_cli.h
+++ b/src/sss_client/sss_cli.h
@@ -32,12 +32,16 @@
 #include <stdint.h>
 #include <limits.h>
 
+#include "config.h"
+#if HAVE_PTHREAD
+#include <pthread.h>
+#endif
+
 #ifndef HAVE_ERRNO_T
 #define HAVE_ERRNO_T
 typedef int errno_t;
 #endif
 
-
 #ifndef EOK
 #define EOK 0
 #endif
@@ -563,10 +567,50 @@ safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter)
  */
 errno_t sss_strnlen(const char *str, size_t maxlen, size_t *len);
 
-void sss_nss_lock(void);
-void sss_nss_unlock(void);
-void sss_pam_lock(void);
-void sss_pam_unlock(void);
+void _sss_nss_lock(void);
+void _sss_nss_unlock(void);
+void _sss_pam_lock(void);
+void _sss_pam_unlock(void);
+
+void sss_nss_cleanup(void *pvt);
+void sss_pam_cleanup(void *pvt);
+
+/*
+ * pthread_cleanup_push is a macro that must be used
+ * on the same indentation level as pthread_cleanup_pop
+ * so we can't wrap it in a do-while(0) loop
+ */
+#if HAVE_PTHREAD
+#define sss_nss_lock()                              \
+    pthread_cleanup_push(sss_nss_cleanup, NULL);    \
+    _sss_nss_lock()
+#else
+#define sss_nss_lock() _sss_nss_lock()
+#endif
+
+#if HAVE_PTHREAD
+#define sss_nss_unlock()                    \
+    pthread_cleanup_pop(0);                 \
+    _sss_nss_unlock()
+#else
+#define sss_nss_cleanup_unlock() _sss_nss_unlock()
+#endif
+
+#if HAVE_PTHREAD
+#define sss_pam_lock()                              \
+    pthread_cleanup_push(sss_pam_cleanup, NULL);    \
+    _sss_pam_lock()
+#else
+#define sss_pam_lock() _sss_pam_lock()
+#endif
+
+#if HAVE_PTHREAD
+#define sss_pam_unlock()                    \
+    pthread_cleanup_pop(0);                 \
+    _sss_pam_unlock()
+#else
+#define sss_pam_cleanup_unlock() _sss_pam_unlock()
+#endif
 
 errno_t sss_readrep_copy_string(const char *in,
                                 size_t *offset,
-- 
1.7.11.2



More information about the sssd-devel mailing list