From c907cdbfa74881626148f90504fdf8c843691066 Mon Sep 17 00:00:00 2001 From: Ben Kibbey Date: Mon, 26 Jan 2009 21:44:34 -0500 Subject: [PATCH] If the client thread terminates while a pinentry is running, terminate the pinentry process in cleanup_pinentry(). Added a few pthread_testcancel() checks and thread cleanup handlers. --- src/commands.c | 11 +++++++-- src/pinentry.c | 28 ++++++++++++++++------- src/pwmd.c | 70 ++++++++++++++++++++++++++++++++++------------------------ 3 files changed, 70 insertions(+), 39 deletions(-) diff --git a/src/commands.c b/src/commands.c index 1776b56d..60d64fc3 100644 --- a/src/commands.c +++ b/src/commands.c @@ -315,6 +315,11 @@ fail: return FALSE; } +static void read_file_header_handler(void *arg) +{ + close((int)arg); +} + file_header_internal_t *read_file_header(const gchar *filename, gboolean v1, gpg_error_t *rc) { @@ -359,9 +364,11 @@ file_header_internal_t *read_file_header(const gchar *filename, gboolean v1, len = read(fd, &fh->fh2, fh_size); if (len != fh_size) { - close(fd); - *rc = GPG_ERR_INV_LENGTH; + gint n = errno; + pthread_cleanup_push(read_file_header_handler, (void *)fd); g_free(fh); + pthread_cleanup_pop(1); + *rc = gpg_error_from_errno(n); return NULL; } diff --git a/src/pinentry.c b/src/pinentry.c index 0aa40158..a5c2cdf0 100644 --- a/src/pinentry.c +++ b/src/pinentry.c @@ -419,6 +419,20 @@ static void reset(struct pinentry_s *pin) pin->status = PINENTRY_NONE; } +/* pin->status_mutex should be locked before calling this function. */ +static void kill_pinentry(struct pinentry_s *pin) +{ + if (pin->pin_pid == 0) + return; + + if (kill(pin->pin_pid, 0) == 0) + if (kill(pin->pin_pid, SIGTERM) == 0) + if (kill(pin->pin_pid, 0) == 0) + kill(pin->pin_pid, SIGKILL); + + pin->pin_pid = 0; +} + static void *timeout_thread(void *arg) { struct pinentry_s *pin = arg; @@ -438,12 +452,7 @@ static void *timeout_thread(void *arg) } MUTEX_LOCK(&pin->status_mutex); - - if (kill(pin->pin_pid, 0) == 0) - if (kill(pin->pin_pid, SIGTERM) == 0) - if (kill(pin->pin_pid, 0) == 0) - kill(pin->pin_pid, SIGKILL); - + kill_pinentry(pin); pin->status = PINENTRY_TIMEOUT; MUTEX_UNLOCK(&pin->cond_mutex); MUTEX_UNLOCK(&pin->status_mutex); @@ -552,8 +561,6 @@ void cleanup_pinentry(struct pinentry_s *pin) if (!pin) return; - unlock_pin_mutex(pin); - if (pin->ctx && pin->pid) pinentry_disconnect(pin); @@ -563,6 +570,10 @@ void cleanup_pinentry(struct pinentry_s *pin) else pthread_mutex_unlock(&pin->status_mutex); + MUTEX_LOCK(&pin->status_mutex); + kill_pinentry(pin); + MUTEX_UNLOCK(&pin->status_mutex); + unlock_pin_mutex(pin); pthread_mutex_destroy(&pin->status_mutex); if (pthread_mutex_trylock(&pin->cond_mutex) == EBUSY) { @@ -648,6 +659,7 @@ int pinentry_iterate(struct client_s *cl, gboolean read_ready) memset(&pk, 0, sizeof(pk)); len = read(cl->pinentry->fd, &pk, sizeof(pk)); + pthread_testcancel(); if (len == sizeof(pk)) { if (pk.error) { diff --git a/src/pwmd.c b/src/pwmd.c index 19162130..680f58af 100644 --- a/src/pwmd.c +++ b/src/pwmd.c @@ -449,6 +449,29 @@ static void xml_error_cb(void *data, xmlErrorPtr e) xmlCopyError(e, client->xml_error); } +static void cleanup_crypto_handler(void *arg) +{ + struct client_crypto_s **a = (struct client_crypto_s **)arg; + struct client_crypto_s *cr = *a; + + if (cr->fh) { + if (cr->fh->fd != -1 || + (cmdline == TRUE && cr->fh->fd != STDOUT_FILENO)) + close(cr->fh->fd); + + if (cr->fh->doc) + gcry_free(cr->fh->doc); + + g_free(cr->fh); + } + + if (cr->gh) + gcry_cipher_close(cr->gh); + + g_free(cr); + *a = NULL; +} + void cleanup_crypto(struct client_crypto_s **c) { struct client_crypto_s *cr = *c; @@ -474,22 +497,9 @@ void cleanup_crypto(struct client_crypto_s **c) if (cr->outbuf) gcry_free(cr->outbuf); - if (cr->fh) { - if (cr->fh->fd != -1 || - (cmdline == TRUE && cr->fh->fd != STDOUT_FILENO)) - close(cr->fh->fd); - - if (cr->fh->doc) - gcry_free(cr->fh->doc); - - g_free(cr->fh); - } - - if (cr->gh) - gcry_cipher_close(cr->gh); - - g_free(cr); - *c = NULL; + pthread_cleanup_push(cleanup_crypto_handler, c); + pthread_testcancel(); + pthread_cleanup_pop(1); } /* @@ -624,8 +634,10 @@ static void *client_msg_sender_thread(void *arg) * message read from a pipe to the clients message queue. The actual sending * of the message is done in client_msg_sender_thread() which waits for a * signal from this function. This prevents blocking in assuan_send_status() - * when sending to remote clients. The messages are sent in order that they - * arrive which wouldn't be guaranteed if a thread was created here instead. + * when sending to remote clients. This also avoids duplicate status messages. + * If an existing status message in the message queue has the same type as the + * current message the the current one will be skipped. This avoids flooding + * the client with old status messages. */ static void *client_msg_thread(void *arg) { @@ -659,9 +671,6 @@ static void *client_msg_thread(void *arg) 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 - * the latest unique ones. */ for (n = 0; n < g_slist_length(thd->msg_queue); n++) { msg = g_slist_nth_data(thd->msg_queue, n); @@ -688,8 +697,8 @@ done: } /* - * Called every time a connection is made via pthread_create(). This is the - * thread entry point. + * Called every time a connection is made from init_new_connection(). This is + * the thread entry point. */ static void *client_thread(void *data) { @@ -733,10 +742,6 @@ static void *client_thread(void *data) cl->thd = thd; - /* - * This is a "child" thread. Don't catch any signals. Let the master - * thread take care of signals in server_loop(). - */ if (new_connection(cl)) goto fail; @@ -750,6 +755,11 @@ static void *client_thread(void *data) } #ifdef WITH_GNUTLS + /* Require the client to explicity set OPTION PINENTRY since the DISPLAY + * might be automatically set from the client. Connections to the X11 + * server usually aren't encrypted and the client might unintentionally + * send the passphrase in the clear. + */ if (thd->remote) cl->pinentry->enable = FALSE; #endif @@ -789,7 +799,6 @@ static void *client_thread(void *data) for (;;) { fd_set rfds; int n; - struct timeval tv = {1,0}; FD_ZERO(&rfds); FD_SET(cl->thd->fd, &rfds); @@ -802,13 +811,15 @@ static void *client_thread(void *data) #else n = cl->thd->fd; #endif - n = select(n+1, &rfds, NULL, NULL, &tv); + n = select(n+1, &rfds, NULL, NULL, NULL); + pthread_testcancel(); if (n <= 0) continue; if (FD_ISSET(cl->thd->fd, &rfds)) { rc = assuan_process_next(cl->ctx); + pthread_testcancel(); if (rc) { cl->inquire_status = INQUIRE_INIT; @@ -837,6 +848,7 @@ static void *client_thread(void *data) case INQUIRE_DONE: cl->inquire_status = INQUIRE_INIT; rc = assuan_process_done(cl->ctx, 0); + pthread_testcancel(); break; } } -- 2.11.4.GIT