Handle short-lived removals of reader in reader driver for PC/SC #2803
Conversation
This was done only to reflect the change in the flags. Before this change, when the reader was removed, the Now the flags should change as It is reflected in the commit message now. Thanks for the review! |
So if I understand correctly, then using SCardStatus for removal/reinsertion may only work in pcsc-lite, right? |
I understood it as |
|
I also measured possible time overhead regarding the changes in PCSC calling. Using the |
|
@LudovicRousseau can you have a look if this looks reasonable to you? |
src/libopensc/reader-pcsc.c
Outdated
| /* Make sure to preserve the CARD_PRESENT flag if the reader was | ||
| * reattached and we called the refresh_attributes too recently */ | ||
| if (priv->reader_state.dwEventState & SCARD_STATE_PRESENT) { | ||
| reader->flags |= SC_READER_CARD_PRESENT; | ||
| } | ||
|
|
||
| rv = priv->gpriv->SCardStatus(priv->pcsc_card, NULL, &readers_len, &cstate, &prot, atr, &atr_len); | ||
| if (priv->pcsc_card != 0 |
There was a problem hiding this comment.
Maybe you can test if priv->pcsc_card != 0 before the call to SCardStatus() to note waste CPU cycles?
A hCard set to 0 is invalid for pcsc-lite https://github.com/LudovicRousseau/PCSC/blob/master/src/winscard.c#L1254-L1256. But I don't know for the other WinSCard implementations (Windows and macOS).
|
No objection. But I am not an OpenSC internals expert. I suggest to make some tests on all the platforms (GNU/Linux, macOS, Windows, etc.) supported by OpenSC to be sure to capture all the different behaviors of |
When the reader is removed and inserted back between two consecutive callings to SCardGetStatusChange(), the change is not reported. The card handle should be invalidated, which is detected by SCardStatus. When no reconnection is done afterwards, SCardGetStatusChange() will still return timeout and ard handle will be invalid.
When the removed reader is detected for the first time, set SC_READER_CARD_CHANGED denoting change. Remove the flag when the condition is hit again. Before this change, when reader was removed the reader->flags in refresh_attributes() was updated from 0x00000001 (SC_READER_CARD_PRESENT) to 0x00000020 (SC_READER_REMOVED). However, when the reader is still removed by another call to refresh_attributes(), the flags remained as 0x00000020 (SC_READER_REMOVED). Now the flags should change with SC_READER_CARD_CHANGED.
When SCardGetStatusChange returns value successfully, remove also SC_READER_REMOVED before setting the current state.
When reader is removed and inserted back between two consecutive calls to PCSC SCardGetStatusChange() function, and no reconnection is done, the status will remain SC_READER_CARD_CHANGED.
The card_detect() can detect removed reader, it calls card_removed(), but card_detect_all() did not remove relation between slot and reader.
| DWORD readers_len = 0, cstate = 0, prot, atr_len = SC_MAX_ATR_SIZE; | ||
| unsigned char atr[SC_MAX_ATR_SIZE]; |
There was a problem hiding this comment.
nit: I would probably move these variables into the inner block where you use them.
| * remove only relation between reader and slot. */ | ||
| if (reader->flags & SC_READER_REMOVED) { | ||
| for (j = 0; j<list_size(&virtual_slots); j++) { | ||
| sc_pkcs11_slot_t *slot = (sc_pkcs11_slot_t *) list_get_at(&virtual_slots, j); |
There was a problem hiding this comment.
there are few minor formatting suggests from clang-format, that I would like to bring in too.
As discussed in #2740, the
refresh_attributes()function inreader-pcsc.cdoes not correctly handle the state when the reader is removed and inserted between two subsequent calls torefresh_attributes(). In that case, the PCSCSCardGetStatusChange()returnsSCARD_E_TIMEOUT, handled as the card is present and did not change.In #2740 (comment), there was proposed to use one more call to
SCardBeginTransaction(), it seems to be also working withSCardStatus()- when timeout is received, the invalid card handle should signalize recently removed reader andSC_READER_CARD_CHANGEDis set.The flag
SC_READER_CARD_CHANGEDis also set whenSCARD_E_UNKNOWN_READERis first received fromSCardGetStatusChange().The changes regarding slot handling remove the infinite loop from
card_detect()whenSC_READER_CARD_CHANGEDis repeatedly acquired. It happened when the reader was removed & inserted back, so the timeout was reached, but no reconnection between therefresh_attributes()inpcsc-reader.chappens - the card handle is invalid, so the flag about changed is repeatedly set. This should probably be handled differently - any insight would be appreciated.The changes were tested with Yubikey according to #2740 (comment).