Bluetooth: a scan holds the driver, so quitting during one cannot crash

The scan task calls the driver from the task thread while the frontend
can stop the driver from its own, so quitting during a scan freed the
driver under the task, which then ended the scan on freed memory and
corrupted the heap.

The driver started is now held by a count: the frontend's hold, and one
taken for each scan, on the frontend's thread, when the scan task is
pushed. Stopping the driver lets go of the frontend's; whichever lets go
last frees it, so a scan running finishes on a live driver. No lock and
nothing waited for.

bluetooth/bluetooth_driver.c

@@ -15,6 +15,9 @@
  */
 
 #include <stdint.h>
+#include <stdlib.h>
+
+#include <retro_atomic.h>
 
 #ifdef HAVE_CONFIG_H
 #include "../config.h"
@@ -58,6 +61,15 @@ const bluetooth_driver_t *bluetooth_drivers[] = {
 
 static bluetooth_driver_state_t bluetooth_driver_st = {0};
 
+/* The frontend's hold, and one for each scan running: whichever lets
+ * go last frees the driver. */
+struct bluetooth_hold
+{
+   const bluetooth_driver_t *drv;
+   void *data;
+   retro_atomic_int_t refs;
+};
+
 bluetooth_driver_state_t *bluetooth_state_get_ptr(void)
 {
    return &bluetooth_driver_st;
@@ -77,30 +89,39 @@ const char* config_get_bluetooth_driver_options(void)
    return char_list_new_special(STRING_LIST_BLUETOOTH_DRIVERS, NULL);
 }
 
-void driver_bluetooth_scan_begin(void)
+struct bluetooth_hold *driver_bluetooth_scan_hold(void)
 {
    bluetooth_driver_state_t *bt_st = &bluetooth_driver_st;
-   if (     bt_st
-        &&  bt_st->active
-        &&  bt_st->drv->scan_begin )
-      bt_st->drv->scan_begin(bt_st->data);
+   if (!bt_st->active || !bt_st->hold)
+      return NULL;
+   retro_atomic_inc_int(&bt_st->hold->refs);
+   return bt_st->hold;
 }
 
-void driver_bluetooth_scan_end(void)
+void driver_bluetooth_scan_release(struct bluetooth_hold *hold)
 {
-   bluetooth_driver_state_t *bt_st = &bluetooth_driver_st;
-   if (     bt_st
-        &&  bt_st->active
-        &&  bt_st->drv->scan_end )
-      bt_st->drv->scan_end(bt_st->data);
+   if (!hold || retro_atomic_fetch_sub_int(&hold->refs, 1) != 1)
+      return;
+   if (hold->drv->free)
+      hold->drv->free(hold->data);
+   free(hold);
 }
 
-bool driver_bluetooth_scan_done(void)
+void driver_bluetooth_scan_begin(struct bluetooth_hold *hold)
 {
-   bluetooth_driver_state_t *bt_st = &bluetooth_driver_st;
-   return      bt_st->active
-           &&  bt_st->drv->scan_done
-           &&  bt_st->drv->scan_done(bt_st->data);
+   if (hold && hold->drv->scan_begin)
+      hold->drv->scan_begin(hold->data);
+}
+
+void driver_bluetooth_scan_end(struct bluetooth_hold *hold)
+{
+   if (hold && hold->drv->scan_end)
+      hold->drv->scan_end(hold->data);
+}
+
+bool driver_bluetooth_scan_done(struct bluetooth_hold *hold)
+{
+   return hold && hold->drv->scan_done && hold->drv->scan_done(hold->data);
 }
 
 bool driver_bluetooth_adapter_generic(void)
@@ -198,12 +219,12 @@ bool bluetooth_driver_ctl(enum rarch_bluetooth_ctl_state state, void *data)
          }
          break;
       case RARCH_BLUETOOTH_CTL_DEINIT:
-        if (bt_st->data && bt_st->drv)
-        {
-           if (bt_st->drv->free)
-              bt_st->drv->free(bt_st->data);
-        }
+        if (bt_st->hold)
+           driver_bluetooth_scan_release(bt_st->hold);
+        else if (bt_st->data && bt_st->drv && bt_st->drv->free)
+           bt_st->drv->free(bt_st->data);
 
+        bt_st->hold   = NULL;
         bt_st->data   = NULL;
         bt_st->active = false;
         break;
@@ -219,11 +240,24 @@ bool bluetooth_driver_ctl(enum rarch_bluetooth_ctl_state state, void *data)
            bt_st->active = true;
            bt_st->data   = bt_st->drv->init();
 
+           if (bt_st->data && !(bt_st->hold = (struct bluetooth_hold*)
+                     calloc(1, sizeof(*bt_st->hold))))
+           {
+              if (bt_st->drv->free)
+                 bt_st->drv->free(bt_st->data);
+              bt_st->data = NULL;
+           }
            if (!bt_st->data)
            {
               RARCH_ERR("Failed to initialize bluetooth driver. Will continue without bluetooth.\n");
               bt_st->active = false;
            }
+           else
+           {
+              bt_st->hold->drv  = bt_st->drv;
+              bt_st->hold->data = bt_st->data;
+              retro_atomic_int_init(&bt_st->hold->refs, 1);
+           }
         }
         else
            bt_st->active = false;

bluetooth/bluetooth_driver.h

@@ -67,10 +67,14 @@ extern bluetooth_driver_t bluetooth_bluetoothctl;
 extern bluetooth_driver_t bluetooth_windows;
 extern bluetooth_driver_t bluetooth_bluez;
 
+struct bluetooth_hold;
+
 typedef struct
 {
    const bluetooth_driver_t *drv;
    void *data;
+   /* the driver started, as a scan holds it */
+   struct bluetooth_hold *hold;
    bool active;
 } bluetooth_driver_state_t;
 
@@ -95,9 +99,16 @@ const char* config_get_bluetooth_driver_options(void);
 #define BLUETOOTH_SCAN_POLL_US 100000
 #endif
 
-void driver_bluetooth_scan_begin(void);
-void driver_bluetooth_scan_end(void);
-bool driver_bluetooth_scan_done(void);
+/* A scan's hold on the driver, taken on the frontend's thread before
+ * the scan starts and let go, on any thread, once it has ended: a
+ * driver stopped during a scan is freed when the scan is done with it,
+ * not under it. NULL with no driver, and the calls below then do
+ * nothing. */
+struct bluetooth_hold *driver_bluetooth_scan_hold(void);
+void driver_bluetooth_scan_release(struct bluetooth_hold *hold);
+void driver_bluetooth_scan_begin(struct bluetooth_hold *hold);
+void driver_bluetooth_scan_end(struct bluetooth_hold *hold);
+bool driver_bluetooth_scan_done(struct bluetooth_hold *hold);
 bool driver_bluetooth_adapter_generic(void);
 
 void driver_bluetooth_get_devices(struct string_list *list);

samples/tasks/bluetooth_scan/bluetooth_scan_test.c

@@ -45,16 +45,35 @@ static long long mono_ms(void)
    return (long long)ts.tv_sec * 1000 + ts.tv_nsec / 1000000;
 }
 
-void driver_bluetooth_scan_begin(void)
-{ begin_at = mono_ms(); retro_atomic_fetch_add_int(&begins, 1); }
-void driver_bluetooth_scan_end(void)
-{ end_at = mono_ms(); retro_atomic_fetch_add_int(&ends, 1); }
+/* the driver's hold: every call on it before its release */
+struct bluetooth_hold { int unused; };
+static struct bluetooth_hold the_hold;
+static retro_atomic_int_t holds    = RETRO_ATOMIC_INT_INITIALIZER(0);
+static retro_atomic_int_t releases = RETRO_ATOMIC_INT_INITIALIZER(0);
+static retro_atomic_int_t misuse   = RETRO_ATOMIC_INT_INITIALIZER(0);
+
+static void hold_used(struct bluetooth_hold *hold)
+{
+   if (     hold != &the_hold
+         || retro_atomic_load_acquire_int(&releases)
+            >= retro_atomic_load_acquire_int(&holds))
+      retro_atomic_fetch_add_int(&misuse, 1);
+}
+
+struct bluetooth_hold *driver_bluetooth_scan_hold(void)
+{ retro_atomic_fetch_add_int(&holds, 1); return &the_hold; }
+void driver_bluetooth_scan_release(struct bluetooth_hold *hold)
+{ hold_used(hold); retro_atomic_fetch_add_int(&releases, 1); }
+void driver_bluetooth_scan_begin(struct bluetooth_hold *hold)
+{ hold_used(hold); begin_at = mono_ms(); retro_atomic_fetch_add_int(&begins, 1); }
+void driver_bluetooth_scan_end(struct bluetooth_hold *hold)
+{ hold_used(hold); end_at = mono_ms(); retro_atomic_fetch_add_int(&ends, 1); }
 const char *msg_hash_to_str(enum msg_hash_enums msg) { return "msg"; }
 
 /* the driver says it has found all it will */
 static retro_atomic_int_t driver_done = RETRO_ATOMIC_INT_INITIALIZER(0);
-bool driver_bluetooth_scan_done(void)
-{ return retro_atomic_load_acquire_int(&driver_done) != 0; }
+bool driver_bluetooth_scan_done(struct bluetooth_hold *hold)
+{ hold_used(hold); return retro_atomic_load_acquire_int(&driver_done) != 0; }
 
 /* ---- the task under test's neighbours ---- */
 
@@ -149,6 +168,11 @@ int main(void)
    check(retro_atomic_load_acquire_int(&ends) == 2
          && end_at - begin_at < 280,
          "a driver done early ends the scan before the window");
+   check(   retro_atomic_load_acquire_int(&holds) == 2
+         && retro_atomic_load_acquire_int(&releases) == 2,
+         "each scan takes the driver once and lets it go once");
+   check(!retro_atomic_load_acquire_int(&misuse),
+         "the driver is used only while held, and only the hold taken");
 
    task_queue_deinit();
 

tasks/task_bluetooth.c

@@ -13,6 +13,7 @@
  */
 
 #include <stdint.h>
+#include <stdlib.h>
 #include <string.h>
 #include <compat/strl.h>
 #include <retro_miscellaneous.h>
@@ -35,12 +36,20 @@
  * a time */
 static retro_time_t task_bluetooth_scan_deadline;
 
+typedef struct
+{
+   struct bluetooth_hold *hold;
+   bool begun;
+} task_bluetooth_scan_t;
+
 static void task_bluetooth_scan_handler(retro_task_t *task)
 {
-   if (!task->state)
+   task_bluetooth_scan_t *scan = (task_bluetooth_scan_t*)task->state;
+
+   if (!scan->begun)
    {
-      driver_bluetooth_scan_begin();
-      task->state = (void*)(uintptr_t)1;
+      driver_bluetooth_scan_begin(scan->hold);
+      scan->begun = true;
       task_bluetooth_scan_deadline = cpu_features_get_time_usec()
          + BLUETOOTH_SCAN_WINDOW_US;
       task_queue_park(task, BLUETOOTH_SCAN_POLL_US);
@@ -51,7 +60,7 @@ static void task_bluetooth_scan_handler(retro_task_t *task)
     * BLUETOOTH_SCAN_POLL_US where the driver can say it has found all
     * it will.  A cancel wakes it and ends the window. */
    if (     !(task_get_flags(task) & RETRO_TASK_FLG_CANCELLED)
-         && !driver_bluetooth_scan_done())
+         && !driver_bluetooth_scan_done(scan->hold))
    {
       retro_time_t now = cpu_features_get_time_usec();
       /* the queue runs a parked task up to half a millisecond early:
@@ -66,7 +75,9 @@ static void task_bluetooth_scan_handler(retro_task_t *task)
       }
    }
 
-   driver_bluetooth_scan_end();
+   driver_bluetooth_scan_end(scan->hold);
+   driver_bluetooth_scan_release(scan->hold);
+   free(scan);
    task->state = NULL;
 
    task_set_progress(task, 100);
@@ -78,23 +89,34 @@ static void task_bluetooth_scan_handler(retro_task_t *task)
 bool task_push_bluetooth_scan(retro_task_callback_t cb)
 {
    retro_task_t   *task = task_init();
+   task_bluetooth_scan_t *scan;
 
    if (!task)
       return false;
+   if (!(scan = (task_bluetooth_scan_t*)calloc(1, sizeof(*scan))))
+   {
+      free(task);
+      return false;
+   }
 
    /* blocking means no other task can run while this one is running,
     * which is the default */
    task->type           = TASK_TYPE_BLOCKING;
-   task->state          = NULL;
+   task->state          = scan;
    task->handler        = task_bluetooth_scan_handler;
    task->callback       = cb;
    task->title          = strdup(msg_hash_to_str(
                            MSG_SCANNING_BLUETOOTH_DEVICES));
 
+   /* taken here, on the frontend's thread, where the driver is
+    * stopped: the scan's thread cannot find it half gone */
+   scan->hold = driver_bluetooth_scan_hold();
    if (task_queue_push(task))
       return true;
 
    /* Another blocking task is already active */
+   driver_bluetooth_scan_release(scan->hold);
+   free(scan);
    task_free_title(task);
    free(task);
    return false;