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

2017-05-03 14:42 GMT+02:00 Maximilian Stein <[email protected]>:

> 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?
>

Correct.
Fixed in
https://github.com/LudovicRousseau/PCSC/commit/789dae8a8acf8d1693597159d7d590a96bdd6424


>
> 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,


[1] https://www.gnu.org/software/libc/manual/html_node/Atomic-Types.html
[2]
https://alioth.debian.org/tracker/index.php?func=detail&aid=315725&group_id=30105&atid=410085

-- 
 Dr. Ludovic Rousseau

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

#! /usr/bin/env python

#   SCardGetStatusChange.py : Unitary test for SCardGetStatusChange()
#   Copyright (C) 2011  Ludovic Rousseau
#
#   This program is free software; you can redistribute it and/or modify
#   it under the terms of the GNU General Public License as published by
#   the Free Software Foundation; either version 3 of the License, or
#   (at your option) any later version.
#
#   This program is distributed in the hope that it will be useful,
#   but WITHOUT ANY WARRANTY; without even the implied warranty of
#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
#   GNU General Public License for more details.
#
#   You should have received a copy of the GNU General Public License along
#   with this program; if not, see <http://www.gnu.org/licenses/>.


from __future__ import print_function
from smartcard.scard import *
from smartcard.pcsc.PCSCExceptions import *


def parseEventState(eventstate):
    stateList = list()
    states = {SCARD_STATE_IGNORE: "Ignore",
              SCARD_STATE_CHANGED: "Changed",
              SCARD_STATE_UNKNOWN: "Unknown",
              SCARD_STATE_UNAVAILABLE: "Unavailable",
              SCARD_STATE_EMPTY: "Empty",
              SCARD_STATE_PRESENT: "Present",
              SCARD_STATE_ATRMATCH: "ATR match",
              SCARD_STATE_EXCLUSIVE: "Exclusive",
              SCARD_STATE_INUSE: "In use",
              SCARD_STATE_MUTE: "Mute",
              SCARD_STATE_UNPOWERED: "Unpowered"}
    for state in states:
        if eventstate & state:
            stateList.append(states[state])
    return stateList

hresult, hcontext = SCardEstablishContext(SCARD_SCOPE_USER)
if hresult != SCARD_S_SUCCESS:
    raise EstablishContextException(hresult)

hresult, readers = SCardListReaders(hcontext, [])
if hresult != SCARD_S_SUCCESS:
    raise ListReadersException(hresult)
print('PC/SC Readers:', readers)

pnp = False

readerstates = {}
if pnp:
    readers = ("\\\\?PnP?\\Notification",)
for reader in (readers):
    print(reader)
    readerstates[reader] = (reader, SCARD_STATE_UNAWARE)

if not pnp:
    hresult, newstates = SCardGetStatusChange(hcontext, 0,
            list(readerstates.values()))
    if hresult != SCARD_S_SUCCESS:
        raise BaseSCardException(hresult)
    print(newstates)
    for readerState in newstates:
        readername, eventstate, atr = readerState
        readerstates[readername] = (readername, eventstate)

print("Remove the reader")

hresult, newstates = SCardGetStatusChange(hcontext, 1000000,
                                          list(readerstates.values()))
print("SCardGetStatusChange()", SCardGetErrorMessage(hresult))
if hresult != SCARD_S_SUCCESS and hresult != SCARD_E_TIMEOUT:
    raise BaseSCardException(hresult)
for readerState in newstates:
    readername, eventstate, atr = readerState
    print(readername, hex(eventstate))
    print(parseEventState(eventstate))

hresult = SCardReleaseContext(hcontext)
print("SCardReleaseContext()", SCardGetErrorMessage(hresult))
if hresult != SCARD_S_SUCCESS:
    raise ReleaseContextException(hresult)