Commit 984f84df authored by Ludovic Rousseau's avatar Ludovic Rousseau

Fix a rare race condition in SCardGetStatusChange()

Thanks to Maximilian Stein for the patch

[Pcsclite-muscle] Rare race condition in SCardGetStatusChange()
http://lists.infradead.org/pipermail/pcsclite-muscle/2018-April/001068.html

" Hello all,

recently we stumbled upon another (rare) race condition in the
SCardGetStatusChange() function. I especially ask Maksim Ivanov and
Florian Kaiser kindly to share their opinion about the problem and the
proposed fix, since they did a good job in improving
SCardGetStatusChange/SCardCancel in the recent past.

The problem:
The problem is basically the small gap between fetching the reader
states from pcscd [winscard_clnt.c:1741 and 2119] and registering for
event notifications [winscard_clnt.c:2070]. If a notification is sent in
this time period, SCardGetStatusChange() will sleep until another event
or the internal time-out is reached. Consider the following flow of
execution and events:

Precondition: [client] knows the current state of all readers from a
previous call to SCardGetStatusChange() and no changes happened since
then. Basically consider a thread that monitors the reader states which
calls SCardGetStatusChange() in a loop.
1. [client] calls SCardGetStatusChange() and fetches the reader states
from pcscd (line 1741). The fetched states are equal to the already
known states.
2. [pcscd] notices a change in any terminal state (smartcard movement or
number of connected clients to the smartcard changes) and sends a signal
to all *registered* clients via EHSignalEventToClients(). [client] is
not yet registered for event notifications, so EHSignalEventToClients()
won't send anything.
3. [client] continues execution and compares the given reader states to
the states fetched before [pcscd] noticed the change. The states are
equal so it registers for event notifications (line 2070). No
notification was sent, so [client] sleeps until the internal time-out
(60 seconds) is reached.
4. [client] reaches the internal time-out and fetches the current reader
states from [pcscd]. Now the states are different and
SCardGetStatusChange returns.

In the end the state change is noticed thanks to the internal polling
mechanism, but in automated environments, 60 seconds is a very long time
until a state change is detected. The error is most likely to occur if
multiple readers are used and state changes are frequent. Even more
likely it occurs if reader state polling is used by pcscd, i.e. when the
IFD handler does not support asynchronous card event notification (no
capability TAG_IFD_POLLING_THREAD_WITH_TIMEOUT). Then multiple readers
can accumulate their events to be processed in a very small time frame.

Suggestion for a fix:
The proposed fix makes fetching the reader states and registering for
event notifications an atomic action. The command
CMD_WAIT_READER_STATE_CHANGE expected no return value anyway, so I made
it return the reader states equal to CMD_GET_READERS_STATE. The action
is protected by the ClientsWaitingForEvent_lock in eventhandler.c, which
prevents parallel calls of MSGSignalClient() via
EHSignalEventToClients(). This is necessary to prevent a signal before
the reader states are sent, which would appear as garbage in the client
socket.

With the proposed fix, the client is registered for events after the
reader states were fetched. So if any difference is found in the local
and remote state (so that SCardGetStatusChange() returns) we have to
unregister from events. This was not necessary before, but works just
like unregistering after a timeout. This could be refined by checking
why the loop was exited and only unregister if necessary.

Unfortunately the proposed fix will slightly alter the internal protocol
between libpcsclite and pcscd, breaking statically linked client
applications with newer pcscd versions.

Further related thoughts:
I'm a bit uncertain if my proposed fix works nicely with SCardCancel(),
because I can think of one very rare situation when
SCardGetStatusChange() times out and unregisters from event
notifications, then gets cancelled but is not informed about that. Then
it re-registers for notifications, because no changes happened. Thus it
will not returned despite it was cancelled. But this should have been an
issue even before my fix.

I think the notification mechanism could be improved by using "response
headers" analogous to the server side, or just an additional field
"command" in the data structs. This way every message related to reader
state events could be identified by the client and handled respectively.
As I understand it, some of the past issues were because of signal,
cancel or stop-reader-state-change messages messing up the client socket
data. With a command field it can be decided what data the client
received, and what data is still expected to be received.

Best regards
Maximilian Stein "
parent 0127d3c7
......@@ -69,6 +69,8 @@ LONG EHRegisterClientForEvent(int32_t filedes)
(void)list_append(&ClientsWaitingForEvent, &filedes);
(void)MSGSendReaderStates(filedes);
(void)pthread_mutex_unlock(&ClientsWaitingForEvent_lock);
return SCARD_S_SUCCESS;
......
......@@ -388,6 +388,8 @@ static LONG SCardGetSetAttrib(SCARDHANDLE hCard, int command, DWORD dwAttrId,
LPBYTE pbAttr, LPDWORD pcbAttrLen);
static LONG getReaderStates(SCONTEXTMAP * currentContextMap);
static LONG getReaderStatesAndRegisterForEvents(SCONTEXTMAP * currentContextMap);
static LONG unregisterFromEvents(SCONTEXTMAP * currentContextMap);
/*
* Thread safety functions
......@@ -1736,7 +1738,7 @@ LONG SCardGetStatusChange(SCARDCONTEXT hContext, DWORD dwTimeout,
}
/* synchronize reader states with daemon */
rv = getReaderStates(currentContextMap);
rv = getReaderStatesAndRegisterForEvents(currentContextMap);
if (rv != SCARD_S_SUCCESS)
goto end;
......@@ -2056,24 +2058,16 @@ LONG SCardGetStatusChange(SCARDCONTEXT hContext, DWORD dwTimeout,
/* Only sleep once for each cycle of reader checks. */
{
struct wait_reader_state_change waitStatusStruct;
struct wait_reader_state_change waitStatusStruct = {0};
struct timeval before, after;
gettimeofday(&before, NULL);
waitStatusStruct.timeOut = dwTime;
waitStatusStruct.rv = SCARD_S_SUCCESS;
/* another thread can do SCardCancel() */
currentContextMap->cancellable = TRUE;
rv = MessageSendWithHeader(CMD_WAIT_READER_STATE_CHANGE,
currentContextMap->dwClientID,
sizeof(waitStatusStruct), &waitStatusStruct);
if (rv != SCARD_S_SUCCESS)
goto end;
/*
* Read a message from the server
*/
......@@ -2089,20 +2083,7 @@ LONG SCardGetStatusChange(SCARDCONTEXT hContext, DWORD dwTimeout,
if (SCARD_E_TIMEOUT == rv)
{
/* ask server to remove us from the event list */
rv = MessageSendWithHeader(CMD_STOP_WAITING_READER_STATE_CHANGE,
currentContextMap->dwClientID,
sizeof(waitStatusStruct), &waitStatusStruct);
if (rv != SCARD_S_SUCCESS)
goto end;
/* Read a message from the server */
rv = MessageReceive(&waitStatusStruct,
sizeof(waitStatusStruct),
currentContextMap->dwClientID);
if (rv != SCARD_S_SUCCESS)
goto end;
rv = unregisterFromEvents(currentContextMap);
}
if (rv != SCARD_S_SUCCESS)
......@@ -2116,7 +2097,7 @@ LONG SCardGetStatusChange(SCARDCONTEXT hContext, DWORD dwTimeout,
}
/* synchronize reader states with daemon */
rv = getReaderStates(currentContextMap);
rv = getReaderStatesAndRegisterForEvents(currentContextMap);
if (rv != SCARD_S_SUCCESS)
goto end;
......@@ -2148,6 +2129,8 @@ LONG SCardGetStatusChange(SCARDCONTEXT hContext, DWORD dwTimeout,
end:
Log1(PCSC_LOG_DEBUG, "Event Loop End");
(void)unregisterFromEvents(currentContextMap);
(void)pthread_mutex_unlock(&currentContextMap->mMutex);
error:
......@@ -3549,3 +3532,49 @@ static LONG getReaderStates(SCONTEXTMAP * currentContextMap)
return SCARD_S_SUCCESS;
}
static LONG getReaderStatesAndRegisterForEvents(SCONTEXTMAP * currentContextMap)
{
int32_t dwClientID = currentContextMap->dwClientID;
LONG rv;
/* Get current reader states from server and register on event list */
rv = MessageSendWithHeader(CMD_WAIT_READER_STATE_CHANGE, dwClientID,
0, NULL);
if (rv != SCARD_S_SUCCESS)
return rv;
/* Read a message from the server */
rv = MessageReceive(&readerStates, sizeof(readerStates), dwClientID);
return rv;
}
static LONG unregisterFromEvents(SCONTEXTMAP * currentContextMap)
{
int32_t dwClientID = currentContextMap->dwClientID;
LONG rv;
struct wait_reader_state_change waitStatusStruct = {0};
/* ask server to remove us from the event list */
rv = MessageSendWithHeader(CMD_STOP_WAITING_READER_STATE_CHANGE,
dwClientID, 0, NULL);
if (rv != SCARD_S_SUCCESS)
return rv;
/* This message can be the response to
* CMD_STOP_WAITING_READER_STATE_CHANGE, an event notification or a
* cancel notification.
* The server side ensures, that no more messages will be sent to
* the client. */
rv = MessageReceive(&waitStatusStruct, sizeof(waitStatusStruct),
dwClientID);
if (rv != SCARD_S_SUCCESS)
return rv;
/* if we received a cancel event the return value will be set
* accordingly */
rv = waitStatusStruct.rv;
return rv;
}
......@@ -46,7 +46,7 @@ THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
/** Major version of the current message protocol */
#define PROTOCOL_VERSION_MAJOR 4
/** Minor version of the current message protocol */
#define PROTOCOL_VERSION_MINOR 3
#define PROTOCOL_VERSION_MINOR 4
/**
* @brief Information transmitted in \ref CMD_VERSION Messages.
......
......@@ -411,24 +411,21 @@ static void ContextThread(LPVOID newContext)
case CMD_WAIT_READER_STATE_CHANGE:
{
struct wait_reader_state_change waStr;
/* nothing to read */
READ_BODY(waStr);
#ifdef USE_USB
/* wait until all readers are ready */
RFWaitForReaderInit();
#endif
/* add the client fd to the list */
/* add the client fd to the list and dump the readers state */
EHRegisterClientForEvent(filedes);
/* We do not send anything here.
* Either the client will timeout or the server will
* answer if an event occurs */
}
break;
case CMD_STOP_WAITING_READER_STATE_CHANGE:
{
struct wait_reader_state_change waStr;
READ_BODY(waStr);
struct wait_reader_state_change waStr = {0};
/* remove the client fd from the list */
waStr.rv = EHUnregisterClientForEvent(filedes);
......@@ -810,7 +807,7 @@ exit:
LONG MSGSignalClient(uint32_t filedes, LONG rv)
{
uint32_t ret;
struct wait_reader_state_change waStr;
struct wait_reader_state_change waStr = {0};
Log2(PCSC_LOG_DEBUG, "Signal client: %d", filedes);
......@@ -820,6 +817,18 @@ LONG MSGSignalClient(uint32_t filedes, LONG rv)
return ret;
} /* MSGSignalClient */
LONG MSGSendReaderStates(uint32_t filedes)
{
uint32_t ret;
Log2(PCSC_LOG_DEBUG, "Send reader states: %d", filedes);
/* dump the readers state */
ret = MessageSend(readerStates, sizeof(readerStates), filedes);
return ret;
}
static LONG MSGAddContext(SCARDCONTEXT hContext, SCONTEXT * threadContext)
{
threadContext->hContext = hContext;
......
......@@ -45,5 +45,6 @@ THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
void ContextsDeinitialize(void);
LONG CreateContextThread(uint32_t *);
LONG MSGSignalClient(uint32_t filedes, LONG rv);
LONG MSGSendReaderStates(uint32_t filedes);
#endif
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment