No data races in EHStatusHandlerThread

[andrei-datcu]

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 RFCheckReaderState 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.

[LudovicRousseau]

Do you have a sample code that demonstrate the problem?
You can instrument the code to add sleep() to make the race more easy to reproduce.

[andrei-datcu]

I've tried providing a sample code with sleep()s to try and reproduce the problem but unfortunately I'm not able to.
I've started looking into this since I've experienced some reports of the reader not being connected immediately after the daemon is started (with the reader being already connected).

Nonetheless, the races seemed pretty obvious to me. Some of the fixes here are leftovers from older incomplete patches:

1. powerState is read and set under a mutex in almost all places in the code (apart from the few I've changed in this PR). The mutex was already there, there were just some places that did not lock it.
2. hLockId was volatile explicitly because it was set and read from two different threads (check the commit message). volatile however does not help avoid UB when shared between threads.

Finally, all the races (and more) are reported if you build the daemon with TSAN.

[LudovicRousseau]

I guess TSAN is https://clang.llvm.org/docs/ThreadSanitizer.html
What is UB?

[andrei-datcu]

Hey, so sorry for not being explicit:

• TSAN is thread sanitizer. you can compile with clang -fsanitize=thread -fsanitize-blacklist=path/to/blacklist.txt. blacklist.txt can contain functions and files which you want to suppress the warning for. You can find examples in the documentation (I suppress everything for libusb because most of those seem to be false positives)
• UB is undefined behavior and the note about volatile and threading can be found here: https://en.cppreference.com/w/c/language/volatile

Note that volatile variables are not suitable for communication between threads; they do not offer atomicity, synchronization, or memory ordering. A read from a volatile variable that is modified by another thread without synchronization or concurrent modification from two unsynchronized threads is undefined behavior due to a data race.

[LudovicRousseau]

I can reproduce the problems.
I will work on them.

[andrei-datcu]

Hey, @LudovicRousseau does the patch I provided have any chance of being accepted? Anything I can help with?

[LudovicRousseau]

I already pushed parts of your changes.
Please rebase on current master.

[andrei-datcu]

@LudovicRousseau Done
Please let me know if there's anything I can do to help with the other part of the patch as well. Thanks!

[LudovicRousseau]

You can rebase again.
See also http:https://lists.infradead.org/pipermail/pcsclite-muscle/2021-November/001214.html

[LudovicRousseau]

I applied your patch. But all the problems are not fixed.
For example when I remove a reader while pcsc_scan is running:

WARNING: ThreadSanitizer: data race (pid=13085)
  Write of size 8 at 0x000000f3bac0 by thread T4:
    #0 memset <null> (pcscd+0x435749)
    #1 RFUnInitializeReader PCSC/src/readerfactory.c:1188:2 (pcscd+0x4c421f)
    #2 removeReader PCSC/src/readerfactory.c:676:2 (pcscd+0x4c1474)
    #3 _UnrefReader PCSC/src/readerfactory.c:150:3 (pcscd+0x4c13e2)
    #4 RFRemoveReader PCSC/src/readerfactory.c:655:5 (pcscd+0x4c2fa8)
    #5 HPRemoveDevice PCSC/src/hotplug_libudev.c:370:4 (pcscd+0x4cbe95)
    #6 HPEstablishUSBNotifications PCSC/src/hotplug_libudev.c:662:6 (pcscd+0x4cb488)

  Previous read of size 8 at 0x000000f3bac0 by thread T9 (mutexes: write M5):
    #0 send <null> (pcscd+0x458335)
    #1 MessageSend PCSC/src/winscard_msg.c:396:14 (pcscd+0x4cf362)
    #3 MSGSendReaderStates PCSC/src/winscard_svc.c:843:8 (pcscd+0x4d114e)
    #3 EHRegisterClientForEvent PCSC/src/eventhandler.c:73:8 (pcscd+0x4be668)
    #4 ContextThread PCSC/src/winscard_svc.c:424:5 (pcscd+0x4d036f)

  Location is global 'readerStates' of size 2944 at 0x000000f3baa0 (pcscd+0x000000f3bac0)

  Mutex M5 (0x000000f3b968) created at:
    #0 pthread_mutex_init <null> (pcscd+0x42d2fd)
    #1 EHInitializeEventStructures PCSC/src/eventhandler.c:146:8 (pcscd+0x4be813)
    #2 RFAllocateReaderSpace PCSC/src/readerfactory.c:181:9 (pcscd+0x4c17ec)
    #3 main PCSC/src/pcscdaemon.c:637:7 (pcscd+0x4c05e5)

  Thread T4 (tid=13091, running) created by main thread at:
    #0 pthread_create <null> (pcscd+0x42be8b)
    #1 ThreadCreate PCSC/src/utils.c:184:8 (pcscd+0x4cc2cc)
    #2 HPRegisterForHotplugEvents PCSC/src/hotplug_libudev.c:784:6 (pcscd+0x4cb1b6)
    #3 main PCSC/src/pcscdaemon.c:753:7 (pcscd+0x4c08dd)

  Thread T9 (tid=13227, running) created by main thread at:
    #0 pthread_create <null> (pcscd+0x42be8b)
    #1 ThreadCreate PCSC/src/utils.c:184:8 (pcscd+0x4cc2cc)
    #2 CreateContextThread PCSC/src/winscard_svc.c:236:7 (pcscd+0x4cfd7b)
    #3 SVCServiceRunLoop PCSC/src/pcscdaemon.c:134:9 (pcscd+0x4c1082)
    #4 main PCSC/src/pcscdaemon.c:786:2 (pcscd+0x4c09c4)

SUMMARY: ThreadSanitizer: data race (PCSC/src/pcscd+0x435749) in memset

[LudovicRousseau]

You can get the current version of the code at http:https://ludovic.rousseau.free.fr/softwares/pcsc-lite/pcsc-lite-1.9.5-1e15010.tar.bz2
Maybe a mutex to protect accesses to readerStates[] is unavoidable.

[LudovicRousseau]

@andrei-datcu do you plan to work on a better patch?

[andrei-datcu]

No. This patch is worse than I initially expected as it even breaks some of the existing functionality. The problem imho is a bit deeper and much harder to solve. Sure, you can throw in a mutex and call it a day: no more ASAN warnings, but there will still be logical races, since you have two threads writing over the same flags. This is bound to offer races no matter what. Sorry for taking up your time.

[LudovicRousseau]

Maybe I missed something. What are the 2 threads and what "same flags" are you referring to?