From 019cb5e956d5433d55838101dae536b4e0bb2171 Mon Sep 17 00:00:00 2001 From: Maximilian Stein Date: Mon, 16 Apr 2018 15:51:24 +0000 Subject: [PATCH] SCardGetStatusChange(): Fix (rare) race condition In SCardGetStatusChange() there is a small gap between fetching the current reader states from pcscd and registering for event notifications. If an event occurrs exactly within this time, it will not be noticed by the client application until the internal timeout of SCardGetStatusChange() (60 seconds) has passed. --- src/eventhandler.c | 2 ++ src/winscard_clnt.c | 76 +++++++++++++++++++++++++++++++++++------------------ src/winscard_msg.h | 2 +- src/winscard_svc.c | 31 ++++++++++++++-------- src/winscard_svc.h | 1 + 5 files changed, 75 insertions(+), 37 deletions(-) diff --git a/src/eventhandler.c b/src/eventhandler.c index d407453..7f1d05f 100644 --- a/src/eventhandler.c +++ b/src/eventhandler.c @@ -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; diff --git a/src/winscard_clnt.c b/src/winscard_clnt.c index d822418..ee293e5 100644 --- a/src/winscard_clnt.c +++ b/src/winscard_clnt.c @@ -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(¤tContextMap->mMutex); error: @@ -3549,3 +3532,46 @@ 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); + if (rv != SCARD_S_SUCCESS) + return rv; + + return SCARD_S_SUCCESS; +} + +static LONG unregisterFromEvents(SCONTEXTMAP * currentContextMap) +{ + int32_t dwClientID = currentContextMap->dwClientID; + LONG rv; + struct wait_reader_state_change waitStatusStruct = {0}; + waitStatusStruct.rv = SCARD_S_SUCCESS; + + /* 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 notifiaction. 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; +} + diff --git a/src/winscard_msg.h b/src/winscard_msg.h index 3b721fc..33eaa41 100644 --- a/src/winscard_msg.h +++ b/src/winscard_msg.h @@ -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. diff --git a/src/winscard_svc.c b/src/winscard_svc.c index d6b3c7b..577d0c9 100644 --- a/src/winscard_svc.c +++ b/src/winscard_svc.c @@ -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; diff --git a/src/winscard_svc.h b/src/winscard_svc.h index 3705a33..bb52c59 100644 --- a/src/winscard_svc.h +++ b/src/winscard_svc.h @@ -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 -- 2.11.0