darwin: add missing locking around cached device list cleanup

The cleanup function darwin_cleanup_devices was intented to be called only once
at program exit. When the initialization/finalization code was changed to
destroy the cached device list on last exit this function should have been
modified to require a lock on darwin_cached_devices_lock. This commit updates
cleanup to protect the cached device list with the cached devices mutex and
updates darwin_init to print out an error and return if a reference leak is
detected.

Also using this opportunity to correct the naming of the mutex. Changed
darwin_cached_devices_lock to darwin_cached_devices_mutex. Also cleaning the
initialization code up a bit. Removing the context pointer from the hotplug
thread as it is not used for anything but logging.

Fixes #1124

Signed-off-by: Nathan Hjelm <hjelmn@google.com>

2 files changed, 77 insertions, 64 deletions

diff --git a/libusb/os/darwin_usb.c b/libusb/os/darwin_usb.c
index 388dbca..7730d71 100644
--- a/libusb/os/darwin_usb.c
+++ b/libusb/os/darwin_usb.c
@@ -2,7 +2,7 @@
 /*
  * darwin backend for libusb 1.0
  * Copyright © 2008-2021 Nathan Hjelm <hjelmn@cs.unm.edu>
- * Copyright © 2019-2021 Google LLC. All rights reserved.
+ * Copyright © 2019-2022 Google LLC. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -58,6 +58,8 @@ static int init_count = 0;
 static const mach_port_t darwin_default_master_port = 0;
 
 /* async event thread */
+/* if both this mutex and darwin_cached_devices_mutex are to be acquired then
+   darwin_cached_devices_mutex must be acquired first. */
 static pthread_mutex_t libusb_darwin_at_mutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t  libusb_darwin_at_cond = PTHREAD_COND_INITIALIZER;
 
@@ -71,7 +73,7 @@ static clock_serv_t clock_monotonic;
 static CFRunLoopRef libusb_darwin_acfl = NULL; /* event cf loop */
 static CFRunLoopSourceRef libusb_darwin_acfls = NULL; /* shutdown signal for event cf loop */
 
-static usbi_mutex_t darwin_cached_devices_lock = PTHREAD_MUTEX_INITIALIZER;
+static usbi_mutex_t darwin_cached_devices_mutex = PTHREAD_MUTEX_INITIALIZER;
 static struct list_head darwin_cached_devices;
 static const char *darwin_device_class = "IOUSBDevice";
 
@@ -80,6 +82,7 @@ static const char *darwin_device_class = "IOUSBDevice";
 /* async event thread */
 static pthread_t libusb_darwin_at;
 
+static void darwin_exit(struct libusb_context *ctx);
 static int darwin_get_config_descriptor(struct libusb_device *dev, uint8_t config_index, void *buffer, size_t len);
 static int darwin_claim_interface(struct libusb_device_handle *dev_handle, uint8_t iface);
 static int darwin_release_interface(struct libusb_device_handle *dev_handle, uint8_t iface);
@@ -172,7 +175,7 @@ static enum libusb_error darwin_to_libusb (IOReturn result) {
   }
 }
 
-/* this function must be called with the darwin_cached_devices_lock held */
+/* this function must be called with the darwin_cached_devices_mutex held */
 static void darwin_deref_cached_device(struct darwin_cached_device *cached_dev) {
   cached_dev->refcount--;
   /* free the device and remove it from the cache */
@@ -394,7 +397,7 @@ static void darwin_devices_detached (void *ptr, io_iterator_t rem_devices) {
 
     /* we need to match darwin_ref_cached_device call made in darwin_get_cached_device function
        otherwise no cached device will ever get freed */
-    usbi_mutex_lock(&darwin_cached_devices_lock);
+    usbi_mutex_lock(&darwin_cached_devices_mutex);
     list_for_each_entry(old_device, &darwin_cached_devices, list, struct darwin_cached_device) {
       if (old_device->session == session) {
         if (old_device->in_reenumerate) {
@@ -418,7 +421,7 @@ static void darwin_devices_detached (void *ptr, io_iterator_t rem_devices) {
       }
     }
 
-    usbi_mutex_unlock(&darwin_cached_devices_lock);
+    usbi_mutex_unlock(&darwin_cached_devices_mutex);
     if (is_reenumerating) {
       continue;
     }
@@ -466,8 +469,8 @@ static void darwin_fail_startup(void) {
 }
 
 static void *darwin_event_thread_main (void *arg0) {
+  UNUSED(arg0);
   IOReturn kresult;
-  struct libusb_context *ctx = (struct libusb_context *)arg0;
   CFRunLoopRef runloop;
   CFRunLoopSourceRef libusb_shutdown_cfsource;
   CFRunLoopSourceContext libusb_shutdown_cfsourcectx;
@@ -495,7 +498,7 @@ static void *darwin_event_thread_main (void *arg0) {
   io_iterator_t          libusb_add_device_iterator;
 
   /* ctx must only be used for logging during thread startup */
-  usbi_dbg (ctx, "creating hotplug event source");
+  usbi_dbg (NULL, "creating hotplug event source");
 
   runloop = CFRunLoopGetCurrent ();
   CFRetain (runloop);
@@ -519,7 +522,7 @@ static void *darwin_event_thread_main (void *arg0) {
                                               NULL, &libusb_rem_device_iterator);
 
   if (kresult != kIOReturnSuccess) {
-    usbi_err (ctx, "could not add hotplug event source: %s", darwin_error_str (kresult));
+    usbi_err (NULL, "could not add hotplug event source: %s", darwin_error_str (kresult));
     CFRelease (libusb_shutdown_cfsource);
     CFRelease (runloop);
     darwin_fail_startup ();
@@ -532,7 +535,7 @@ static void *darwin_event_thread_main (void *arg0) {
                                               NULL, &libusb_add_device_iterator);
 
   if (kresult != kIOReturnSuccess) {
-    usbi_err (ctx, "could not add hotplug event source: %s", darwin_error_str (kresult));
+    usbi_err (NULL, "could not add hotplug event source: %s", darwin_error_str (kresult));
     CFRelease (libusb_shutdown_cfsource);
     CFRelease (runloop);
     darwin_fail_startup ();
@@ -542,7 +545,7 @@ static void *darwin_event_thread_main (void *arg0) {
   darwin_clear_iterator (libusb_rem_device_iterator);
   darwin_clear_iterator (libusb_add_device_iterator);
 
-  usbi_dbg (ctx, "darwin event thread ready to receive events");
+  usbi_dbg (NULL, "darwin event thread ready to receive events");
 
   /* signal the main thread that the hotplug runloop has been created. */
   pthread_mutex_lock (&libusb_darwin_at_mutex);
@@ -582,73 +585,81 @@ static void *darwin_event_thread_main (void *arg0) {
   pthread_exit (NULL);
 }
 
-/* cleanup function to destroy cached devices */
+/* cleanup function to destroy cached devices. must be called with a lock on darwin_cached_devices_mutex */
 static void darwin_cleanup_devices(void) {
   struct darwin_cached_device *dev, *next;
 
   list_for_each_entry_safe(dev, next, &darwin_cached_devices, list, struct darwin_cached_device) {
+    if (dev->refcount > 1) {
+      usbi_err(NULL, "device still referenced at libusb_exit");
+    }
     darwin_deref_cached_device(dev);
   }
 }
 
-static int darwin_init(struct libusb_context *ctx) {
-  bool first_init;
-  int rc;
+/* must be called with a lock on darwin_cached_devices_mutex */
+static int darwin_first_time_init(void) {
+  if (NULL == darwin_cached_devices.next) {
+    list_init (&darwin_cached_devices);
+  }
 
-  first_init = (1 == ++init_count);
+  if (!list_empty(&darwin_cached_devices)) {
+    usbi_err(NULL, "libusb_device reference not released on last exit. will not continue");
+    return LIBUSB_ERROR_OTHER;
+  }
 
-  do {
-    if (first_init) {
-      if (NULL == darwin_cached_devices.next) {
-        list_init (&darwin_cached_devices);
-      }
-      assert(list_empty(&darwin_cached_devices));
 #if !defined(HAVE_CLOCK_GETTIME)
-      /* create the clocks that will be used if clock_gettime() is not available */
-      host_name_port_t host_self;
+  /* create the clocks that will be used if clock_gettime() is not available */
+  host_name_port_t host_self;
 
-      host_self = mach_host_self();
-      host_get_clock_service(host_self, CALENDAR_CLOCK, &clock_realtime);
-      host_get_clock_service(host_self, SYSTEM_CLOCK, &clock_monotonic);
-      mach_port_deallocate(mach_task_self(), host_self);
+  host_self = mach_host_self();
+  host_get_clock_service(host_self, CALENDAR_CLOCK, &clock_realtime);
+  host_get_clock_service(host_self, SYSTEM_CLOCK, &clock_monotonic);
+  mach_port_deallocate(mach_task_self(), host_self);
 #endif
-    }
 
-    rc = darwin_scan_devices (ctx);
-    if (LIBUSB_SUCCESS != rc)
-      break;
+  int rc = pthread_create (&libusb_darwin_at, NULL, darwin_event_thread_main, NULL);
+  if (0 != rc) {
+    usbi_err (NULL, "could not create event thread, error %d", rc);
+    return LIBUSB_ERROR_OTHER;
+  }
 
-    if (first_init) {
-      rc = pthread_create (&libusb_darwin_at, NULL, darwin_event_thread_main, ctx);
-      if (0 != rc) {
-        usbi_err (ctx, "could not create event thread, error %d", rc);
-        rc = LIBUSB_ERROR_OTHER;
-        break;
-      }
+  pthread_mutex_lock (&libusb_darwin_at_mutex);
+  while (NULL == libusb_darwin_acfl) {
+    pthread_cond_wait (&libusb_darwin_at_cond, &libusb_darwin_at_mutex);
+  }
 
-      pthread_mutex_lock (&libusb_darwin_at_mutex);
-      while (!libusb_darwin_acfl)
-        pthread_cond_wait (&libusb_darwin_at_cond, &libusb_darwin_at_mutex);
-      if (libusb_darwin_acfl == LIBUSB_DARWIN_STARTUP_FAILURE) {
-        libusb_darwin_acfl = NULL;
-        rc = LIBUSB_ERROR_OTHER;
-      }
-      pthread_mutex_unlock (&libusb_darwin_at_mutex);
+  if (libusb_darwin_acfl == LIBUSB_DARWIN_STARTUP_FAILURE) {
+    libusb_darwin_acfl = NULL;
+    rc = LIBUSB_ERROR_OTHER;
+  }
+  pthread_mutex_unlock (&libusb_darwin_at_mutex);
+
+  return rc;
+}
+
+static int darwin_init_context(struct libusb_context *ctx) {
+  usbi_mutex_lock(&darwin_cached_devices_mutex);
 
-      if (0 != rc)
-        pthread_join (libusb_darwin_at, NULL);
+  bool first_init = (1 == ++init_count);
+
+  if (first_init) {
+    int rc = darwin_first_time_init();
+    if (LIBUSB_SUCCESS != rc) {
+      usbi_mutex_unlock(&darwin_cached_devices_mutex);
+      return rc;
     }
-  } while (0);
+  }
+  usbi_mutex_unlock(&darwin_cached_devices_mutex);
+
+  return darwin_scan_devices (ctx);
+}
 
+static int darwin_init(struct libusb_context *ctx) {
+  int rc = darwin_init_context(ctx);
   if (LIBUSB_SUCCESS != rc) {
-    if (first_init) {
-      darwin_cleanup_devices ();
-#if !defined(HAVE_CLOCK_GETTIME)
-      mach_port_deallocate(mach_task_self(), clock_realtime);
-      mach_port_deallocate(mach_task_self(), clock_monotonic);
-#endif
-    }
-    --init_count;
+    /* clean up any allocated resources */
+    darwin_exit(ctx);
   }
 
   return rc;
@@ -657,6 +668,7 @@ static int darwin_init(struct libusb_context *ctx) {
 static void darwin_exit (struct libusb_context *ctx) {
   UNUSED(ctx);
 
+  usbi_mutex_lock(&darwin_cached_devices_mutex);
   if (0 == --init_count) {
     /* stop the event runloop and wait for the thread to terminate. */
     pthread_mutex_lock (&libusb_darwin_at_mutex);
@@ -674,6 +686,7 @@ static void darwin_exit (struct libusb_context *ctx) {
     mach_port_deallocate(mach_task_self(), clock_monotonic);
 #endif
   }
+  usbi_mutex_unlock(&darwin_cached_devices_mutex);
 }
 
 static int get_configuration_index (struct libusb_device *dev, UInt8 config_value) {
@@ -1018,7 +1031,7 @@ static enum libusb_error darwin_get_cached_device(struct libusb_context *ctx, io
     usbi_dbg(ctx, "parent sessionID: 0x%" PRIx64, parent_sessionID);
   }
 
-  usbi_mutex_lock(&darwin_cached_devices_lock);
+  usbi_mutex_lock(&darwin_cached_devices_mutex);
   do {
     list_for_each_entry(new_device, &darwin_cached_devices, list, struct darwin_cached_device) {
       usbi_dbg(ctx, "matching sessionID/locationID 0x%" PRIx64 "/0x%x against cached device with sessionID/locationID 0x%" PRIx64 "/0x%x",
@@ -1094,7 +1107,7 @@ static enum libusb_error darwin_get_cached_device(struct libusb_context *ctx, io
     }
   } while (0);
 
-  usbi_mutex_unlock(&darwin_cached_devices_lock);
+  usbi_mutex_unlock(&darwin_cached_devices_mutex);
 
   return ret;
 }
@@ -1945,10 +1958,10 @@ static void darwin_destroy_device(struct libusb_device *dev) {
 
   if (dpriv->dev) {
     /* need to hold the lock in case this is the last reference to the device */
-    usbi_mutex_lock(&darwin_cached_devices_lock);
+    usbi_mutex_lock(&darwin_cached_devices_mutex);
     darwin_deref_cached_device (dpriv->dev);
     dpriv->dev = NULL;
-    usbi_mutex_unlock(&darwin_cached_devices_lock);
+    usbi_mutex_unlock(&darwin_cached_devices_mutex);
   }
 }
 
@@ -2504,7 +2517,7 @@ static int darwin_reload_device (struct libusb_device_handle *dev_handle) {
   struct darwin_cached_device *dpriv = DARWIN_CACHED_DEVICE(dev_handle->dev);
   enum libusb_error err;
 
-  usbi_mutex_lock(&darwin_cached_devices_lock);
+  usbi_mutex_lock(&darwin_cached_devices_mutex);
   (*(dpriv->device))->Release(dpriv->device);
   dpriv->device = darwin_device_from_service (HANDLE_CTX (dev_handle), dpriv->service);
   if (!dpriv->device) {
@@ -2512,7 +2525,7 @@ static int darwin_reload_device (struct libusb_device_handle *dev_handle) {
   } else {
     err = LIBUSB_SUCCESS;
   }
-  usbi_mutex_unlock(&darwin_cached_devices_lock);
+  usbi_mutex_unlock(&darwin_cached_devices_mutex);
 
   return err;
 }
diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index b7c17a1..e1d64a3 100644
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 11725
+#define LIBUSB_NANO 11726