-
-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
No data races in EHStatusHandlerThread #112
Conversation
Do you have a sample code that demonstrate the problem? |
I've tried providing a sample code with sleep()s to try and reproduce the problem but unfortunately I'm not able to. Nonetheless, the races seemed pretty obvious to me. Some of the fixes here are leftovers from older incomplete patches:
Finally, all the races (and more) are reported if you build the daemon with TSAN. |
I guess TSAN is https://clang.llvm.org/docs/ThreadSanitizer.html |
Hey, so sorry for not being explicit:
|
I can reproduce the problems. |
Hey, @LudovicRousseau does the patch I provided have any chance of being accepted? Anything I can help with? |
This will help fix potential race conditions. I was not able to generate a problem using ThreadSanitizer but that is not a proof that the code is correct https://clang.llvm.org/docs/ThreadSanitizer.html Thanks to andrei-datcu for the patch "No data races in EHStatusHandlerThread #112" #112
I already pushed parts of your changes. |
12e94ee
to
4d8ed49
Compare
@LudovicRousseau Done |
You can rebase again. |
4d8ed49
to
c8c7d7c
Compare
I applied your patch. But all the problems are not fixed.
|
You can get the current version of the code at http://ludovic.rousseau.free.fr/softwares/pcsc-lite/pcsc-lite-1.9.5-1e15010.tar.bz2 |
"volatile" does not help solve data races in multi-thread programs. See 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. " We must use "_Atomic" from C11 instead. Thanks to andrei-datcu for the patch "No data races in EHStatusHandlerThread #112" #112
@andrei-datcu do you plan to work on a better patch? |
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. |
Maybe I missed something. What are the 2 threads and what "same flags" are you referring to? |
Problem:
The thread running
EHStatusHandlerThread
will race all therContext->readerState->fields
as well as thepowerState
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 inRFCheckReaderState
and a few other functions inreaderfactory.c
. Whenever a copy is consumed it is also freed. Note that even though this adds dynamic allocation it's at most 2 instances ofREADER_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