From f5656ff363a00c7649224d827b92c3e6031b6b77 Mon Sep 17 00:00:00 2001 From: NIIBE Yutaka Date: Wed, 7 Jun 2023 15:26:34 +0900 Subject: [PATCH] kbx: Fix datastream_thread and use the data pipe. * g10/call-keyboxd.c (gpg_keyboxd_deinit_session_data): Release the assuan connection before kbx_client_data_release. (open_context): Enable use of the data pipe. * sm/keydb.c (gpgsm_keydb_deinit_session_data): Release the assuan connection before kbx_client_data_release. (open_context): Enable use of the data pipe. * kbx/kbx-client-util.c (struct kbx_client_data_s): Add THD field. (prepare_data_pipe): Close the pipe output end as it's been sent already. Remember the KCD->THD, so that it can be joined later. (datastream_thread): Finish when reading no data from the pipe. (kbx_client_data_release): Join the thread. Then, we can safely call es_fclose on the FP. -- GnuPG-bug-id: 6512 Signed-off-by: NIIBE Yutaka --- g10/call-keyboxd.c | 13 ++++++++++--- kbx/kbx-client-util.c | 32 ++++++++++++-------------------- sm/keydb.c | 13 ++++++++++--- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/g10/call-keyboxd.c b/g10/call-keyboxd.c index 7f4d5f493..dc3d30a93 100644 --- a/g10/call-keyboxd.c +++ b/g10/call-keyboxd.c @@ -94,8 +94,6 @@ gpg_keyboxd_deinit_session_data (ctrl_t ctrl) log_error ("oops: trying to cleanup an active keyboxd context\n"); else { - kbx_client_data_release (kbl->kcd); - kbl->kcd = NULL; if (kbl->ctx && in_transaction) { /* This is our hack to commit the changes done during a @@ -112,6 +110,15 @@ gpg_keyboxd_deinit_session_data (ctrl_t ctrl) } assuan_release (kbl->ctx); kbl->ctx = NULL; + /* + * Since there may be pipe output FD sent to the server (so + * that it can receive data through the pipe), we should + * release the assuan connection before releasing KBL->KCD. + * This way, the data receiving thread can finish cleanly, + * and we can join the thread. + */ + kbx_client_data_release (kbl->kcd); + kbl->kcd = NULL; } xfree (kbl); } @@ -223,7 +230,7 @@ open_context (ctrl_t ctrl, keyboxd_local_t *r_kbl) return err; } - err = kbx_client_data_new (&kbl->kcd, kbl->ctx, 1); + err = kbx_client_data_new (&kbl->kcd, kbl->ctx, 0); if (err) { assuan_release (kbl->ctx); diff --git a/kbx/kbx-client-util.c b/kbx/kbx-client-util.c index 79d512bb3..ca791d4a3 100644 --- a/kbx/kbx-client-util.c +++ b/kbx/kbx-client-util.c @@ -53,6 +53,7 @@ struct kbx_client_data_s /* Condition variable to sync the datastream with the command. */ npth_mutex_t mutex; npth_cond_t cond; + npth_t thd; /* The data received from the keyboxd and an error code if there was * a problem (in which case DATA is also set to NULL. This is only @@ -103,7 +104,6 @@ prepare_data_pipe (kbx_client_data_t kcd) int rc; int inpipe[2]; estream_t infp; - npth_t thread; npth_attr_t tattr; kcd->fp = NULL; @@ -142,6 +142,7 @@ prepare_data_pipe (kbx_client_data_t kcd) return 0; } + close (inpipe[1]); kcd->fp = infp; @@ -154,8 +155,8 @@ prepare_data_pipe (kbx_client_data_t kcd) kcd->fp = NULL; return err; } - npth_attr_setdetachstate (&tattr, NPTH_CREATE_DETACHED); - rc = npth_create (&thread, &tattr, datastream_thread, kcd); + npth_attr_setdetachstate (&tattr, NPTH_CREATE_JOINABLE); + rc = npth_create (&kcd->thd, &tattr, datastream_thread, kcd); if (rc) { err = gpg_error_from_errno (rc); @@ -197,20 +198,8 @@ datastream_thread (void *arg) gnupg_sleep (1); continue; } -#ifdef HAVE_W32_SYSTEM - if (nread == 0) - { - gnupg_sleep (1); - continue; - } -#endif - if (nread != 4) - { - err = gpg_error (GPG_ERR_EIO); - log_error ("error reading data length from keyboxd: %s\n", - "short read"); - continue; - } + if (nread < 4) + break; datalen = buf32_to_size_t (lenbuf); /* log_debug ("keyboxd announced %zu bytes\n", datalen); */ @@ -340,11 +329,14 @@ kbx_client_data_release (kbx_client_data_t kcd) if (!kcd) return; + + if (npth_join (kcd->thd, NULL)) + log_error ("kbx_client_data_release failed on npth_join"); + fp = kcd->fp; kcd->fp = NULL; - es_fclose (fp); /* That close should let the thread run into an error. */ - /* FIXME: Make thread killing explicit. Otherwise we run in a - * log_fatal due to the destroyed mutex. */ + es_fclose (fp); + npth_cond_destroy (&kcd->cond); npth_mutex_destroy (&kcd->mutex); xfree (kcd); diff --git a/sm/keydb.c b/sm/keydb.c index 38737c96a..9b3c7c8ba 100644 --- a/sm/keydb.c +++ b/sm/keydb.c @@ -161,10 +161,17 @@ gpgsm_keydb_deinit_session_data (ctrl_t ctrl) log_error ("oops: trying to cleanup an active keydb context\n"); else { - kbx_client_data_release (kbl->kcd); - kbl->kcd = NULL; assuan_release (kbl->ctx); kbl->ctx = NULL; + /* + * Since there may be pipe output FD sent to the server (so + * that it can receive data through the pipe), we should + * release the assuan connection before releasing KBL->KCD. + * This way, the data receiving thread can finish cleanly, + * and we can join the thread. + */ + kbx_client_data_release (kbl->kcd); + kbl->kcd = NULL; } xfree (kbl); } @@ -580,7 +587,7 @@ open_context (ctrl_t ctrl, keydb_local_t *r_kbl) return err; } - err = kbx_client_data_new (&kbl->kcd, kbl->ctx, 1); + err = kbx_client_data_new (&kbl->kcd, kbl->ctx, 0); if (err) { assuan_release (kbl->ctx);