From 89824e5d59efc5db0175550332bbf6ad54ba978f Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Wed, 1 Mar 2006 11:05:47 +0000 Subject: [PATCH] Fixed card removal problems --- TODO | 7 +- scd/ChangeLog | 18 ++- scd/apdu.c | 4 +- scd/app.c | 14 ++- scd/ccid-driver.c | 21 +++- scd/command.c | 276 +++++++++++++++++++++++++--------------------- 6 files changed, 203 insertions(+), 137 deletions(-) diff --git a/TODO b/TODO index 7a1f989b4..85e08ed16 100644 --- 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. diff --git a/scd/ChangeLog b/scd/ChangeLog index 3b850a293..d539d210e 100644 --- a/scd/ChangeLog +++ b/scd/ChangeLog @@ -1,10 +1,26 @@ +2006-03-01 Werner Koch + + * 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 + + * apdu.c (open_pcsc_reader): Fixed double free. Thanks to Moritz. + 2006-02-09 Werner Koch + * 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. diff --git a/scd/apdu.c b/scd/apdu.c index 5a5f18b43..adaaec612 100644 --- a/scd/apdu.c +++ b/scd/apdu.c @@ -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) diff --git a/scd/app.c b/scd/app.c index fad45ed85..7f6a8cc9f 100644 --- 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) diff --git a/scd/ccid-driver.c b/scd/ccid-driver.c index 519cb5f2d..e990f757a 100644 --- a/scd/ccid-driver.c +++ b/scd/ccid-driver.c @@ -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); diff --git a/scd/command.c b/scd/command.c index 1b7a8f67e..805164d0f 100644 --- a/scd/command.c +++ b/scd/command.c @@ -62,9 +62,26 @@ && (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; + + +/*-- Local prototypes --*/ +static void update_reader_status_file (void); @@ -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) + + if (slot != -1 && send_reset && !IS_LOCKED (ctrl) ) { - 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 (apdu_reset (slot)) { - 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 (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; - /* Reset card removed flag for the current reader. */ + /* 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. 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"); } @@ -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; + } - return slot; + /* Try to open the reader. */ + if (ss->slot == -1) + ss->slot = apdu_open_reader (opt.reader_port); + + 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; + for (idx=0; idx < DIM(slot_table); idx++) + { + struct slot_status_s *ss = slot_table + idx; - log_info ("updating status of slot %d to 0x%04X\n", slot, status); + 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. */ + + if (!ss->any || ss->status != status || ss->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", + ss->slot, status); - sprintf (templ, "reader_%d.status", slot); - fname = make_filename (opt.homedir, templ, NULL ); - fp = fopen (fname, "w"); - if (fp) + 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) { - 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; - - - /* 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); + 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"); }