Race condition with SCardGetStatusChange() when USB Reader is removed

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Race condition with SCardGetStatusChange() when USB Reader is removed

Stein, Maximilian
Hello,

it is possible that a client application listening for events via
SCardGetStatusChange() is not informed that an USB reader was removed,
thus blocking until given or internal time-out.

Summary:
Upon USB reader removal the first thing to happen is that the readers
event handling thread is stopped. The last thing it does before exiting
is to signal the event to the listening clients, but at this point the
readerStates structure of the reader is not changed / uninitialized. The
client receives the event notification but can detect no changes. This
behaviour depends on the timing of both the client application and
pcscd, thus it's a bit tricky to figure out.


Long version:
The removal of an USB reader is detected by the hotplug monitoring
mechanism which then calls RFRemoveReader() [readerfactory.c].
RFRemoveReader() boils down to removeReader(), which first stops the
event handling thread of the reader (EHDestroyEventHandler() in
[eventhandler.c]) and then frees / reinitialises the READER_STATE and
READER_CONTEXT structs of the reader.

EHDestroyEventHandler() sets LockId of the reader context so that the
event handler thread (EHStatusHandlerThread()) will exit at the end of
the loop. If the reader was removed *after a successful* call to
IFDStatusICC(), e.g. while sleeping for PCSCLITE_STATUS_POLL_RATE, the
global readerStates struct contains valid *unchanged* information.
Before the thread exits it calls EHSignalEventToClients() but the client
will not detect any event, because readerStates is unchanged.

Inside SCardGetStatusChange() [winscard_clnt.c] the client receives the
event notification and wakes up. It immediately calls getReaderStates()
which synchronizes the global readerStates structs. Since there was no
change on the server side yet, the client detects no events. As a
result, the client stays inside the SCardGetStatusChange() loop and
listens again for an event notification. *But* if checking the
readerStates takes a bit longer, the client is not fast enough to listen
for one more very important event: There is another event notification
on USB reader removal at the end of RFRemoveReader(). Then all status
structs for the reader are initialized again and
EHSignalEventToClients() is called. A client that receives this event
notification will notice that the reader disappeared. But if the client
is not fast enough to start listening for events again (as mentioned
above), no change is detected. This results in the client missing the
change and waiting for more event notifications until time-out (which
can be set to INFINITE) is reached.

Attached you can find a patch that provokes this behaviour
[Provoke-race-condition-in-SCardGetStatusChange.patch]. To reproduce you
need to attach an USB reader, then call a client application that calls
SCardGetStatusChange() for the PnP-Notification and remove the reader.


Suggestions for a fix:
1) Remove EHSignalEventToClient() when EHStatusHandlerThread() exits.
First at this point there is no updated data visible to the client.
Second an event notification will be given at the end of RFRemoveReader().
2) Clean up readerState - or set error status as if IFDStatusICC()
failed - when EHStatusHandlerThread() exits, before calling
EHSignalEventToClient().


Best regards
Maximilian


P.S. RFUnInitializeReader() always returns success, so the check for (rv
!= SCARD_S_SUCCESS) in removeReader() [readerfactory.c] is a bit
confusing and could be removed?

P.P.S. In ContextThread() [winscard_svc.c], sending readerStates to the
client in CMD_GET_READER_STATES is prone to a race condition (even if
very unlikely). The readerStates array is not protected by a mutex, so
it can be changed while it is copied/sent to the client socket. But I'm
uncertain if this would ever result in any problematic inconsistencies.

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

Provoke-race-condition-in-SCardGetStatusChange.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Race condition with SCardGetStatusChange() when USB Reader is removed

Ludovic Rousseau
2017-05-03 14:42 GMT+02:00 Maximilian Stein <[hidden email]>:
Hello,

Hello,
 

it is possible that a client application listening for events via
SCardGetStatusChange() is not informed that an USB reader was removed,
thus blocking until given or internal time-out.

Yes. It is possible.
pcsc-lite may contain bugs.
 

Summary:
Upon USB reader removal the first thing to happen is that the readers
event handling thread is stopped. The last thing it does before exiting
is to signal the event to the listening clients, but at this point the
readerStates structure of the reader is not changed / uninitialized. The
client receives the event notification but can detect no changes. This
behaviour depends on the timing of both the client application and
pcscd, thus it's a bit tricky to figure out.


Long version:
The removal of an USB reader is detected by the hotplug monitoring
mechanism which then calls RFRemoveReader() [readerfactory.c].
RFRemoveReader() boils down to removeReader(), which first stops the
event handling thread of the reader (EHDestroyEventHandler() in
[eventhandler.c]) and then frees / reinitialises the READER_STATE and
READER_CONTEXT structs of the reader.

EHDestroyEventHandler() sets LockId of the reader context so that the
event handler thread (EHStatusHandlerThread()) will exit at the end of
the loop. If the reader was removed *after a successful* call to
IFDStatusICC(), e.g. while sleeping for PCSCLITE_STATUS_POLL_RATE, the
global readerStates struct contains valid *unchanged* information.
Before the thread exits it calls EHSignalEventToClients() but the client
will not detect any event, because readerStates is unchanged.

Inside SCardGetStatusChange() [winscard_clnt.c] the client receives the
event notification and wakes up. It immediately calls getReaderStates()
which synchronizes the global readerStates structs. Since there was no
change on the server side yet, the client detects no events. As a
result, the client stays inside the SCardGetStatusChange() loop and
listens again for an event notification. *But* if checking the
readerStates takes a bit longer, the client is not fast enough to listen
for one more very important event: There is another event notification
on USB reader removal at the end of RFRemoveReader(). Then all status
structs for the reader are initialized again and
EHSignalEventToClients() is called. A client that receives this event
notification will notice that the reader disappeared. But if the client
is not fast enough to start listening for events again (as mentioned
above), no change is detected. This results in the client missing the
change and waiting for more event notifications until time-out (which
can be set to INFINITE) is reached.

Attached you can find a patch that provokes this behaviour
[Provoke-race-condition-in-SCardGetStatusChange.patch]. To reproduce you
need to attach an USB reader, then call a client application that calls
SCardGetStatusChange() for the PnP-Notification and remove the reader.


Suggestions for a fix:
1) Remove EHSignalEventToClient() when EHStatusHandlerThread() exits.
First at this point there is no updated data visible to the client.
Second an event notification will be given at the end of RFRemoveReader().
2) Clean up readerState - or set error status as if IFDStatusICC()
failed - when EHStatusHandlerThread() exits, before calling
EHSignalEventToClient().


Best regards
Maximilian

I tried to reproduce the problem with the attached sample code but without success.
I tried using the special reader "\\?PnP?\Notification" and also using the current reader name but could not reproduce the problem. Yes, I first applied your patch and I get the extra sleep() in pcscd.

You can change line 52 of my sample code to use the PnP reader or the normal one.

Can you provide a/your sample code to reproduce the problem?


P.S. RFUnInitializeReader() always returns success, so the check for (rv
!= SCARD_S_SUCCESS) in removeReader() [readerfactory.c] is a bit
confusing and could be removed?


P.P.S. In ContextThread() [winscard_svc.c], sending readerStates to the
client in CMD_GET_READER_STATES is prone to a race condition (even if
very unlikely). The readerStates array is not protected by a mutex, so
it can be changed while it is copied/sent to the client socket. But I'm
uncertain if this would ever result in any problematic inconsistencies.

Exact.
readerStates may be inaccurate or even incoherent (like the reader name) when sent to the client.

From "Atomic Types" [1]:
" In practice, you can assume that int is atomic. You can also assume that pointer types are atomic; that is very convenient. Both of these assumptions are true on all of the machines that the GNU C Library supports and on all POSIX systems we know of. "

The 2 only fields in READER_STATE structure that are not int types are readerName[] and cardAtr[]. These fields should not change often.

I do not think this is a real problem. But I still created a bug at [2] so I (or someone else) can work on time when time permit.

Regards,

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

SCardGetStatusChange4.py (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Race condition with SCardGetStatusChange() when USB Reader is removed

Stein, Maximilian
> Yes. It is possible.
> pcsc-lite may contain bugs.

I'm sorry if that sounded offensive. Personally I appreciate the work
and effort you put into this project very much and merely want to
contribute to its further improvement.


> I tried to reproduce the problem with the attached sample code but
> without success.
> I tried using the special reader "\\?PnP?\Notification" and also using
> the current reader name but could not reproduce the problem. Yes, I
> first applied your patch and I get the extra sleep() in pcscd.
>
> You can change line 52 of my sample code to use the PnP reader or the
> normal one.

On my testing machine (Xubuntu 16.04 python-pyscard installed from
repos) I can reproduce the problem with the python Unit Tests and your
sample code, with any value in line 52. But on another installation of
Ubuntu 16.04 (self compiled pyscard) I can't reproduce it either.

My patch includes a change in the lipcsclite client library which is
very important to provoke the race condition. This change should produce
debug output ("Waiting 2s to provoke...") when executing the test
programs with env variable PCSCLITE_DEBUG=0.

Did you get this additional client side debug output?

On the Ubuntu 16.04 machine I don't get the additional debug output, so
it seems like the pyscard Python module is not using the currently
installed libpcsclite. This is very strange because there is only the
self compiled libpcsclite installed. And with a test program written in
C, the problem is reproducible (including debug output) on both systems.


> Can you provide a/your sample code to reproduce the problem?
>

My test program in C is attached and should compile like this:
$ gcc -I/usr/local/include/PCSC/ SCardGetStatusChange_Disconnect.c -o
SCardGetStatusChange_Disconnect -lpcsclite

There has to be exactly one reader connected before starting the
program. SCardGetStatusChange is called 2 times with reader states for
both the PnP Notification and the connected reader. First call is to get
the current reader state. The second call is blocking waiting for
events. This call stays blocked even if the observed reader is removed.


Thanks for you efforts. I'm sorry that I didn't include a sample code
for reproduction in the first place. I could reproduce it using the
provided Unit tests (e.g. SCardGetStatusChange_PnP.py) and considered
this should be enough.

Best regards
Maximilian

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

SCardGetStatusChange_Disconnect.c (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Race condition with SCardGetStatusChange() when USB Reader is removed

Ludovic Rousseau


2017-05-08 12:47 GMT+02:00 Maximilian Stein <[hidden email]>:
> Yes. It is possible.
> pcsc-lite may contain bugs.

I'm sorry if that sounded offensive. Personally I appreciate the work
and effort you put into this project very much and merely want to
contribute to its further improvement.

No offense.
I was making joke of myself :-)
I should have added a smiley.

> I tried to reproduce the problem with the attached sample code but
> without success.
> I tried using the special reader "\\?PnP?\Notification" and also using
> the current reader name but could not reproduce the problem. Yes, I
> first applied your patch and I get the extra sleep() in pcscd.
>
> You can change line 52 of my sample code to use the PnP reader or the
> normal one.

On my testing machine (Xubuntu 16.04 python-pyscard installed from
repos) I can reproduce the problem with the python Unit Tests and your
sample code, with any value in line 52. But on another installation of
Ubuntu 16.04 (self compiled pyscard) I can't reproduce it either.

My patch includes a change in the lipcsclite client library which is
very important to provoke the race condition. This change should produce
debug output ("Waiting 2s to provoke...") when executing the test
programs with env variable PCSCLITE_DEBUG=0.

Did you get this additional client side debug output?

I was not using the correct libpcsclite.
Now I can reproduce the problem using your C code.

The client will not be blocked during an INFINITE time but for 60 seconds
https://anonscm.debian.org/cgit/pcsclite/PCSC.git/tree/src/winscard_clnt.c#n1783
But that is still a bug.


My proposed patch is do modify removeReader() [readerfactory.c] to call EHDestroyEventHandler() _after_ RFUnInitializeReader() instead of before.

Something like:
--- /tmp/paqtOc_readerfactory.c 2017-05-09 15:15:23.885862634 +0200
+++ src/readerfactory.c 2017-05-09 15:15:21.461801253 +0200
@@ -615,6 +615,8 @@ LONG RFRemoveReader(const char *readerNa
 
 LONG removeReader(READER_CONTEXT * sContext)
 {
+   RFUnInitializeReader(sContext);
+
    /* Try to destroy the thread */
    if (sContext -> pthThread)
        (void)EHDestroyEventHandler(sContext);
@@ -626,8 +628,6 @@ LONG removeReader(READER_CONTEXT * sCont
        return SCARD_E_INVALID_VALUE;
    }
 
-   RFUnInitializeReader(sContext);
-
    *sContext->pMutex -= 1;


With this patch, the problem is fixed on my side.
Do you confirm it also fixes the problem for you?

Thanks

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Race condition with SCardGetStatusChange() when USB Reader is removed

Ludovic Rousseau
2017-05-09 15:16 GMT+02:00 Ludovic Rousseau <[hidden email]>:
My proposed patch is do modify removeReader() [readerfactory.c] to call EHDestroyEventHandler() _after_ RFUnInitializeReader() instead of before.

Ignore this patch. With it pcscd crashes on reader removal.
I will propose something better.

Bye

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Race condition with SCardGetStatusChange() when USB Reader is removed

Ludovic Rousseau


2017-05-10 13:44 GMT+02:00 Ludovic Rousseau <[hidden email]>:
2017-05-09 15:16 GMT+02:00 Ludovic Rousseau <[hidden email]>:
My proposed patch is do modify removeReader() [readerfactory.c] to call EHDestroyEventHandler() _after_ RFUnInitializeReader() instead of before.

Ignore this patch. With it pcscd crashes on reader removal.
I will propose something better.

But fixed in cbe677fc8fe94663fa70c5179385b17bdb7debec
https://github.com/LudovicRousseau/PCSC/commit/cbe677fc8fe94663fa70c5179385b17bdb7debec

Thanks

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Race condition with SCardGetStatusChange() when USB Reader is removed

Stein, Maximilian
In reply to this post by Ludovic Rousseau
> No offense.
> I was making joke of myself :-)
> I should have added a smiley.

None taken =) I'm glad to here we were only lost in transcription.


> I was not using the correct libpcsclite.
> Now I can reproduce the problem using your C code.

Can this become a general issue with pyscard? In the end I didn't quite
understand why the python unit tests used a 'different' libpcsclite when
there was only the patched version installed on the system...


Thanks for the fix. I was unsure if something could break when the
notification in the status handler thread was removed, though it didn't
seem so.

Maximilian

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Race condition with SCardGetStatusChange() when USB Reader is removed

Ludovic Rousseau
2017-05-11 17:17 GMT+02:00 Maximilian Stein <[hidden email]>:
> I was not using the correct libpcsclite.
> Now I can reproduce the problem using your C code.

Can this become a general issue with pyscard? In the end I didn't quite
understand why the python unit tests used a 'different' libpcsclite when
there was only the patched version installed on the system...

In my case I played with pcsc-spy [1].
install_spy.sh [2] confused the libraries.

I don't know what problem you had/have with PySCard.
Maybe you have multiple and different libpcsclite.so.1 available in your system.

Thanks for the fix. I was unsure if something could break when the
notification in the status handler thread was removed, though it didn't
seem so.

I reviewed the code and it should be OK.

Bye

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle