Skip to content

Commit

Permalink
Ensure cleanup on libusb hotplug shutdown
Browse files Browse the repository at this point in the history
Fix the race condition that can result in the hotplug_libusb's rescan
thread triggering use-after-free during the daemon shutdown. The commit
makes the main thread wait (using a POSIX pipe) for the rescan thread's
cleanup instead of relying on a non-deterministic "sleep 1 second"
workaround.

Also fix memory leaks that could happen during the rescan thread
shutdown. This could happen if the AraKiriHotPlug was false when it was
checked by the HPRescanUsbBus() function, but changed to true by the
time the next iteration of the "while" loop in
HPEstablishUSBNotifications() checked it. The commit moves the cleanup
to the latter function, so that the cleanup always takes place.
  • Loading branch information
emaxx-google committed Apr 12, 2024
1 parent e50c01e commit 07749bc
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 34 deletions.
74 changes: 41 additions & 33 deletions src/hotplug_libusb.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ pthread_mutex_t usbNotifierMutex;
static pthread_t usbNotifyThread;
static int driverSize = -1;
static bool AraKiriHotPlug = false;
/**
* the first end is written to when the USB rescan is requested; the second end
* is written to when the rescan thread is about to shut down
*/
static int rescan_pipe[] = { -1, -1 };
extern int HPForceReaderPolling;

Expand Down Expand Up @@ -447,30 +451,6 @@ static void HPRescanUsbBus(void)

/* free the libusb allocated list & devices */
libusb_free_device_list(devs, 1);

if (AraKiriHotPlug)
{
int retval;

for (i=0; i<PCSCLITE_MAX_READERS_CONTEXTS; i++)
{
if (readerTracker[i].fullName != NULL)
HPCleanupHotPluggable(i);
}

for (i=0; i<driverSize; i++)
{
/* free strings allocated by strdup() */
free(driverTracker[i].bundleName);
free(driverTracker[i].libraryPath);
free(driverTracker[i].readerName);
free(driverTracker[i].CFBundleName);
}
free(driverTracker);

Log1(PCSC_LOG_INFO, "Hotplug stopped");
pthread_exit(&retval);
}
}

static void * HPEstablishUSBNotifications(int pipefd[2])
Expand Down Expand Up @@ -526,17 +506,10 @@ static void * HPEstablishUSBNotifications(int pipefd[2])
SYS_Sleep(HPForceReaderPolling);
HPRescanUsbBus();
}
libusb_exit(ctx);
}
else
{
char dummy;

if (pipe(rescan_pipe) == -1)
{
Log2(PCSC_LOG_ERROR, "pipe: %s", strerror(errno));
return NULL;
}
while (read(rescan_pipe[0], &dummy, sizeof(dummy)) > 0)
{
Log1(PCSC_LOG_INFO, "Reload serial configuration");
Expand All @@ -546,10 +519,32 @@ static void * HPEstablishUSBNotifications(int pipefd[2])
#endif
Log1(PCSC_LOG_INFO, "End reload serial configuration");
}
close(rescan_pipe[0]);
rescan_pipe[0] = -1;
}

/* Clean up resources before exiting the thread. */
for (int i=0; i<PCSCLITE_MAX_READERS_CONTEXTS; i++)
{
if (readerTracker[i].fullName != NULL)
HPCleanupHotPluggable(i);
}
libusb_exit(ctx);
for (int i=0; i<driverSize; i++)
{
/* free strings allocated by strdup() */
free(driverTracker[i].bundleName);
free(driverTracker[i].libraryPath);
free(driverTracker[i].readerName);
free(driverTracker[i].CFBundleName);
}
free(driverTracker);

Log1(PCSC_LOG_INFO, "Hotplug stopped");

/* notify the main thread that the rescan shutdown completed */
char dummy;
if (write(rescan_pipe[0], &dummy, sizeof(dummy)) == -1)
Log2(PCSC_LOG_ERROR, "write: %s", strerror(errno));

return NULL;
}

Expand All @@ -567,6 +562,7 @@ LONG HPSearchHotPluggables(const char * hpDirPath)

if (HPReadBundleValues(hpDirPath) > 0)
{
/* used for waiting for the initialization completion */
int pipefd[2];
char c;

Expand All @@ -576,6 +572,13 @@ LONG HPSearchHotPluggables(const char * hpDirPath)
return -1;
}

/* used for rescan and shutdown events */
if (pipe(rescan_pipe) == -1)
{
Log2(PCSC_LOG_ERROR, "pipe: %s", strerror(errno));
return -1;
}

ThreadCreate(&usbNotifyThread, THREAD_ATTR_DETACHED,
(PCSCLITE_THREAD_FUNCTION( )) HPEstablishUSBNotifications, pipefd);

Expand All @@ -599,6 +602,11 @@ LONG HPStopHotPluggables(void)
AraKiriHotPlug = true;
if (rescan_pipe[1] >= 0)
{
char dummy;
/* wait until the rescan thread completes the cleanup */
read(rescan_pipe[1], &dummy, sizeof(dummy));
close(rescan_pipe[0]);
rescan_pipe[0] = -1;
close(rescan_pipe[1]);
rescan_pipe[1] = -1;
}
Expand Down
1 change: 0 additions & 1 deletion src/pcscdaemon.c
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ static void SVCServiceRunLoop(void)
#ifdef USE_USB
(void)HPStopHotPluggables();
#endif
(void)SYS_Sleep(1);

/* stop all the clients */
ContextsDeinitialize();
Expand Down

0 comments on commit 07749bc

Please sign in to comment.