CCID should use CLOCK_MONOTONIC when possible

[hendrikdonner]

I'm hunting down an issue with spurious timeouts i see since CCID 1.5.0, one problem that was noticed is that timeouts happen when e.g. the system clock jumps into the future. Very rare on the target platform, but it does happen.

This is due to the use of CLOCK_REALTIME for pthread_cond_timedwait in ccid_usb.c.

I'm trying out the following diff to see if it fixes my timeout issues, rough idea so far:

--- a/src/ccid_usb.c
+++ b/src/ccid_usb.c
@@ -1019,7 +1019,7 @@ read_again:
                        time_t timeout_sec = usbDevice[reader_index].ccid.readTimeout  / 1000;
                        long timeout_msec = usbDevice[reader_index].ccid.readTimeout - timeout_sec * 1000;

-                       clock_gettime(CLOCK_REALTIME, &timeout);
+                       clock_gettime(CLOCK_MONOTONIC, &timeout);
                        timeout.tv_sec += timeout_sec;
                        timeout.tv_nsec += timeout_msec * 1000 * 1000;
                        if (timeout.tv_nsec > 1000 * 1000 * 1000)
@@ -1910,7 +1910,6 @@ static int Multi_InterruptRead(int reader_index, int timeout /* in ms */)
        struct usbDevice_MultiSlot_Extension *msExt;
        unsigned char buffer[CCID_INTERRUPT_SIZE];
        struct timespec cond_wait_until;
-       struct timeval local_time;
        int rv, status, interrupt_byte, interrupt_mask;

        msExt = usbDevice[reader_index].multislot_extension;
@@ -1927,10 +1926,7 @@ static int Multi_InterruptRead(int reader_index, int timeout /* in ms */)
        interrupt_mask = 0x02 << (2 * (usbDevice[reader_index].ccid.bCurrentSlotIndex % 4));

        /* Wait until the condition is signaled or a timeout occurs */
-       gettimeofday(&local_time, NULL);
-       cond_wait_until.tv_sec = local_time.tv_sec;
-       cond_wait_until.tv_nsec = local_time.tv_usec * 1000;
-
+       clock_gettime(CLOCK_MONOTONIC, &cond_wait_until);
        cond_wait_until.tv_sec += timeout / 1000;
        cond_wait_until.tv_nsec += 1000000 * (timeout % 1000);

@@ -2099,6 +2095,7 @@ static struct usbDevice_MultiSlot_Extension *Multi_CreateFirstSlot(int reader_in
 {
        struct usbDevice_MultiSlot_Extension *msExt;
        struct multiSlot_ConcurrentAccess *concurrent;
+       pthread_condattr_t condattr;

        /* Allocate a new extension buffer */
        msExt = malloc(sizeof(struct usbDevice_MultiSlot_Extension));
@@ -2116,7 +2113,10 @@ static struct usbDevice_MultiSlot_Extension *Multi_CreateFirstSlot(int reader_in

        /* Create mutex and condition object for the interrupt polling */
        pthread_mutex_init(&msExt->mutex, NULL);
-       pthread_cond_init(&msExt->condition, NULL);
+       pthread_condattr_init(&condattr);
+       pthread_condattr_setclock(&condattr, CLOCK_MONOTONIC);
+       pthread_cond_init(&msExt->condition, &condattr);
+       pthread_condattr_destroy(&condattr);

        /* concurrent USB read */
        concurrent = calloc(usbDevice[reader_index].ccid.bMaxSlotIndex +1,
@@ -2131,7 +2131,10 @@ static struct usbDevice_MultiSlot_Extension *Multi_CreateFirstSlot(int reader_in
        {
                /* Create mutex and condition object for the concurrent read */
                pthread_mutex_init(&concurrent[slot].mutex, NULL);
-               pthread_cond_init(&concurrent[slot].condition, NULL);
+               pthread_condattr_init(&condattr);
+               pthread_condattr_setclock(&condattr, CLOCK_MONOTONIC);
+               pthread_cond_init(&concurrent[slot].condition, &condattr);
+               pthread_condattr_destroy(&condattr);
        }
        msExt->concurrent = concurrent;

Notice that libusb itself has CLOCK_MONOTONIC support, see e.g. libusb/os/threads_posix.c for a similar approach to the pthread_cond_timedwait API. A cleaner implementation should probably check if the OS supports the relevant pthread API calls just like libusb does.

[hendrikdonner]

This was meant for the CCID repo, hope it's not too confusing :)

[LudovicRousseau]

Maybe you want to create a PR for the CCID project?

[hendrikdonner]

Maybe you want to create a PR for the CCID project?

Let me see. I think you might be able to transfer this issue over to the CCID project btw.

[LudovicRousseau]

Yes, I can transfer the patch. But it is also nice to have your name in the git history.

[hendrikdonner]

Yes, I can transfer the patch. But it is also nice to have your name in the git history.

I was talking about moving this issue. Anyways, PR is open on the correct repo.

[LudovicRousseau]

PR is at https://github.com/LudovicRousseau/CCID/pull/138