Page MenuHome GnuPG

Keep holding READER_LOCK_TABLE and make clear distinction among close/releasing_PCSC_context/nullify_rdrname
Needs ReviewPublic

Authored by gniibe on May 7 2021, 6:51 AM.

Details

Reviewers
werner
Summary

There are three things, which is done by current close_pcsc_reader.

  • as a method for closing for a card reader
  • releasing PC/SC context by pcsc_release_context
  • nullify (clear) pcsc.rdrname fields.

pcsc.rdrname fields are set, to point reader name string which is gotten by pcsc_reader_list, and it is nullified by apdu_dev_list_finish (after freeing dl->table), even if there are still active/opened accesses to readers.

Test Plan

Test on GNU/Linux.
Test on Windows.

Diff Detail

Repository
rG GnuPG
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gniibe requested review of this revision.May 7 2021, 6:51 AM
gniibe created this revision.
werner requested changes to this revision.May 7 2021, 8:36 AM
werner added a subscriber: werner.

Keeping the lock over the call to the function does not look very robust to me. This is why I removed it. And since then PC/SC worked on Windows for me. Modulo this:
All these changes don't tackle the real problem that windows gets struck in a removed-card state.

scd/apdu.c
843

That change re-introduces the original problem. Well, probably not because your other changes take care that the counter is not already at zero. However, this is too easy to get wrong, so a log_assert (pcsc.count > 0) would be appreciated.

This revision now requires changes to proceed.May 7 2021, 8:36 AM

OK. As I pointed out a commit having multiple things may make analysis difficult, I should have been careful.
So, let me fix the problem by multiple commits.

The first commit is adding log_assert and replacing one use case of close_pc_reader (at two places) into new function release_pcsc_context.

The second commit is replacing a use case of close_pcsc_reader by clearing pcsc.rdrname and calling release_pcsc_context.
This makes the use of close_pcsc_reader to its original purpose only (== closing PC/SC reader as a method of close_reader).

Last commit will be:

diff --git a/scd/apdu.c b/scd/apdu.c
index c809e6791..41c06e0ab 100644
--- a/scd/apdu.c
+++ b/scd/apdu.c
@@ -2094,12 +2094,8 @@ 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;
 }
@@ -2123,10 +2119,11 @@ apdu_dev_list_finish (struct dev_list *dl)
         pcsc.rdrname[i] = NULL;
 
       log_assert (pcsc.count > 0);
-      if (--pcsc.count == 0)
+      if (pcsc.count == 0)
         release_pcsc_context ();
     }
   xfree (dl);
+  npth_mutex_unlock (&reader_table_lock);
 }

And if the coding style of hiding mutex_lock/mutex_unlock inside different functions matters, we can expose the mutex to its user.

But there is only a single use of apdu_dev_list_start/finish. So, I'm not sure exposing is worth to do.

Exposing reader_table_lock would be better.
I found a dead-lock condition when apdu_close_reader is called during apdu_dev_list_start/finish.

gniibe added inline comments.
scd/apdu.c
843

Assertions are added.

gniibe marked an inline comment as done.

Make the lock holding narrower, and it allows no exposing reader_table_lock.

We should add a comment at the caller side, that this takes a lock in apdu.c.

Please note that we don't use lock in apdu_dev_list_start/finish any more.
Use of lock is narrowed, only within apdu_open_reader function.