Skip to content

Commit

Permalink
Fix data races in EHStatusHandlerThread()
Browse files Browse the repository at this point in the history 
Problem:
The thread running EHStatusHandlerThread() will race all the
rContext->readerState->fields as well as the powerState and some other
flags. These flags are both consumed and set by the thread running the
ccid driver as well.

Solution:
Make the flow unidirectional*: the status thread will always set its own
copy and "stash" a unique copy ready to be consumed by the driver
thread. That copy is consumed and rcontext->readerState is updated in
RFCheckReaderStatus() and a few other functions in readerfactory.c.
Whenever a copy is consumed it is also freed. Note that even though this
adds dynamic allocation it's at most 2 instances of READER_STATE.

This PR uses the C11 _Atomic feature. I'm not very familiar with all the
compilers pcsc is built with, but if you deem this to be incompatible,
please review the overall approach and I can #ifdef and do some mutexes
for platforms which do not provide _Atomic

* the flow is unidirectional except for the readerName which is logged
  at the beginning of the status thread. That should not be a problem
  since the name is set from the driver thread before the status thread
  is spawned.

==================
WARNING: ThreadSanitizer: data race (pid=28786)
  Write of size 4 at 0x000000f55c10 by thread T3:
    #0 EHStatusHandlerThread <null> (pcscd+0x4c3aed)

  Previous read of size 4 at 0x000000f55c10 by main thread:
    #0 RFWaitForReaderInit <null> (pcscd+0x4d0e27)
    #1 main <null> (pcscd+0x4c70c6)

  Location is global 'readerStates' of size 2944 at 0x000000f55b60 (pcscd+0x000000f55c10)

  Thread T3 (tid=28790, running) created by main thread at:
    #0 pthread_create <null> (pcscd+0x42be9b)
    #1 ThreadCreate <null> (pcscd+0x4e1c79)
    #2 EHSpawnEventHandler <null> (pcscd+0x4c2e9f)
    #3 RFAddReader <null> (pcscd+0x4cac07)
    #4 HPAddDevice <null> (pcscd+0x4e0a2e)
    #5 HPScanUSB <null> (pcscd+0x4dfe65)
    #6 HPRegisterForHotplugEvents <null> (pcscd+0x4dfd3a)
    #7 main <null> (pcscd+0x4c705c)

SUMMARY: ThreadSanitizer: data race (/home/rousseau/sc/costa/PCSC/src/pcscd+0x4c3aed) in EHStatusHandlerThread
==================
  • Loading branch information
@andrei-datcu @LudovicRousseau
andrei-datcu authored and LudovicRousseau committed Dec 12, 2021
1 parent 4a2fb8f commit 8434617
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 40 deletions.
103 changes: 63 additions & 40 deletions src/eventhandler.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#include "config.h"
#include <stdatomic.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <errno.h>
Expand Down Expand Up @@ -212,6 +213,9 @@ void EHDestroyEventHandler(READER_CONTEXT * rContext)
/* Zero the thread */
rContext->pthThread = 0;

/* destroy any unconsumed state updates */
free(atomic_exchange(&rContext->ehThreadReaderState, NULL));

Log1(PCSC_LOG_INFO, "Thread stomped.");

return;
Expand Down Expand Up @@ -241,12 +245,25 @@ LONG EHSpawnEventHandler(READER_CONTEXT * rContext)
return SCARD_S_SUCCESS;
}

static void sendStateUpdate(READER_CONTEXT* rc, READER_STATE* s)
{
READER_STATE* sc = malloc(sizeof(*s));
if (!sc)
{
Log1(PCSC_LOG_ERROR, "malloc failed");
return;
}
memcpy(sc, s, sizeof(*s));
free(atomic_exchange(&rc->ehThreadReaderState, sc));
}

static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
{
LONG rv;
#ifndef NO_LOG
const char *readerName;
#endif
READER_STATE rState = {};
DWORD dwStatus;
uint32_t readerState;
int32_t readerSharing;
Expand All @@ -259,6 +276,7 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
* Zero out everything
*/
dwStatus = 0;
memset(&rState, 0, sizeof(rState));

#ifndef NO_LOG
readerName = rContext->readerState->readerName;
Expand All @@ -269,30 +287,30 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
if ((SCARD_S_SUCCESS == rv) && (dwStatus & SCARD_PRESENT))
{
#ifdef DISABLE_AUTO_POWER_ON
rContext->readerState->cardAtrLength = 0;
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.cardAtrLength = 0;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;
readerState = SCARD_PRESENT;
Log1(PCSC_LOG_INFO, "Skip card power on");
#else
dwAtrLen = sizeof(rContext->readerState->cardAtr);
dwAtrLen = sizeof(rState.cardAtr);
rv = IFDPowerICC(rContext, IFD_POWER_UP,
rContext->readerState->cardAtr, &dwAtrLen);
rContext->readerState->cardAtrLength = dwAtrLen;
rState.cardAtr, &dwAtrLen);
rState.cardAtrLength = dwAtrLen;

/* the protocol is unset after a power on */
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;

if (rv == IFD_SUCCESS)
{
readerState = SCARD_PRESENT | SCARD_POWERED | SCARD_NEGOTIABLE;
RFSetPowerState(rContext, POWER_STATE_POWERED);
Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_POWERED");

if (rContext->readerState->cardAtrLength > 0)
if (rState.cardAtrLength > 0)
{
LogXxd(PCSC_LOG_INFO, "Card ATR: ",
rContext->readerState->cardAtr,
rContext->readerState->cardAtrLength);
rState.cardAtr,
rState.cardAtrLength);
}
else
Log1(PCSC_LOG_INFO, "Card ATR: (NULL)");
Expand All @@ -311,18 +329,19 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
else
{
readerState = SCARD_ABSENT;
rContext->readerState->cardAtrLength = 0;
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.cardAtrLength = 0;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;

dwCurrentState = SCARD_ABSENT;
}

/*
* Set all the public attributes to this reader
*/
rContext->readerState->readerState = readerState;
rContext->readerState->readerSharing = readerSharing = rContext->contexts;
rState.readerState = readerState;
rState.readerSharing = readerSharing = rContext->contexts;

sendStateUpdate(rContext, &rState);
EHSignalEventToClients();

while (1)
Expand All @@ -338,12 +357,13 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
/*
* Set error status on this reader while errors occur
*/
rContext->readerState->readerState = SCARD_UNKNOWN;
rContext->readerState->cardAtrLength = 0;
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.readerState = SCARD_UNKNOWN;
rState.cardAtrLength = 0;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;

dwCurrentState = SCARD_UNKNOWN;

sendStateUpdate(rContext, &rState);
EHSignalEventToClients();
}

Expand All @@ -361,15 +381,16 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
*/
(void)RFSetReaderEventState(rContext, SCARD_REMOVED);

rContext->readerState->cardAtrLength = 0;
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rContext->readerState->readerState = SCARD_ABSENT;
rState.cardAtrLength = 0;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.readerState = SCARD_ABSENT;
dwCurrentState = SCARD_ABSENT;

rContext->readerState->eventCounter++;
if (rContext->readerState->eventCounter > 0xFFFF)
rContext->readerState->eventCounter = 0;
rState.eventCounter++;
if (rState.eventCounter > 0xFFFF)
rState.eventCounter = 0;

sendStateUpdate(rContext, &rState);
EHSignalEventToClients();
}

Expand All @@ -380,9 +401,9 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
dwCurrentState == SCARD_UNKNOWN)
{
#ifdef DISABLE_AUTO_POWER_ON
rContext->readerState->cardAtrLength = 0;
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rContext->readerState->readerState = SCARD_PRESENT;
rState.cardAtrLength = 0;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.readerState = SCARD_PRESENT;
RFSetPowerState(rContext, POWER_STATE_UNPOWERED);
Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_UNPOWERED");
rv = IFD_SUCCESS;
Expand All @@ -391,46 +412,47 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
/*
* Power and reset the card
*/
dwAtrLen = sizeof(rContext->readerState->cardAtr);
dwAtrLen = sizeof(rState.cardAtr);
rv = IFDPowerICC(rContext, IFD_POWER_UP,
rContext->readerState->cardAtr, &dwAtrLen);
rContext->readerState->cardAtrLength = dwAtrLen;
rState.cardAtr, &dwAtrLen);
rState.cardAtrLength = dwAtrLen;

/* the protocol is unset after a power on */
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;

if (rv == IFD_SUCCESS)
{
rContext->readerState->readerState = SCARD_PRESENT | SCARD_POWERED | SCARD_NEGOTIABLE;
rState.readerState = SCARD_PRESENT | SCARD_POWERED | SCARD_NEGOTIABLE;
RFSetPowerState(rContext, POWER_STATE_POWERED);
Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_POWERED");
}
else
{
rContext->readerState->readerState = SCARD_PRESENT | SCARD_SWALLOWED;
rState.readerState = SCARD_PRESENT | SCARD_SWALLOWED;
RFSetPowerState(rContext, POWER_STATE_UNPOWERED);
Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_UNPOWERED");
rContext->readerState->cardAtrLength = 0;
rState.cardAtrLength = 0;
}
#endif

dwCurrentState = SCARD_PRESENT;

rContext->readerState->eventCounter++;
if (rContext->readerState->eventCounter > 0xFFFF)
rContext->readerState->eventCounter = 0;
rState.eventCounter++;
if (rState.eventCounter > 0xFFFF)
rState.eventCounter = 0;

Log2(PCSC_LOG_INFO, "Card inserted into %s", readerName);

sendStateUpdate(rContext, &rState);
EHSignalEventToClients();

if (rv == IFD_SUCCESS)
{
if (rContext->readerState->cardAtrLength > 0)
if (rState.cardAtrLength > 0)
{
LogXxd(PCSC_LOG_INFO, "Card ATR: ",
rContext->readerState->cardAtr,
rContext->readerState->cardAtrLength);
rState.cardAtr,
rState.cardAtrLength);
}
else
Log1(PCSC_LOG_INFO, "Card ATR: (NULL)");
Expand All @@ -446,7 +468,8 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
if (readerSharing != rContext->contexts)
{
readerSharing = rContext->contexts;
rContext->readerState->readerSharing = readerSharing;
rState.readerSharing = readerSharing;
sendStateUpdate(rContext, &rState);
EHSignalEventToClients();
}

Expand Down Expand Up @@ -482,7 +505,7 @@ static void * EHStatusHandlerThread(READER_CONTEXT * rContext)
Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_UNPOWERED");

/* the protocol is unset after a power down */
rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
rState.cardProtocol = SCARD_PROTOCOL_UNDEFINED;
}

/* the card was in use */
Expand Down
26 changes: 26 additions & 0 deletions src/readerfactory.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#include "config.h"
#include <stdatomic.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
Expand Down Expand Up @@ -101,6 +102,27 @@ static int RDR_CLIHANDLES_seeker(const void *el, const void *key)
return 0;
}

static void fetchReaderState(READER_CONTEXT * sReader)
{
struct pubReaderStatesList* src;
src = atomic_exchange(&sReader->ehThreadReaderState, NULL);
if (!src)
{
return;
}

/* copy all the fields that may be updated on the event handler
* status thread */
struct pubReaderStatesList* dst = sReader->readerState;
dst->eventCounter = src->eventCounter;
dst->readerState = src->readerState;
dst->readerSharing = src->readerSharing;
memcpy(dst->cardAtr, src->cardAtr, sizeof(src->cardAtr));
dst->cardAtrLength = src->cardAtrLength;
dst->cardProtocol = src->cardProtocol;

free(src);
}

LONG _RefReader(READER_CONTEXT * sReader)
{
Expand Down Expand Up @@ -141,6 +163,7 @@ LONG RFAllocateReaderSpace(unsigned int customMaxReaderHandles)
{
sReadersContexts[i] = malloc(sizeof(READER_CONTEXT));
sReadersContexts[i]->vHandle = NULL;
atomic_init(&sReadersContexts[i]->ehThreadReaderState, NULL);

/* Zero out each value in the struct */
memset(readerStates[i].readerName, 0, MAX_READERNAME);
Expand Down Expand Up @@ -1366,6 +1389,7 @@ LONG RFClearReaderEventState(READER_CONTEXT * rContext, SCARDHANDLE hCard)

LONG RFCheckReaderStatus(READER_CONTEXT * rContext)
{
fetchReaderState(rContext);
if (rContext->readerState->readerState & SCARD_UNKNOWN)
return SCARD_E_READER_UNAVAILABLE;
else
Expand Down Expand Up @@ -1431,6 +1455,7 @@ void RFWaitForReaderInit(void)
/* reader is present */
if (sReadersContexts[i]->vHandle != NULL)
{
fetchReaderState(sReadersContexts[i]);
/* but card state is not yet available */
if (READER_NOT_INITIALIZED
== sReadersContexts[i]->readerState->cardAtrLength)
Expand Down Expand Up @@ -1536,6 +1561,7 @@ void RFReCheckReaderConf(void)
{
if (sReadersContexts[r]->vHandle != 0)
{
fetchReaderState(sReadersContexts[i]);
char lpcStripReader[MAX_READERNAME];
int tmplen;

Expand Down
2 changes: 2 additions & 0 deletions src/readerfactory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
pthread_mutex_t reference_lock; /**< reference mutex */

struct pubReaderStatesList *readerState; /**< link to the reader state */
/**< the latest state as updated in the event handler thread. you fetch, you free */
struct pubReaderStatesList * _Atomic ehThreadReaderState;
/* we can't use READER_STATE * here since eventhandler.h can't be
* included because of circular dependencies */
};
Expand Down

0 comments on commit 8434617

Please sign in to comment.