From 2f990866e80c6b16218d888d1ebf66aebaa0b19d Mon Sep 17 00:00:00 2001 From: Ben Kibbey Date: Sun, 25 Jan 2009 12:08:12 -0500 Subject: [PATCH] Better mutex locking and unlocking macros. Useful for debugging. --- src/Makefile.am | 2 +- src/cache.h | 19 ++----------------- src/commands.c | 25 ++++++++++--------------- src/lock.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/pinentry.c | 56 ++++++++++++++++++++++++++++++++++++++++--------------- src/pwmd.c | 58 +++++++++++++++++++++++++++++++-------------------------- 6 files changed, 143 insertions(+), 74 deletions(-) create mode 100644 src/lock.h diff --git a/src/Makefile.am b/src/Makefile.am index 48dd3aba..094b4b92 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,7 +1,7 @@ bin_PROGRAMS = pwmd pwmd_SOURCES = pwmd.c pwmd.h xml.c xml.h pwmd_error.c pwmd_error.h commands.c \ commands.h common.h cache.c cache.h gettext.h misc.c misc.h \ - status.h + status.h lock.h pwmd_LDFLAGS = @XML_LIBS@ @GLIB_LIBS@ @LIBGCRYPT_LIBS@ @GPG_ERROR_LIBS@ pwmd_CFLAGS = -DLOCALEDIR=\"${prefix}/share/locale\" @XML_CPPFLAGS@ \ @GLIB_CFLAGS@ @LIBGCRYPT_CFLAGS@ @GPG_ERROR_CFLAGS@ diff --git a/src/cache.h b/src/cache.h index c1f4f5d8..32bb383c 100644 --- a/src/cache.h +++ b/src/cache.h @@ -20,23 +20,8 @@ #define CACHE_H #include - -#ifdef DEBUG -#define CACHE_LOCK_DEBUG log_write(N_("%s(%i): LOCK tid=%p"), __FUNCTION__, __LINE__, pthread_self()); -#define CACHE_UNLOCK_DEBUG log_write(N_("%s(%i): UNLOCK tid=%p"), __FUNCTION__, __LINE__, pthread_self()); -#else -#define CACHE_LOCK_DEBUG -#define CACHE_UNLOCK_DEBUG -#endif - -#define CACHE_TRYLOCK(ctx) if (pthread_mutex_trylock(&cache_mutex) == EBUSY) {\ - if (ctx) \ - send_status(ctx, STATUS_LOCKED, NULL); \ - pthread_mutex_lock(&cache_mutex); \ - } - -#define CACHE_LOCK(ctx) CACHE_LOCK_DEBUG CACHE_TRYLOCK(ctx) -#define CACHE_UNLOCK CACHE_UNLOCK_DEBUG pthread_mutex_unlock(&cache_mutex) +#include +#include "status.h" struct file_mutex_s { pthread_mutex_t mutex; diff --git a/src/commands.c b/src/commands.c index b9bf5291..ac19c1ec 100644 --- a/src/commands.c +++ b/src/commands.c @@ -48,6 +48,7 @@ #include "tls.h" #endif #include "commands.h" +#include "lock.h" static void *z_alloc(void *data, unsigned items, unsigned size) { @@ -135,14 +136,13 @@ void unlock_file_mutex(struct client_s *client) } CACHE_UNLOCK; - pthread_mutex_unlock(&m->mutex); + MUTEX_UNLOCK(&m->mutex); client->has_lock = client->is_lock_cmd = FALSE; } gpg_error_t lock_file_mutex(struct client_s *client) { struct file_mutex_s *m; - gint e; if (client->has_lock == TRUE) return 0; @@ -155,22 +155,13 @@ gpg_error_t lock_file_mutex(struct client_s *client) } CACHE_UNLOCK; - e = pthread_mutex_trylock(&m->mutex); - if (e && e != EBUSY) { - log_write("%s(%i): %s", __FUNCTION__, __LINE__, strerror(e)); - return gpg_error_from_errno(e); - } - - if (e == EBUSY) { + if (pthread_mutex_trylock(&m->mutex) == EBUSY) { if (client->ctx) { /* * If a client disconnects unexpectedly while waiting for a * lock, this lets the thread terminate because send_status() - * will return an error. We can't use an PTH_EVENT_FUNC here - * because the thread that would call the callback uses it's - * own stack space which messes up access to the client data - * (if I understand it correctly). + * will return an error. */ while (pthread_mutex_trylock(&m->mutex) == EBUSY) { gpg_error_t rc = send_status(client->ctx, STATUS_LOCKED, NULL); @@ -181,8 +172,12 @@ gpg_error_t lock_file_mutex(struct client_s *client) sleep(1); } } - else - pthread_mutex_lock(&m->mutex); + else { + MUTEX_LOCK(&m->mutex); + } + } + else { + MUTEX_LOCK_DEBUG; } client->has_lock = TRUE; diff --git a/src/lock.h b/src/lock.h new file mode 100644 index 00000000..fd03ae96 --- /dev/null +++ b/src/lock.h @@ -0,0 +1,57 @@ +/* vim:tw=78:ts=8:sw=4:set ft=c: */ +/* + Copyright (C) 2006-2009 Ben Kibbey + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02110-1301 USA +*/ +#ifndef LOCK_H +#define LOCK_H + +#include +#include +#include "cache.h" + +#ifdef DEBUG +#define MUTEX_LOCK_DEBUG \ + log_write("%s(%i): %s: LOCK", __FILE__, __LINE__, __FUNCTION__); +#define MUTEX_UNLOCK_DEBUG \ + log_write("%s(%i): %s: UNLOCK", __FILE__, __LINE__, __FUNCTION__); +#else +#define MUTEX_LOCK_DEBUG +#define MUTEX_UNLOCK_DEBUG +#endif + +#define CACHE_LOCK(ctx) MUTEX_TRYLOCK(ctx, &cache_mutex) +#define CACHE_UNLOCK MUTEX_UNLOCK(&cache_mutex) + +#define MUTEX_LOCK(m) \ + MUTEX_LOCK_DEBUG pthread_mutex_lock(m) + +#define MUTEX_UNLOCK(m) \ + MUTEX_UNLOCK_DEBUG pthread_mutex_unlock(m) + +#define MUTEX_TRYLOCK(ctx, m) \ + if (pthread_mutex_trylock(m) == EBUSY) { \ + if (ctx) \ + send_status(ctx, STATUS_LOCKED, NULL); \ + log_write(N_("%s(%i): %s: mutex is LOCKED"), __FILE__, __LINE__, \ + __FUNCTION__); \ + MUTEX_LOCK(m); \ + } \ + else { \ + MUTEX_LOCK_DEBUG \ + } + +#endif diff --git a/src/pinentry.c b/src/pinentry.c index b0f90b99..0aa40158 100644 --- a/src/pinentry.c +++ b/src/pinentry.c @@ -46,6 +46,7 @@ #include "pinentry.h" #include "misc.h" #include "pwmd_error.h" +#include "lock.h" void free_client_list(); static gpg_error_t set_pinentry_strings(struct pinentry_s *pin, int which); @@ -425,18 +426,18 @@ static void *timeout_thread(void *arg) clock_gettime(CLOCK_REALTIME, &ts); ts.tv_sec += pin->timeout; - pthread_mutex_lock(&pin->cond_mutex); + MUTEX_LOCK(&pin->cond_mutex); pthread_cond_timedwait(&pin->cond, &pin->cond_mutex, &ts); /* pthread_cond_signal() was called from pinentry_iterate() (we have a * key). */ if (pin->status == PINENTRY_NONE) { - pthread_mutex_unlock(&pin->cond_mutex); + MUTEX_UNLOCK(&pin->cond_mutex); pthread_exit(PTHREAD_CANCELED); return NULL; } - pthread_mutex_lock(&pin->status_mutex); + MUTEX_LOCK(&pin->status_mutex); if (kill(pin->pin_pid, 0) == 0) if (kill(pin->pin_pid, SIGTERM) == 0) @@ -444,8 +445,8 @@ static void *timeout_thread(void *arg) kill(pin->pin_pid, SIGKILL); pin->status = PINENTRY_TIMEOUT; - pthread_mutex_unlock(&pin->cond_mutex); - pthread_mutex_unlock(&pin->status_mutex); + MUTEX_UNLOCK(&pin->cond_mutex); + MUTEX_UNLOCK(&pin->status_mutex); pthread_exit(PTHREAD_CANCELED); return NULL; } @@ -511,16 +512,27 @@ gpg_error_t pinentry_fork(assuan_context_t ctx) gpg_error_t lock_pin_mutex(struct client_s *client) { - while (pthread_mutex_trylock(&pin_mutex) == EBUSY) { + if (pthread_mutex_trylock(&pin_mutex) == EBUSY) { if (client->ctx) { - gpg_error_t rc = send_status(client->ctx, STATUS_LOCKED, NULL); - - if (rc) - return rc; + /* + * If a client disconnects unexpectedly while waiting for a + * lock, this lets the thread terminate because send_status() + * will return an error. + */ + while (pthread_mutex_trylock(&pin_mutex) == EBUSY) { + gpg_error_t rc = send_status(client->ctx, STATUS_LOCKED, NULL); + + if (rc) + return rc; + + sleep(1); + } } - - sleep(1); + else + MUTEX_LOCK(&pin_mutex); } + else + MUTEX_LOCK_DEBUG; client->pinentry->has_lock = TRUE; return 0; @@ -531,7 +543,7 @@ void unlock_pin_mutex(struct pinentry_s *pin) if (pin->has_lock == FALSE) return; - pthread_mutex_unlock(&pin_mutex); + MUTEX_UNLOCK(&pin_mutex); pin->has_lock = FALSE; } @@ -545,7 +557,20 @@ void cleanup_pinentry(struct pinentry_s *pin) if (pin->ctx && pin->pid) pinentry_disconnect(pin); + if (pthread_mutex_trylock(&pin->status_mutex) == EBUSY) { + MUTEX_UNLOCK(&pin->status_mutex); + } + else + pthread_mutex_unlock(&pin->status_mutex); + pthread_mutex_destroy(&pin->status_mutex); + + if (pthread_mutex_trylock(&pin->cond_mutex) == EBUSY) { + MUTEX_UNLOCK(&pin->cond_mutex); + } + else + pthread_mutex_unlock(&pin->cond_mutex); + pthread_mutex_destroy(&pin->cond_mutex); pthread_cond_destroy(&pin->cond); g_free(pin->ttyname); @@ -603,7 +628,7 @@ int pinentry_iterate(struct client_s *cl, gboolean read_ready) { gpg_error_t rc; - pthread_mutex_lock(&cl->pinentry->status_mutex); + MUTEX_LOCK(&cl->pinentry->status_mutex); /* Set from pinentry_timeout_thread(). */ if (cl->pinentry->status == PINENTRY_TIMEOUT) { @@ -612,6 +637,7 @@ int pinentry_iterate(struct client_s *cl, gboolean read_ready) cleanup_client(cl); reset(cl->pinentry); unlock_pin_mutex(cl->pinentry); + MUTEX_UNLOCK(&cl->pinentry->status_mutex); } if (cl->pinentry->status == PINENTRY_RUNNING) { @@ -699,7 +725,7 @@ int pinentry_iterate(struct client_s *cl, gboolean read_ready) } } - pthread_mutex_unlock(&cl->pinentry->status_mutex); + MUTEX_UNLOCK(&cl->pinentry->status_mutex); return 0; } diff --git a/src/pwmd.c b/src/pwmd.c index a2ea013e..4e8bf123 100644 --- a/src/pwmd.c +++ b/src/pwmd.c @@ -70,6 +70,7 @@ #include "cache.h" #include "misc.h" #include "pwmd.h" +#include "lock.h" GCRY_THREAD_OPTION_PTHREAD_IMPL; @@ -160,6 +161,9 @@ static struct client_thread_s *find_cn(pthread_t tid) { guint t, i; + if (!cn_thread_list) + return NULL; + pthread_mutex_lock(&cn_mutex); for (t = g_slist_length(cn_thread_list), i = 0; i < t; i++) { @@ -356,9 +360,9 @@ gpg_error_t send_status(assuan_context_t ctx, status_msg_t which, status = "CACHE"; break; case STATUS_CLIENTS: - pthread_mutex_lock(&cn_mutex); + MUTEX_LOCK(&cn_mutex); line = print_fmt(buf, sizeof(buf), "%i", g_slist_length(cn_thread_list)); - pthread_mutex_unlock(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); status = "CLIENTS"; break; case STATUS_CONFIG: @@ -417,7 +421,7 @@ void send_status_all(status_msg_t which) { guint i, t; - pthread_mutex_lock(&cn_mutex); + MUTEX_LOCK(&cn_mutex); for (t = g_slist_length(cn_thread_list), i = 0; i < t; i++) { struct client_thread_s *cn = g_slist_nth_data(cn_thread_list, i); @@ -428,7 +432,7 @@ void send_status_all(status_msg_t which) pthread_join(tid, NULL); } - pthread_mutex_unlock(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); } static void xml_error_cb(void *data, xmlErrorPtr e) @@ -497,7 +501,7 @@ static void cleanup_cb(void *arg) struct client_thread_s *cn = arg; struct client_s *cl = cn->cl; - pthread_mutex_lock(&cn_mutex); + MUTEX_LOCK(&cn_mutex); log_write(N_("exiting, fd=%i"), cn->fd); if (cl->thd) { @@ -541,16 +545,18 @@ static void cleanup_cb(void *arg) if (cn->name) g_free(cn->name); - if (pthread_mutex_trylock(&cn->msg_mutex) == EBUSY) - pthread_mutex_unlock(&cn->msg_mutex); + if (pthread_mutex_trylock(&cn->msg_mutex) == EBUSY) { + MUTEX_UNLOCK(&cn->msg_mutex); + } else pthread_mutex_unlock(&cn->msg_mutex); pthread_mutex_destroy(&cn->msg_mutex); pthread_cond_destroy(&cn->msg_cond); - if (pthread_mutex_trylock(&cn->msg_sender_mutex) == EBUSY) - pthread_mutex_unlock(&cn->msg_sender_mutex); + if (pthread_mutex_trylock(&cn->msg_sender_mutex) == EBUSY) { + MUTEX_UNLOCK(&cn->msg_sender_mutex); + } else pthread_mutex_unlock(&cn->msg_sender_mutex); @@ -569,7 +575,7 @@ static void cleanup_cb(void *arg) cn_thread_list = g_slist_remove(cn_thread_list, cn); g_free(cn); - pthread_mutex_unlock(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); send_status_all(STATUS_CLIENTS); } @@ -577,7 +583,7 @@ static void *client_msg_sender_thread(void *arg) { struct client_thread_s *thd = arg; - pthread_mutex_lock(&thd->msg_sender_mutex); + MUTEX_LOCK(&thd->msg_sender_mutex); for (;;) { struct status_msg_s *msg; @@ -595,9 +601,9 @@ static void *client_msg_sender_thread(void *arg) break; /* Unlock to prevent blocking in client_msg_thread(). */ - pthread_mutex_unlock(&thd->msg_sender_mutex); + MUTEX_UNLOCK(&thd->msg_sender_mutex); rc = send_status(thd->cl->ctx, msg->msg, NULL); - pthread_mutex_lock(&thd->msg_sender_mutex); + MUTEX_LOCK(&thd->msg_sender_mutex); thd->msg_queue = g_slist_remove(thd->msg_queue, msg); g_free(msg); pthread_testcancel(); @@ -623,7 +629,7 @@ static void *client_msg_thread(void *arg) { struct client_thread_s *thd = arg; - pthread_mutex_lock(&thd->msg_mutex); + MUTEX_LOCK(&thd->msg_mutex); for (;;) { fd_set rfds; @@ -649,7 +655,7 @@ static void *client_msg_thread(void *arg) continue; } - pthread_mutex_lock(&thd->msg_sender_mutex); + MUTEX_LOCK(&thd->msg_sender_mutex); /* Don't append duplicate status messages. If the client is lagged and * there are a bunch of status messages needing to be sent, only send @@ -665,14 +671,14 @@ static void *client_msg_thread(void *arg) if (!msg) { log_write("%s(%i): %s", __FILE__, __LINE__, strerror(ENOMEM)); - pthread_mutex_unlock(&thd->msg_sender_mutex); + MUTEX_UNLOCK(&thd->msg_sender_mutex); continue; } msg->msg = m; thd->msg_queue = g_slist_append(thd->msg_queue, msg); done: - pthread_mutex_unlock(&thd->msg_sender_mutex); + MUTEX_UNLOCK(&thd->msg_sender_mutex); pthread_cond_signal(&thd->msg_sender_cond); } @@ -695,8 +701,8 @@ static void *client_thread(void *data) * fails (returns) for some reason before init_new_connection() releases * the cn_mutex. */ - pthread_mutex_lock(&cn_mutex); - pthread_mutex_unlock(&cn_mutex); + MUTEX_LOCK(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); pthread_cleanup_push(cleanup_cb, thd); if (!cl) { @@ -1613,7 +1619,7 @@ static void init_new_connection(gint fd, gchar *addr) return; } - pthread_mutex_lock(&cn_mutex); + MUTEX_LOCK(&cn_mutex); new->fd = fd; #ifdef WITH_GNUTLS @@ -1629,13 +1635,13 @@ static void init_new_connection(gint fd, gchar *addr) if (n) { g_free(new); log_write("pthread_create(): %s", strerror(n)); - pthread_mutex_unlock(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); return; } new->tid = tid; cn_thread_list = g_slist_append(cn_thread_list, new); - pthread_mutex_unlock(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); if (addr) log_write(N_("new connection: fd=%i, addr=%s"), fd, addr); @@ -1875,9 +1881,9 @@ static void server_loop(gint sockfd, gchar **socketpath) unlink(*socketpath); g_free(*socketpath); *socketpath = NULL; - pthread_mutex_lock(&cn_mutex); + MUTEX_LOCK(&cn_mutex); n = g_slist_length(cn_thread_list); - pthread_mutex_unlock(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); if (n) log_write(N_("waiting for all clients to disconnect")); @@ -1889,9 +1895,9 @@ static void server_loop(gint sockfd, gchar **socketpath) } sleep(1); - pthread_mutex_lock(&cn_mutex); + MUTEX_LOCK(&cn_mutex); n = g_slist_length(cn_thread_list); - pthread_mutex_unlock(&cn_mutex); + MUTEX_UNLOCK(&cn_mutex); } startStopKeepAlive(TRUE); -- 2.11.4.GIT