>From 460aa7faebb60b5c7d5f3d78f90640b39f999756 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek 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 #include +#include "config.h" +#if HAVE_PTHREAD +#include +#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