[Pcsclite-muscle] Race condition with SCardGetStatusChange() when USB Reader is removed

Thu May 4 01:17:23 PDT 2017

2017-05-03 14:42 GMT+02:00 Maximilian Stein <maximilian.stein at secunet.com>:

> Hello,
>

Hello,

>
> it is possible that a client application listening for events via
> SCardGetStatusChange() is not informed that an USB reader was removed,
> thus blocking until given or internal time-out.
>

Yes. It is possible.
pcsc-lite may contain bugs.

>
> Summary:
> Upon USB reader removal the first thing to happen is that the readers
> event handling thread is stopped. The last thing it does before exiting
> is to signal the event to the listening clients, but at this point the
> readerStates structure of the reader is not changed / uninitialized. The
> client receives the event notification but can detect no changes. This
> behaviour depends on the timing of both the client application and
> pcscd, thus it's a bit tricky to figure out.
>
>
> Long version:
> The removal of an USB reader is detected by the hotplug monitoring
> mechanism which then calls RFRemoveReader() [readerfactory.c].
> RFRemoveReader() boils down to removeReader(), which first stops the
> event handling thread of the reader (EHDestroyEventHandler() in
> [eventhandler.c]) and then frees / reinitialises the READER_STATE and
> READER_CONTEXT structs of the reader.
>
> EHDestroyEventHandler() sets LockId of the reader context so that the
> event handler thread (EHStatusHandlerThread()) will exit at the end of
> the loop. If the reader was removed *after a successful* call to
> IFDStatusICC(), e.g. while sleeping for PCSCLITE_STATUS_POLL_RATE, the
> global readerStates struct contains valid *unchanged* information.
> Before the thread exits it calls EHSignalEventToClients() but the client
> will not detect any event, because readerStates is unchanged.
>
> Inside SCardGetStatusChange() [winscard_clnt.c] the client receives the
> event notification and wakes up. It immediately calls getReaderStates()
> which synchronizes the global readerStates structs. Since there was no
> change on the server side yet, the client detects no events. As a
> result, the client stays inside the SCardGetStatusChange() loop and
> listens again for an event notification. *But* if checking the
> readerStates takes a bit longer, the client is not fast enough to listen
> for one more very important event: There is another event notification
> on USB reader removal at the end of RFRemoveReader(). Then all status
> structs for the reader are initialized again and
> EHSignalEventToClients() is called. A client that receives this event
> notification will notice that the reader disappeared. But if the client
> is not fast enough to start listening for events again (as mentioned
> above), no change is detected. This results in the client missing the
> change and waiting for more event notifications until time-out (which
> can be set to INFINITE) is reached.
>
> Attached you can find a patch that provokes this behaviour
> [Provoke-race-condition-in-SCardGetStatusChange.patch]. To reproduce you
> need to attach an USB reader, then call a client application that calls
> SCardGetStatusChange() for the PnP-Notification and remove the reader.
>
>
> Suggestions for a fix:
> 1) Remove EHSignalEventToClient() when EHStatusHandlerThread() exits.
> First at this point there is no updated data visible to the client.
> Second an event notification will be given at the end of RFRemoveReader().
> 2) Clean up readerState - or set error status as if IFDStatusICC()
> failed - when EHStatusHandlerThread() exits, before calling
> EHSignalEventToClient().
>
>
> Best regards
> Maximilian
>

I tried to reproduce the problem with the attached sample code but without
success.
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.

You can change line 52 of my sample code to use the PnP reader or the
normal one.

Can you provide a/your sample code to reproduce the problem?

P.S. RFUnInitializeReader() always returns success, so the check for (rv
> != SCARD_S_SUCCESS) in removeReader() [readerfactory.c] is a bit
> confusing and could be removed?
>

Correct.
Fixed in
https://github.com/LudovicRousseau/PCSC/commit/789dae8a8acf8d1693597159d7d590a96bdd6424

>
> P.P.S. In ContextThread() [winscard_svc.c], sending readerStates to the
> client in CMD_GET_READER_STATES is prone to a race condition (even if
> very unlikely). The readerStates array is not protected by a mutex, so
> it can be changed while it is copied/sent to the client socket. But I'm
> uncertain if this would ever result in any problematic inconsistencies.
>

Exact.
readerStates may be inaccurate or even incoherent (like the reader name)
when sent to the client.

>From "Atomic Types" [1]:
" In practice, you can assume that int 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. "

The 2 only fields in READER_STATE structure that are not int types are
readerName[] and cardAtr[]. These fields should not change often.

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.

Regards,

[1] https://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html
[2]
https://alioth.debian.org/tracker/index.php?func=detail&aid=315725&group_id=30105&atid=410085

-- 
 Dr. Ludovic Rousseau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20170504/e0eeeac9/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: SCardGetStatusChange4.py
Type: text/x-python
Size: 3078 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20170504/e0eeeac9/attachment-0001.py>