Race condition with SCardGetStatusChange() when USB Reader is removed

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.

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


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?

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.

_______________________________________________
Pcsclite-muscle mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

From db917fde6ee0ae166d891f615d3fb93ff09bd35f Mon Sep 17 00:00:00 2001
From: Maximilian Stein <[email protected]>
Date: Wed, 3 May 2017 13:43:56 +0200
Subject: [PATCH] Provoke race condition in SCardGetStatusChange()

---
 src/eventhandler.c  |    4 ++--
 src/readerfactory.c |    4 ++++
 src/winscard_clnt.c |    4 ++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/eventhandler.c b/src/eventhandler.c
index 4def507..8057366 100644
--- a/src/eventhandler.c
+++ b/src/eventhandler.c
@@ -467,10 +467,10 @@ static void EHStatusHandlerThread(READER_CONTEXT * rContext)
 
 			ret = rContext->pthCardEvent(rContext->slot, timeout);
 			if (IFD_SUCCESS != ret)
-				(void)SYS_USleep(PCSCLITE_STATUS_POLL_RATE);
+				(void)SYS_USleep(/*PCSCLITE_STATUS_POLL_RATE*/ 4*1000*1000); /* Wait 4s to enable race condition in SCardGetStatusChange */
 		}
 		else
-			(void)SYS_USleep(PCSCLITE_STATUS_POLL_RATE);
+			(void)SYS_USleep(/*PCSCLITE_STATUS_POLL_RATE*/ 4*1000*1000); /* Wait 4s to enable race condition in SCardGetStatusChange */
 
 #ifndef DISABLE_ON_DEMAND_POWER_ON
 		/* the card is powered but not used */
diff --git a/src/readerfactory.c b/src/readerfactory.c
index 4f97892..2cc120e 100644
--- a/src/readerfactory.c
+++ b/src/readerfactory.c
@@ -629,6 +629,10 @@ LONG removeReader(READER_CONTEXT * sContext)
 			return SCARD_E_INVALID_VALUE;
 		}
 
+		Log1(PCSC_LOG_DEBUG, "Wait 1s to provoke SCardGetStatusChange race condition");
+		SYS_USleep(1*1000*1000);
+		Log1(PCSC_LOG_DEBUG, "Waited 1s");
+
 		rv = RFUnInitializeReader(sContext);
 		if (rv != SCARD_S_SUCCESS)
 			return rv;
diff --git a/src/winscard_clnt.c b/src/winscard_clnt.c
index ef8ebcd..5563c35 100644
--- a/src/winscard_clnt.c
+++ b/src/winscard_clnt.c
@@ -2119,6 +2119,10 @@ LONG SCardGetStatusChange(SCARDCONTEXT hContext, DWORD dwTimeout,
 				if (rv != SCARD_S_SUCCESS)
 					goto end;
 
+				Log1(PCSC_LOG_DEBUG, "Wait 2s to provoke SCardGetStatusChange race condition");
+				SYS_USleep(2*1000*1000);
+				Log1(PCSC_LOG_DEBUG, "Waited 2s");
+
 				if (INFINITE != dwTimeout)
 				{
 					long int diff;
-- 
1.7.9.5