<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2017-05-03 14:42 GMT+02:00 Maximilian Stein <span dir="ltr"><<a href="mailto:maximilian.stein@secunet.com" target="_blank">maximilian.stein@secunet.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br></blockquote><div><br></div><div>Hello,<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
it is possible that a client application listening for events via<br>
SCardGetStatusChange() is not informed that an USB reader was removed,<br>
thus blocking until given or internal time-out.<br></blockquote><div><br></div><div>Yes. It is possible.<br></div><div>pcsc-lite may contain bugs.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Summary:<br>
Upon USB reader removal the first thing to happen is that the readers<br>
event handling thread is stopped. The last thing it does before exiting<br>
is to signal the event to the listening clients, but at this point the<br>
readerStates structure of the reader is not changed / uninitialized. The<br>
client receives the event notification but can detect no changes. This<br>
behaviour depends on the timing of both the client application and<br>
pcscd, thus it's a bit tricky to figure out.<br>
<br>
<br>
Long version:<br>
The removal of an USB reader is detected by the hotplug monitoring<br>
mechanism which then calls RFRemoveReader() [readerfactory.c].<br>
RFRemoveReader() boils down to removeReader(), which first stops the<br>
event handling thread of the reader (EHDestroyEventHandler() in<br>
[eventhandler.c]) and then frees / reinitialises the READER_STATE and<br>
READER_CONTEXT structs of the reader.<br>
<br>
EHDestroyEventHandler() sets LockId of the reader context so that the<br>
event handler thread (EHStatusHandlerThread()) will exit at the end of<br>
the loop. If the reader was removed *after a successful* call to<br>
IFDStatusICC(), e.g. while sleeping for PCSCLITE_STATUS_POLL_RATE, the<br>
global readerStates struct contains valid *unchanged* information.<br>
Before the thread exits it calls EHSignalEventToClients() but the client<br>
will not detect any event, because readerStates is unchanged.<br>
<br>
Inside SCardGetStatusChange() [winscard_clnt.c] the client receives the<br>
event notification and wakes up. It immediately calls getReaderStates()<br>
which synchronizes the global readerStates structs. Since there was no<br>
change on the server side yet, the client detects no events. As a<br>
result, the client stays inside the SCardGetStatusChange() loop and<br>
listens again for an event notification. *But* if checking the<br>
readerStates takes a bit longer, the client is not fast enough to listen<br>
for one more very important event: There is another event notification<br>
on USB reader removal at the end of RFRemoveReader(). Then all status<br>
structs for the reader are initialized again and<br>
EHSignalEventToClients() is called. A client that receives this event<br>
notification will notice that the reader disappeared. But if the client<br>
is not fast enough to start listening for events again (as mentioned<br>
above), no change is detected. This results in the client missing the<br>
change and waiting for more event notifications until time-out (which<br>
can be set to INFINITE) is reached.<br>
<br>
Attached you can find a patch that provokes this behaviour<br>
[Provoke-race-condition-in-<wbr>SCardGetStatusChange.patch]. To reproduce you<br>
need to attach an USB reader, then call a client application that calls<br>
SCardGetStatusChange() for the PnP-Notification and remove the reader.<br>
<br>
<br>
Suggestions for a fix:<br>
1) Remove EHSignalEventToClient() when EHStatusHandlerThread() exits.<br>
First at this point there is no updated data visible to the client.<br>
Second an event notification will be given at the end of RFRemoveReader().<br>
2) Clean up readerState - or set error status as if IFDStatusICC()<br>
failed - when EHStatusHandlerThread() exits, before calling<br>
EHSignalEventToClient().<br>
<br>
<br>
Best regards<br>
Maximilian<br></blockquote><div><br></div><div>I tried to reproduce the problem with the attached sample code but without success.<br></div><div>I tried using the special reader "\\?PnP?\Notification" and also using the current reader name but could not reproduce the problem. Yes, I first applied your patch and I get the extra sleep() in pcscd.<br><br></div><div>You can change line 52 of my sample code to use the PnP reader or the normal one.<br><br></div><div>Can you provide a/your sample code to reproduce the problem?<br><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
P.S. RFUnInitializeReader() always returns success, so the check for (rv<br>
!= SCARD_S_SUCCESS) in removeReader() [readerfactory.c] is a bit<br>
confusing and could be removed?<br></blockquote><div><br></div><div>Correct.<br>Fixed in <a href="https://github.com/LudovicRousseau/PCSC/commit/789dae8a8acf8d1693597159d7d590a96bdd6424">https://github.com/LudovicRousseau/PCSC/commit/789dae8a8acf8d1693597159d7d590a96bdd6424</a><br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
P.P.S. In ContextThread() [winscard_svc.c], sending readerStates to the<br>
client in CMD_GET_READER_STATES is prone to a race condition (even if<br>
very unlikely). The readerStates array is not protected by a mutex, so<br>
it can be changed while it is copied/sent to the client socket. But I'm<br>
uncertain if this would ever result in any problematic inconsistencies.<br></blockquote></div><br clear="all"></div><div class="gmail_extra">Exact.<br>readerStates may be inaccurate or even incoherent (like the reader name) when sent to the client.<br><br></div><div class="gmail_extra">From "Atomic Types" [1]:<br>" In practice, you can assume that <code>int</code> is atomic.
You can also assume that pointer
types are atomic; that is very convenient.  Both of these assumptions
are true on all of the machines that the GNU C Library supports and on
all POSIX systems we know of. "<br><br></div><div class="gmail_extra">The 2 only fields in READER_STATE structure that are not int types are readerName[] and cardAtr[]. These fields should not change often.<br><br></div><div class="gmail_extra">I do not think this is a real problem. But I still created a bug at [2] so I (or someone else) can work on time when time permit.<br><br></div><div class="gmail_extra">Regards,<br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><br>[1] <a href="https://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html">https://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html</a><br>[2] <a href="https://alioth.debian.org/tracker/index.php?func=detail&aid=315725&group_id=30105&atid=410085">https://alioth.debian.org/tracker/index.php?func=detail&aid=315725&group_id=30105&atid=410085</a><br><br></div><div class="gmail_extra">-- <br><div class="gmail_signature"> Dr. Ludovic Rousseau</div>
</div></div>