From c594dcfc93486cd26e193aa5c82bb8a8f30ab57b Mon Sep 17 00:00:00 2001 From: Werner Koch Date: Mon, 17 Jun 2019 16:19:22 +0200 Subject: [PATCH] scd: Add explict functions for 'app' reference counting. * scd/app.c (app_ref): New. (app_unref): New. (release_application): Renamed to ... (app_unref_locked): this and remove arg locked_already. Change callers to use this or app_ref. * scd/command.c (open_card_with_request): (cmd_pksign, cmd_pkauth, cmd_pkdecrypt): Use app_ref and app_unref instead of accessing the counter directly. -- This is better in case we need to debug stuff. There is a real change however: We now lock and unlock the app before changing the reference count. The whole app locking business should be reviewed because we pass pointers along without immediately bumping the refcount. Signed-off-by: Werner Koch --- scd/app-common.h | 6 +++++- scd/app.c | 48 ++++++++++++++++++++++++++++++++++++------------ scd/command.c | 21 +++++++++++---------- scd/scdaemon.h | 3 +++ 4 files changed, 55 insertions(+), 23 deletions(-) diff --git a/scd/app-common.h b/scd/app-common.h index cf51d26fe..8dc43285e 100644 --- a/scd/app-common.h +++ b/scd/app-common.h @@ -164,7 +164,11 @@ gpg_error_t select_application (ctrl_t ctrl, const char *name, app_t *r_app, int scan, const unsigned char *serialno_bin, size_t serialno_bin_len); char *get_supported_applications (void); -void release_application (app_t app, int locked_already); + +app_t app_ref (app_t app); +void app_unref (app_t app); +void app_unref_locked (app_t app); + gpg_error_t app_munge_serialno (app_t app); gpg_error_t app_write_learn_status (app_t app, ctrl_t ctrl, unsigned int flags); diff --git a/scd/app.c b/scd/app.c index 9640c8015..9055c9e8e 100644 --- a/scd/app.c +++ b/scd/app.c @@ -242,7 +242,7 @@ app_reset (app_t app, ctrl_t ctrl, int send_reset) else { ctrl->app_ctx = NULL; - release_application (app, 0); + app_unref (app); } return err; @@ -541,6 +541,7 @@ select_application (ctrl_t ctrl, const char *name, app_t *r_app, err = check_conflict (a, name); if (!err) { + /* Note: We do not use app_ref as we are already locked. */ a->ref_count++; *r_app = a; if (a_prev) @@ -604,7 +605,8 @@ deallocate_app (app_t app) a_prev = a; if (app->ref_count) - log_error ("trying to release context used yet (%d)\n", app->ref_count); + log_error ("trying to release still used app context (%d)\n", + app->ref_count); if (app->fnc.deinit) { @@ -618,13 +620,24 @@ deallocate_app (app_t app) xfree (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 deferring the deallocation to allow for a later reuse by - a new connection. */ + +/* Increment the reference counter for APP. Returns the APP. */ +app_t +app_ref (app_t app) +{ + lock_app (app, NULL); + ++app->ref_count; + unlock_app (app); + return app; +} + + +/* Decrement the reference counter for APP. Note that we are using + * reference counting to track the users of the application and are + * deferring the actual deallocation to allow for a later reuse by a + * new connection. Using NULL for APP is a no-op. */ void -release_application (app_t app, int locked_already) +app_unref (app_t app) { if (!app) return; @@ -634,15 +647,26 @@ release_application (app_t app, int locked_already) is using the card - this way the PIN cache and other cached data are preserved. */ - if (!locked_already) - lock_app (app, NULL); + lock_app (app, NULL); + if (!app->ref_count) + log_bug ("trying to release an already released context\n"); + --app->ref_count; + unlock_app (app); +} + + +/* This is the same as app_unref but assumes that APP is already + * locked. */ +void +app_unref_locked (app_t app) +{ + if (!app) + return; if (!app->ref_count) log_bug ("trying to release an already released context\n"); --app->ref_count; - if (!locked_already) - unlock_app (app); } diff --git a/scd/command.c b/scd/command.c index 9173a6843..426615c8b 100644 --- a/scd/command.c +++ b/scd/command.c @@ -234,7 +234,7 @@ open_card_with_request (ctrl_t ctrl, const char *apptype, const char *serialno) /* Re-scan USB devices. Release APP, before the scan. */ ctrl->app_ctx = NULL; - release_application (app, 0); + app_unref (app); if (serialno) serialno_bin = hex_to_buffer (serialno, &serialno_bin_len); @@ -804,14 +804,14 @@ cmd_pksign (assuan_context_t ctx, char *line) if (app) { if (direct) - app->ref_count++; + app_ref (app); rc = app_sign (app, ctrl, keyidstr, hash_algo, pin_cb, ctx, ctrl->in_data.value, ctrl->in_data.valuelen, &outdata, &outdatalen); if (direct) - app->ref_count--; + app_unref (app); } else rc = gpg_error (GPG_ERR_NO_SECKEY); @@ -872,12 +872,12 @@ cmd_pkauth (assuan_context_t ctx, char *line) if (app) { if (direct) - app->ref_count++; + app_ref (app); rc = app_auth (app, ctrl, keyidstr, pin_cb, ctx, ctrl->in_data.value, ctrl->in_data.valuelen, &outdata, &outdatalen); if (direct) - app->ref_count--; + app_unref (app); } else rc = gpg_error (GPG_ERR_NO_SECKEY); @@ -933,12 +933,12 @@ cmd_pkdecrypt (assuan_context_t ctx, char *line) if (app) { if (direct) - app->ref_count++; + app_ref (app); rc = app_decipher (ctrl->app_ctx, ctrl, keyidstr, pin_cb, ctx, ctrl->in_data.value, ctrl->in_data.valuelen, &outdata, &outdatalen, &infoflags); if (direct) - app->ref_count--; + app_unref (app); } else rc = gpg_error (GPG_ERR_NO_SECKEY); @@ -1648,7 +1648,7 @@ cmd_restart (assuan_context_t ctx, char *line) if (app) { ctrl->app_ctx = NULL; - release_application (app, 0); + app_unref (app); } if (locked_session && ctrl->server_local == locked_session) { @@ -2169,7 +2169,8 @@ popup_prompt (void *opaque, int on) } -/* Helper to send the clients a status change notification. */ +/* Helper to send the clients a status change notification. Note that + * this fucntion assumes that APP is already locked. */ void send_client_notifications (app_t app, int removal) { @@ -2199,7 +2200,7 @@ send_client_notifications (app_t app, int removal) { sl->ctrl_backlink->app_ctx = NULL; sl->card_removed = 1; - release_application (app, 1); + app_unref_locked (app); } if (!sl->event_signal || !sl->assuan_ctx) diff --git a/scd/scdaemon.h b/scd/scdaemon.h index 230653b11..7eb08a904 100644 --- a/scd/scdaemon.h +++ b/scd/scdaemon.h @@ -129,7 +129,10 @@ void send_keyinfo (ctrl_t ctrl, int data, const char *keygrip_str, const char *serialno, const char *idstr); void popup_prompt (void *opaque, int on); + +/* Take care: this function assumes that APP is locked. */ void send_client_notifications (app_t app, int removal); + void scd_kick_the_loop (void); int get_active_connection_count (void);