Fixed card removal problems
authorWerner Koch <wk@gnupg.org>
Wed, 1 Mar 2006 11:05:47 +0000 (11:05 +0000)
committerWerner Koch <wk@gnupg.org>
Wed, 1 Mar 2006 11:05:47 +0000 (11:05 +0000)
TODO
scd/ChangeLog
scd/apdu.c
scd/app.c
scd/ccid-driver.c
scd/command.c

diff --git a/TODO b/TODO
index 7a1f989..85e08ed 100644 (file)
--- a/TODO
+++ b/TODO
@@ -67,7 +67,12 @@ might want to have an agent context for each service request
   Using the session_list in command.c and the lock_table in app.c.  IT
   would be better to do this just at one place. First we need to see
   how we can support cards with multiple applications.
-   
+** Detecting a removed card works only after the ticker detected it.
+ We should check the card status in open-card to make this smoother.
+ Needs to be integrated with the status file update, though.  It is
+ not a real problem because application will get a card removed status
+ and should the send a reset to try solving the problem.
+
 * tests
 ** Makefile.am
   We use printf(1) to setup the library path, this is not portable.
index 3b850a2..d539d21 100644 (file)
@@ -1,10 +1,26 @@
+2006-03-01  Werner Koch  <wk@g10code.com>
+
+       * command.c (status_file_update_lock): New.
+       (scd_update_reader_status_file): Use lock and factor existing code
+       out to ..
+       (update_reader_status_file): .. this.
+       (do_reset): Use the lock and call update_reader_status_file.
+
+2006-02-20  Werner Koch  <wk@g10code.com>
+
+       * apdu.c (open_pcsc_reader): Fixed double free.  Thanks to Moritz.
+
 2006-02-09  Werner Koch  <wk@g10code.com>
 
+       * command.c (get_reader_slot, do_reset) 
+       (scd_update_reader_status_file): Rewrote.
+
        * app.c (release_application): Factored code out to ..
        (deallocate_app): new function.
        (select_application): Introduce new saved application stuff.
        (application_notify_card_removed): New.
-       * command.c (update_card_removed): Call it.
+       * command.c (update_card_removed): Call it here.
+       (do_reset): And here.
 
        * app.c (check_application_conflict): New.
        * command.c (open_card): Use it here.
index 5a5f18b..adaaec6 100644 (file)
@@ -1594,6 +1594,7 @@ open_pcsc_reader (const char *portstr)
     }
   strcpy (reader_table[slot].rdrname, portstr? portstr : list);
   xfree (list);
+  list = NULL;
 
   err = pcsc_connect (reader_table[slot].pcsc.context,
                       reader_table[slot].rdrname,
@@ -1611,7 +1612,6 @@ open_pcsc_reader (const char *portstr)
       xfree (reader_table[slot].rdrname);
       reader_table[slot].rdrname = NULL;
       reader_table[slot].used = 0;
-      xfree (list);
       return -1 /*pcsc_error_to_sw (err)*/;
     }
 
@@ -2369,7 +2369,7 @@ apdu_close_reader (int slot)
 }
 
 /* Shutdown a reader; that is basically the same as a close but keeps
-   the handle ready for later use. A apdu_reset_header should be used
+   the handle ready for later use. A apdu_reset_reader should be used
    to get it active again. */
 int
 apdu_shutdown_reader (int slot)
index fad45ed..7f6a8cc 100644 (file)
--- a/scd/app.c
+++ b/scd/app.c
@@ -165,13 +165,17 @@ application_notify_card_removed (int slot)
     return;
 
   /* Deallocate a saved application for that slot, so that we won't
-     try to reuse it. */
-  if (lock_table[slot].initialized && lock_table[slot].last_app)
+     try to reuse it.  If there is no saved application, set a flag so
+     that we won't save the current state. */
+  if (lock_table[slot].initialized)
     {
       app_t app = lock_table[slot].last_app;
 
-      lock_table[slot].last_app = NULL;
-      deallocate_app (app);
+      if (app)
+        {
+          lock_table[slot].last_app = NULL;
+          deallocate_app (app);
+        }
     }
 }
 
@@ -380,7 +384,7 @@ deallocate_app (app_t app)
 /* Free the resources associated with the application APP.  APP is
    allowed to be NULL in which case this is a no-op.  Note that we are
    using reference counting to track the users of the application and
-   actually deferiing the deallcoation to allow for a later resuse by
+   actually deferring the deallocation to allow for a later reuse by
    a new connection. */
 void
 release_application (app_t app)
index 519cb5f..e990f75 100644 (file)
@@ -989,7 +989,12 @@ scan_or_find_devices (int readerno, const char *readerid,
 
       fd = open (transports[i].name, O_RDWR);
       if (fd == -1)
-        continue;
+        {
+          log_debug ("failed to open `%s': %s\n",
+                     transports[i].name, strerror (errno));
+          continue;
+        }
+      log_debug ("opened `%s': fd=%d\n", transports[i].name, fd);
 
       rid = malloc (strlen (transports[i].name) + 30 + 10);
       if (!rid)
@@ -1042,6 +1047,7 @@ scan_or_find_devices (int readerno, const char *readerid,
         }
       free (rid);
       close (fd);
+      log_debug ("closed fd %d\n", fd);
     }
 
   if (scan_mode)
@@ -1202,7 +1208,10 @@ ccid_open_reader (ccid_driver_t *handle, const char *readerid)
       if (idev)
         usb_close (idev);
       if (dev_fd != -1)
-        close (dev_fd);
+        {
+          close (dev_fd);
+          log_debug ("closed fd %d\n", dev_fd);
+        }
       free (*handle);
       *handle = NULL;
     }
@@ -1245,6 +1254,7 @@ do_close_reader (ccid_driver_t handle)
   if (handle->dev_fd != -1)
     {
       close (handle->dev_fd);
+      log_debug ("closed fd %d\n", handle->dev_fd);
       handle->dev_fd = -1;
     }
 }
@@ -1314,7 +1324,10 @@ ccid_shutdown_reader (ccid_driver_t handle)
         usb_close (handle->idev);
       handle->idev = NULL;
       if (handle->dev_fd != -1)
-        close (handle->dev_fd);
+        {
+          close (handle->dev_fd);
+          log_debug ("closed fd %d\n", handle->dev_fd);
+        }
       handle->dev_fd = -1;
     }
 
@@ -1327,7 +1340,7 @@ ccid_shutdown_reader (ccid_driver_t handle)
 int 
 ccid_close_reader (ccid_driver_t handle)
 {
-  if (!handle || !handle->idev)
+  if (!handle || (!handle->idev && handle->dev_fd == -1))
     return 0;
 
   do_close_reader (handle);
index 1b7a8f6..805164d 100644 (file)
       && (c)->reader_slot == locked_session->ctrl_backlink->reader_slot)
 
 
+/* This structure is used to keep track of open readers (slots). */
+struct slot_status_s 
+{
+  int valid;  /* True if the other objects are valid. */
+  int slot;   /* Slot number of the reader or -1 if not open. */
+
+  int reset_failed; /* A reset failed. */
+
+  int any;    /* Flag indicating whether any status check has been
+                 done.  This is set once to indicate that the status
+                 tracking for the slot has been initialized.  */
+  unsigned int status;  /* Last status of the slot. */
+  unsigned int changed; /* Last change counter of teh slot. */
+};
+
+
 /* Data used to associate an Assuan context with local server data.
    This object describes the local properties of one session.  */
-struct server_local_s {
+struct server_local_s 
+{
   /* We keep a list of all active sessions with the anchor at
      SESSION_LIST (see below).  This field is used for linking. */
   struct server_local_s *next_session; 
@@ -86,6 +103,10 @@ struct server_local_s {
 };
 
 
+/* The table with information on all used slots. */
+static struct slot_status_s slot_table[10];
+
+
 /* To keep track of all running sessions, we link all active server
    contexts and the anchor in this variable.  */
 static struct server_local_s *session_list;
@@ -94,6 +115,13 @@ static struct server_local_s *session_list;
    in this variable. */
 static struct server_local_s *locked_session;
 
+/* While doing a reset we need to make sure that the ticker does not
+   call scd_update_reader_status_file while we are using it. */
+static pth_mutex_t status_file_update_lock = PTH_MUTEX_INIT;
+
+\f
+/*-- Local prototypes --*/
+static void update_reader_status_file (void);
 
 
 \f
@@ -107,7 +135,9 @@ update_card_removed (int slot, int value)
   for (sl=session_list; sl; sl = sl->next_session)
     if (sl->ctrl_backlink
         && sl->ctrl_backlink->reader_slot == slot)
-      sl->card_removed = value;
+      {
+        sl->card_removed = value;
+      }
   if (value)
     application_notify_card_removed (slot);
 }
@@ -126,69 +156,52 @@ has_option (const char *line, const char *name)
 }
 
 
-/* Reset the card and free the application context.  With DO_CLOSE set
-   to true and this is the last session with a reference to the
-   reader, close the reader and don't do just a reset. */
+/* Reset the card and free the application context.  With SEND_RESET
+   set to true actually send a RESET to the reader. */
 static void
-do_reset (ctrl_t ctrl, int do_close)
+do_reset (ctrl_t ctrl, int send_reset)
 {
   int slot = ctrl->reader_slot;
 
+  if (!(slot == -1 || (slot >= 0 && slot < DIM(slot_table))))
+    BUG ();
+
   if (ctrl->app_ctx)
     {
       release_application (ctrl->app_ctx);
       ctrl->app_ctx = NULL;
     }
-  if (ctrl->reader_slot != -1)
-    {
-      struct server_local_s *sl;
 
-      /* If we are the only session with the reader open we may close
-         it.  If not, do a reset unless a lock is held on the
-         reader.  */
-      for (sl=session_list; sl; sl = sl->next_session)
-        if (sl != ctrl->server_local
-            && sl->ctrl_backlink->reader_slot == ctrl->reader_slot)
-          break;
-      if (sl) /* There is another session with the reader open. */
-        {
-          if ( IS_LOCKED (ctrl) ) /* If it is locked, release it. */
-            ctrl->reader_slot = -1;
-          else
-            {
-              if (do_close) /* Always mark reader unused. */
-                ctrl->reader_slot = -1;
-              else if (apdu_reset (ctrl->reader_slot)) /* Reset only if
-                                                          not locked */
-                {
-                  /* The reset failed.  Mark the reader as closed. */
-                  ctrl->reader_slot = -1;
-                }
-
-              if (locked_session && ctrl->server_local == locked_session)
-                {
-                  locked_session = NULL;
-                  log_debug ("implicitly unlocking due to RESET\n");
-                }
-            }
-        }
-      else /* No other session has the reader open.  */
+  if (slot != -1 && send_reset && !IS_LOCKED (ctrl) )
+    {
+      if (apdu_reset (slot)) 
         {
-          if (do_close || apdu_reset (ctrl->reader_slot))
-            {
-              apdu_close_reader (ctrl->reader_slot);
-              ctrl->reader_slot = -1;
-            }
-          if ( IS_LOCKED (ctrl) )
-            {
-              log_debug ("WARNING: cleaning up stale session lock\n");
-              locked_session =  NULL;
-            }
+          slot_table[slot].reset_failed = 1;
         }
     }
+  ctrl->reader_slot = -1;
+
+  /* If we hold a lock, unlock now. */
+  if (locked_session && ctrl->server_local == locked_session)
+    {
+      locked_session = NULL;
+      log_info ("implicitly unlocking due to RESET\n");
+    }
 
-  /* Reset card removed flag for the current reader.  */
+  /* Reset card removed flag for the current reader.  We need to take
+     the lock here so that the ticker thread won't concurrently try to
+     update the file.  Note that the update function will set the card
+     removed flag and we will later reset it - not a particualar nice
+     way of implementing it but it works. */
+  if (!pth_mutex_acquire (&status_file_update_lock, 0, NULL))
+    {
+      log_error ("failed to acquire status_fle_update lock\n");
+      return;
+    }
+  update_reader_status_file ();
   update_card_removed (slot, 0);
+  if (!pth_mutex_release (&status_file_update_lock))
+    log_error ("failed to release status_file_update lock\n");
 }
 
 \f
@@ -197,7 +210,7 @@ reset_notify (assuan_context_t ctx)
 {
   ctrl_t ctrl = assuan_get_pointer (ctx); 
 
-  do_reset (ctrl, 0);
+  do_reset (ctrl, 1);
 }
 
 
@@ -226,18 +239,22 @@ option_handler (assuan_context_t ctx, const char *key, const char *value)
 static int
 get_reader_slot (void)
 {
-  struct server_local_s *sl;
-  int slot= -1;
+  struct slot_status_s *ss;
 
-  for (sl=session_list; sl; sl = sl->next_session)
-    if (sl->ctrl_backlink
-        && (slot = sl->ctrl_backlink->reader_slot) != -1)
-      break;
+  ss = &slot_table[0]; /* One reader for now. */
 
-  if (slot == -1)
-    slot = apdu_open_reader (opt.reader_port);
+  /* Initialize the item if needed. */
+  if (!ss->valid)
+    {
+      ss->slot = -1;
+      ss->valid = 1;
+    }
+
+  /* Try to open the reader. */
+  if (ss->slot == -1)
+    ss->slot = apdu_open_reader (opt.reader_port);
 
-  return slot;
+  return ss->slot;
 }
 
 /* If the card has not yet been opened, do it.  Note that this
@@ -349,7 +366,7 @@ cmd_serialno (assuan_context_t ctx, char *line)
     {
       if ( IS_LOCKED (ctrl) )
         return gpg_error (GPG_ERR_LOCKED);
-      do_reset (ctrl, 0);
+      do_reset (ctrl, 1);
     }
 
   if ((rc = open_card (ctrl, *line? line:NULL)))
@@ -1305,7 +1322,7 @@ cmd_getinfo (assuan_context_t ctx, char *line)
 
 /* RESTART
 
-   Restart the current connection; this is a kind of warn reset.  It
+   Restart the current connection; this is a kind of warm reset.  It
    deletes the context used by this connection but does not send a
    RESET to the card.  Thus the card itself won't get reset. 
 
@@ -1462,7 +1479,7 @@ scd_command_handler (int fd)
     }
 
   /* Cleanup.  */
-  do_reset (&ctrl, 1); 
+  do_reset (&ctrl, 0); 
 
   /* Release the server object.  */
   if (session_list == ctrl.server_local)
@@ -1532,77 +1549,88 @@ send_status_info (ctrl_t ctrl, const char *keyword, ...)
 }
 
 
-/* This function is called by the ticker thread to check for changes
-   of the reader stati.  It updates the reader status files and if
-   requested by the caller also send a signal to the caller.  */
-void
-scd_update_reader_status_file (void)
+/* This is the core of scd_update_reader_status_file but the caller
+   needs to take care of the locking. */
+static void
+update_reader_status_file (void)
 {
-  static struct {
-    int any;
-    unsigned int status;
-    unsigned int changed;
-  } last[10];
-  int slot;
-  int used;
+  int idx;
   unsigned int status, changed;
 
   /* Note, that we only try to get the status, because it does not
      make sense to wait here for a operation to complete.  If we are
      busy working with a card, delays in the status file update should
      be acceptable. */
-  for (slot=0; (slot < DIM(last)
-                &&!apdu_enum_reader (slot, &used)); slot++)
-    if (used && !apdu_get_status (slot, 0, &status, &changed))
-      {
-        if (!last[slot].any || last[slot].status != status
-            || last[slot].changed != changed )
-          {
-            char *fname;
-            char templ[50];
-            FILE *fp;
-            struct server_local_s *sl;
-
-            log_info ("updating status of slot %d to 0x%04X\n", slot, status);
-            
-            sprintf (templ, "reader_%d.status", slot);
-            fname = make_filename (opt.homedir, templ, NULL );
-            fp = fopen (fname, "w");
-            if (fp)
-              {
-                fprintf (fp, "%s\n",
-                         (status & 1)? "USABLE":
-                         (status & 4)? "ACTIVE":
-                         (status & 2)? "PRESENT": "NOCARD");
-                fclose (fp);
-              }
-            xfree (fname);
-
-            /* Set the card removed flag for all current sessions.  We
-               will set this on any card change because a reset or
-               SERIALNO request must be done in any case.  */
-            if (last[slot].any)
-              update_card_removed (slot, 1);
-
-            last[slot].any = 1;
-            last[slot].status = status;
-            last[slot].changed = changed;
+  for (idx=0; idx < DIM(slot_table); idx++)
+    {
+      struct slot_status_s *ss = slot_table + idx;
 
+      if (!ss->valid || ss->slot == -1)
+        continue; /* Not valid or reader not yet open. */
+      
+      if ( apdu_get_status (ss->slot, 0, &status, &changed) )
+        continue; /* Get status failed. */
 
-            /* Send a signal to all clients who applied for it.  */
-            for (sl=session_list; sl; sl = sl->next_session)
-              if (sl->event_signal && sl->assuan_ctx)
-                {
-                  pid_t pid = assuan_get_pid (sl->assuan_ctx);
-                  int signo = sl->event_signal;
+      if (!ss->any || ss->status != status || ss->changed != changed )
+        {
+          char *fname;
+          char templ[50];
+          FILE *fp;
+          struct server_local_s *sl;
 
-                  log_info ("client pid is %d, sending signal %d\n",
-                            pid, signo);
+          log_info ("updating status of slot %d to 0x%04X\n",
+                    ss->slot, status);
+            
+          sprintf (templ, "reader_%d.status", ss->slot);
+          fname = make_filename (opt.homedir, templ, NULL );
+          fp = fopen (fname, "w");
+          if (fp)
+            {
+              fprintf (fp, "%s\n",
+                       (status & 1)? "USABLE":
+                       (status & 4)? "ACTIVE":
+                       (status & 2)? "PRESENT": "NOCARD");
+              fclose (fp);
+            }
+          xfree (fname);
+            
+          /* Set the card removed flag for all current sessions.  We
+             will set this on any card change because a reset or
+             SERIALNO request must be done in any case.  */
+          if (ss->any)
+            update_card_removed (ss->slot, 1);
+          
+          ss->any = 1;
+          ss->status = status;
+          ss->changed = changed;
+
+          /* Send a signal to all clients who applied for it.  */
+          for (sl=session_list; sl; sl = sl->next_session)
+            if (sl->event_signal && sl->assuan_ctx)
+              {
+                pid_t pid = assuan_get_pid (sl->assuan_ctx);
+                int signo = sl->event_signal;
+                
+                log_info ("client pid is %d, sending signal %d\n",
+                          pid, signo);
 #ifndef HAVE_W32_SYSTEM
-                  if (pid != (pid_t)(-1) && pid && signo > 0)
-                    kill (pid, signo);
+                if (pid != (pid_t)(-1) && pid && signo > 0)
+                  kill (pid, signo);
 #endif
-                }
-          }
-      }
+              }
+        }
+    }
+}
+
+/* This function is called by the ticker thread to check for changes
+   of the reader stati.  It updates the reader status files and if
+   requested by the caller also send a signal to the caller.  */
+void
+scd_update_reader_status_file (void)
+{
+  if (!pth_mutex_acquire (&status_file_update_lock, 1, NULL))
+    return; /* locked - give up. */
+  update_reader_status_file ();
+  if (!pth_mutex_release (&status_file_update_lock))
+    log_error ("failed to release status_file_update lock\n");
 }