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 <gniibe@fsij.org>

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);

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);
+  npth_attr_setdetachstate (&tattr, NPTH_CREATE_JOINABLE);
-  rc = npth_create (&thread, &tattr, datastream_thread, kcd);
+  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 < 4)
-      if (nread == 0)
+        break;
-        {
-          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;
-        }
 
       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.  */
+  es_fclose (fp);
-  /* FIXME: Make thread killing explicit.  Otherwise we run in a
+
-   * log_fatal due to the destroyed mutex. */
   npth_cond_destroy (&kcd->cond);
   npth_mutex_destroy (&kcd->mutex);
   xfree (kcd);

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);