Memory-Leak in pcscd

[Max-Mustermann33]

Describe the bug
After updating to current versions of pcscd, ccid and libusb I encountered a steady memory leak. In my case the leak was in the range of 10-40 MB each day.

The actual problem stems from the fact that pcscd is calling libusb_init() with the default libusb-context, while ccid is calling libusb_init() with a special context. While this is not illegal to do (afaik) with newer libusb-versions it causes that memory allocated in libusb-function usbi_add_event_source() will not be freed for the libusb_context that was created first. At some point libusb was changed that the memory allocated there will not be freed in usbi_remove_event_source(), but later on when either the libusb_context is destroyed (which never happens in my case) or when an event occurs. But events only occur on the second libusb_context and never on the first one.

I applied this patch to ccid and the memory leak disappeared. Though I guess this would cause trouble when libusb <= 1.0.8 would be used.

The auto-formating here seems to clash with +/- from diff so I had to add some "escape slashes".

diff --git a/src/ccid_usb.c b/src/ccid_usb.c
index 17f1296..573019d 100644
--- a/src/ccid_usb.c
+++ b/src/ccid_usb.c
@@ -59,8 +59,9 @@
 
 /* Using the default libusb context */
 /* does not work for libusb <= 1.0.8 */
-/* #define ctx NULL */
-static libusb_context *ctx = NULL;
+#define ctx NULL
+/*static libusb_context *ctx = NULL;*/
 
 #define CCID_INTERRUPT_SIZE 8
 
@@ -193,9 +194,6 @@ static void close_libusb_if_needed(void)
 {
        bool to_exit = true;
 
-      if (NULL == ctx)
-      return;
-
        /* if at least 1 reader is still in use we do not exit libusb */
        for (int i=0; i<CCID_DRIVER_MAX_READERS; i++)
        {
@@ -207,7 +205,6 @@ static void close_libusb_if_needed(void)
        {
                DEBUG_INFO1("libusb_exit");
                libusb_exit(ctx);
-              ctx = NULL;
        }
 } /* close_libusb_if_needed */
 
@@ -350,15 +347,12 @@ status_t OpenUSBByName(unsigned int reader_index, /*@null@*/ char *device)
        GET_KEY("ifdProductString", values)
        GET_KEY("Copyright", values)
 
-       if (NULL == ctx)
+       rv = libusb_init(ctx);
+       if (rv != 0)
        {
-               rv = libusb_init(&ctx);
-               if (rv != 0)
-               {
-                       DEBUG_CRITICAL2("libusb_init failed: %s", libusb_error_name(rv));
-                       return_value = STATUS_UNSUCCESSFUL;
-                       goto end1;
-               }
+               DEBUG_CRITICAL2("libusb_init failed: %s", libusb_error_name(rv));
+               return_value = STATUS_UNSUCCESSFUL;
+               goto end1;
        }
 
#define GET_KEYS(key, values) \

To Reproduce
pcscd is started with the following arguments:
-S --error --foreground -c

Versions:

• ccid version 1.6.2 (but should be the same with current master)
• pcsc-lite version 2.3.3
• libusb version 1.0.29
• pcscd -v
  pcsc-lite version 2.2.0
  Copyright (C) 1999-2002 by David Corcoran corcoran@musclecard.com.
  Copyright (C) 2001-2024 by Ludovic Rousseau ludovic.rousseau@free.fr.
  Copyright (C) 2003-2004 by Damien Sauveron sauveron@labri.fr.
  Report bugs to pcsclite-muscle@lists.infradead.org.
  Enabled features: Linux x86_64-pc-linux-gnu serial usb libusb usbdropdir=/usr/lib/pcsc/drivers ipcdir=/var/run filter configdir=//etc/reader.conf.d
  MAX_READERNAME: 128, PCSCLITE_MAX_READERS_CONTEXTS: 16

Platform:

• Custom OS
• Does not actually require a smartcard (The problem also exists with softtokens being used)

Log:
The only syslog-message produces by pcscd is the following, but I guess this might just be because it was too early after boot to try and access USB. The daemon still works after that.

pcscd: hotplug_libusb.c:470:HPEstablishUSBNotifications() libusb_init failed: LIBUSB_ERROR_OTHER

[LudovicRousseau]

Why don't you use libudev on Linux?

What is your use case exactly? You have a lot of USB hotplug events?

[Max-Mustermann33]

I actually don't know what the pros/cons of using libudev vs libusb are for pcscd.

The system uses a smartcard to authenticate itself in a PKI and also to generate random-numbers. So there is regular access to the smartcard needed. USB hotplug in general is not uncommon (though I would not call it "a lot"). USB hotplug for smartcard-readers is possible, but should be quite rare.

[LudovicRousseau]

I asked about USB hotplug because you changed code in the driver that is used only when a reader is connected/disconnected.
If the USB device is always connected (which is what I understood of your use case) then your changes should not have a big impact.

Can you provide a pcscd log as documented in https://ccid.apdu.fr/#support ?

[Max-Mustermann33]

As far as I understood the problem, it is not directly connected to USB hotplug. But the fact that there is a second libusb-context opened causes the problem. For me it looked like (in gdb) that the process is scanning for available USB-devices on a regular basis - adding and removing the USB-event sources again and again. But only due to 2 libusb-contexts being active at the same time, for one of the contexts the memory for those USB-event sources could not be freed anymore.

I removed APDU-logs for now.

log3.txt

[LudovicRousseau]

The benefit of libudev over libusb for pcsc-lite is that there is no active polling every 1 second, so no memory leak from libusb.

[...]
00111112 [121207476025024] readerfactory.c:391:RFAddReader() Using the reader polling thread
00001083 [121207476025024] hotplug_libusb.c:492:HPEstablishUSBNotifications() Driver ifd-ccid.bundle does not support IFD_GENERATE_HOTPLUG. Using active polling instead.
00000012 [121207476025024] hotplug_libusb.c:502:HPEstablishUSBNotifications() Polling forced every 1 second(s)
[...]

The problem comes from src/hotplug_libusb.c. It should be fixed in pcsc-lite, not the CCID driver.

[Max-Mustermann33]

Okay that sounds like a reasonable advantage for libudev.

But if one were to use libusb - what do you mean when you say the problem comes from hotplug_libusb.c?
I guess the polling itself is not a problem per se. Or do you mean that the pcscd should not use the default-context for libusb?

[LudovicRousseau]

The memory leak comes (I am pretty sure) from pcsc-lite scanning the USB bus every 1 seconds.
Maybe using a non-default libusb context in hotplug_libusb.c could solve the problem.
If you can try such a change that would help me.

[Max-Mustermann33]

As far as I understand the issue, the core problem is that more than one libusb context exists at the same time. The scanning of the USB-bus for USB-hotplug can't be the only cause for creating and removing USB-event-sources.

I tried patching hotplug_libusb.c to use a non-default libusb-context and reverting my changes to ccid. The result was that pcscd was leaking memory. Also I had a problem because one of the two connected USB-smartcard-readers was not detected anymore.

My interpretation of this is that in this setup both pcsc and ccid tried to use non-default libusb contexts, but they still used different contexts. So the initial problem still persists.
Still there are obious differences to the original version (default context in pcscd and non-default in ccid) that I cannot explain. Like the fact that one SC-reader did not work anymore and the fact that the initial amount of used memory dropped quite a lot. (11MB --> 2MB) But the latter was also the case, when I tested the system with only the ccid-changes.

[LudovicRousseau]

Do you know in which git commit the change in libusb was done?
Thanks

[LudovicRousseau]

I had a look at the code and the message "libusb_init failed: xxx" is displayed just before pcscd is terminated.
https://github.com/LudovicRousseau/PCSC/blob/master/src/hotplug_libusb.c#L461

I guess a new pcscd is started (without this error) later in your case.

I modified pcscd to rescan the USB bus every 100 µs instead of every second (so 10 000 more often) and was not able to reproduce a memory leak.
I am using Debian testing with libusb 1.0.29-2, pcsc-lite 2.4.0 and libccid 1.7.0-1

To do the same on your side you can use this patch:

diff --git a/src/hotplug_libusb.c b/src/hotplug_libusb.c
index dd46bf76..73fd0f14 100644
--- a/src/hotplug_libusb.c
+++ b/src/hotplug_libusb.c
@@ -499,7 +499,7 @@ static void * HPEstablishUSBNotifications(int pipefd[2])
 	{
 		while (!AraKiriHotPlug)
 		{
-			SYS_Sleep(HPForceReaderPolling);
+			SYS_USleep(100);
 			HPRescanUsbBus();
 		}
 	}

With the patch applied, is the memory leak more visible/obvious?

[Max-Mustermann33]

Do you know in which git commit the change in libusb was done? Thanks

It was this commit:
dd7df2b7 core: protect against changes to the pollfd list during handle_events
I know this is not exactly very recent, but the libusb-version I used before was really old.

[Max-Mustermann33]

There is no pcscd-restart in my case, but there is a patch that I use, which prevents exactly this.

diff --git a/src/hotplug_libusb.c b/src/hotplug_libusb.c
index d7d0dd8b..967bce11 100644
--- a/src/hotplug_libusb.c
+++ b/src/hotplug_libusb.c
@@ -468,9 +468,9 @@ static void * HPEstablishUSBNotifications(int pipefd[2])
        if (r < 0)
        {
                Log2(PCSC_LOG_CRITICAL, "libusb_init failed: %s", libusb_error_name(r));
-               /* emergency exit */
-               kill(getpid(), SIGTERM);
-               return NULL;
+               /* signal that the main thread can continue, no USB readers will be available */
+               write(pipefd[1], &c, 1);
+               return;
        }
 
        /* scan the USB bus for devices at startup */

When I use your patch to increase the amount of USB-scanning, then the leak becomes way more obvious. I can see an increase in memory usage of almost 1 MB every second.
When you can't reproduce the leak like that, then it looks like there is some other condition necessary to trigger this memory-leak.

[LudovicRousseau]

I don't think this patch will compile.
HPEstablishUSBNotifications() is not a void function so you have to return a value.

[Max-Mustermann33]

I don't think this patch will compile. HPEstablishUSBNotifications() is not a void function so you have to return a value.

The gcc from debian bookworm does compile this code - with a warning of course. I guess the compiler would choose something to return here. But anyway this code was in fact changed to "return NULL" in the meantime already.