From bb8e3996e44f4f057f0771b9b72c89feae00f1b1 Mon Sep 17 00:00:00 2001
From: Werner Koch <wk@gnupg.org>
Date: Wed, 28 Apr 2021 18:19:33 +0200
Subject: [PATCH] scd: Fix problem with reader list becoming empty.

* scd/apdu.c (close_pcsc_reader): Do not decrement refcount if already
zero.  Always release context if or becomes zero.
(apdu_dev_list_start): Unlock prior to close_pcsc_reader.  For PC/SC
increment the count.  Always release the lock.
(apdu_dev_list_finish): No more unlocking.  Use close_pcsc_reader
instead of code duplication.

* scd/apdu.c (pcsc_error_string): Add an error code.
* scd/scdaemon.c (scd_kick_the_loop): Fix a diagnostic.
--

There was an obvious bug in that the pcsc.count could go below zero
and thus there was no chance to get the context release.  Releasing
and recreating the context is at least under Windows important to get
rit of the PCSC_E_SERVICE_STOPPED.

Also removes a potential problem in holding the reader_table_lock
between calls to apdu_dev_list_start apdu_dev_list_finish.  There is
no need for this.  Instead we bump the pcsc.count.

The reader_table_lock strategy should be reviewed; we may be able to
remove it.

Signed-off-by: Werner Koch <wk@gnupg.org>
---
 scd/apdu.c     | 44 +++++++++++++++++++++++---------------------
 scd/scdaemon.c |  4 +++-
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/scd/apdu.c b/scd/apdu.c
index 10657102e..c826301ef 100644
--- a/scd/apdu.c
+++ b/scd/apdu.c
@@ -625,6 +625,7 @@ pcsc_error_string (long err)
     case 0x001c: s = "card unsupported"; break;
     case 0x001d: s = "no service"; break;
     case 0x001e: s = "service stopped"; break;
+    case 0x002e: s = "no readers available"; break;
     default:     s = "unknown PC/SC error code"; break;
     }
   return s;
@@ -827,12 +828,16 @@ control_pcsc (int slot, pcsc_dword_t ioctl_code,
 static int
 close_pcsc_reader (int slot)
 {
+  /*log_debug ("%s: count=%d (ctx=%x)\n", __func__, pcsc.count, pcsc.context);*/
   (void)slot;
-  if (--pcsc.count == 0 && npth_mutex_trylock (&reader_table_lock) == 0)
+  if (!pcsc.count || !--pcsc.count)
     {
       int i;
 
-      pcsc_release_context (pcsc.context);
+      /*log_debug ("%s: releasing context\n", __func__);*/
+      npth_mutex_lock (&reader_table_lock);
+      if (pcsc.context)
+        pcsc_release_context (pcsc.context);
       pcsc.context = 0;
       for (i = 0; i < MAX_READER; i++)
         pcsc.rdrname[i] = NULL;
@@ -2008,12 +2013,15 @@ apdu_dev_list_start (const char *portstr, struct dev_list **l_p)
       char *p = NULL;
 
       if (!pcsc.context)
-        if (pcsc_init () < 0)
-          {
-            xfree (dl);
-            npth_mutex_unlock (&reader_table_lock);
-            return gpg_error (GPG_ERR_NO_SERVICE);
-          }
+        {
+          /* log_debug ("%s: No context - calling init\n", __func__); */
+          if (pcsc_init () < 0)
+            {
+              xfree (dl);
+              npth_mutex_unlock (&reader_table_lock);
+              return gpg_error (GPG_ERR_NO_SERVICE);
+            }
+        }
 
       r = pcsc_list_readers (pcsc.context, NULL, NULL, &nreader);
       if (!r)
@@ -2024,9 +2032,9 @@ apdu_dev_list_start (const char *portstr, struct dev_list **l_p)
               err = gpg_error_from_syserror ();
 
               log_error ("error allocating memory for reader list\n");
+              npth_mutex_unlock (&reader_table_lock);
               close_pcsc_reader (0);
               xfree (dl);
-              npth_mutex_unlock (&reader_table_lock);
               return err;
             }
           r = pcsc_list_readers (pcsc.context, NULL, p, &nreader);
@@ -2036,9 +2044,9 @@ apdu_dev_list_start (const char *portstr, struct dev_list **l_p)
           log_error ("pcsc_list_readers failed: %s (0x%lx)\n",
                      pcsc_error_string (r), r);
           xfree (p);
+          npth_mutex_unlock (&reader_table_lock);
           close_pcsc_reader (0);
           xfree (dl);
-          npth_mutex_unlock (&reader_table_lock);
           return iso7816_map_sw (pcsc_error_to_sw (r));
         }
 
@@ -2074,8 +2082,12 @@ apdu_dev_list_start (const char *portstr, struct dev_list **l_p)
               break;
             }
         }
+
+      pcsc.count++;
     }
 
+  npth_mutex_unlock (&reader_table_lock);
+
   *l_p = dl;
   return 0;
 }
@@ -2092,20 +2104,10 @@ apdu_dev_list_finish (struct dev_list *dl)
   else
 #endif
     { /* PC/SC readers.  */
-      int i;
-
       xfree (dl->table);
-      for (i = 0; i < MAX_READER; i++)
-        pcsc.rdrname[i] = NULL;
-
-      if (pcsc.count == 0)
-        {
-          pcsc_release_context (pcsc.context);
-          pcsc.context = 0;
-        }
+      close_pcsc_reader (0);
     }
   xfree (dl);
-  npth_mutex_unlock (&reader_table_lock);
 }
 
 
diff --git a/scd/scdaemon.c b/scd/scdaemon.c
index 60d68c26a..da1f21d1f 100644
--- a/scd/scdaemon.c
+++ b/scd/scdaemon.c
@@ -1231,7 +1231,7 @@ scd_kick_the_loop (void)
 #else
   int ret = kill (main_thread_pid, SIGCONT);
   if (ret < 0)
-    log_error ("SetEvent for scd_kick_the_loop failed: %s\n",
+    log_error ("sending signal for scd_kick_the_loop failed: %s\n",
                gpg_strerror (gpg_error_from_syserror ()));
 #endif
 }
@@ -1287,6 +1287,8 @@ handle_connections (int listen_fd)
 
     events[0] = the_event = INVALID_HANDLE_VALUE;
     events[1] = INVALID_HANDLE_VALUE;
+    /* Create event for manual reset, initially non-signaled.  Make it
+     * waitable and inheritable.  */
     h = CreateEvent (&sa, TRUE, FALSE, NULL);
     if (!h)
       log_error ("can't create scd event: %s\n", w32_strerror (-1) );