Race condition in opening/closing of uhid file descriptor
When HID host is connected, uhid file descriptor is opened from bta_hh
in main thread. However, when it is disconnected, uhid file descriptor
is closed from btif_hh in jni thread.
If the HID device gets disconnected and connected in quick succession,
the race condition can cause connection to fail.
Bug: 264988156
Test: Manual
Change-Id: I9ff6196eeebdd0a1dd5245f29a174c4b28dbe1b6
diff --git a/system/bta/hh/bta_hh_act.cc b/system/bta/hh/bta_hh_act.cc
index 668dcd1..c237534 100644
--- a/system/bta/hh/bta_hh_act.cc
+++ b/system/bta/hh/bta_hh_act.cc
@@ -617,13 +617,12 @@
/* increase connection number */
bta_hh_cb.cnt_num++;
- /* initialize device driver */
- bta_hh_co_open(p_cb->hid_handle, p_cb->sub_class, p_cb->attr_mask,
- p_cb->app_id);
-
conn.status = p_cb->status;
conn.le_hid = p_cb->is_le_device;
conn.scps_supported = p_cb->scps_supported;
+ conn.sub_class = p_cb->sub_class;
+ conn.attr_mask = p_cb->attr_mask;
+ conn.app_id = p_cb->app_id;
BTM_LogHistory(kBtmLogTag, p_cb->addr, "Opened",
base::StringPrintf(
@@ -634,26 +633,25 @@
{
/* inform role manager */
bta_sys_conn_open(BTA_ID_HH, p_cb->app_id, p_cb->addr);
- }
- /* set protocol mode when not default report mode */
- if (p_cb->mode != BTA_HH_PROTO_RPT_MODE
- && !p_cb->is_le_device
- ) {
- if ((HID_HostWriteDev(dev_handle, HID_TRANS_SET_PROTOCOL,
- HID_PAR_PROTOCOL_BOOT_MODE, 0, 0, NULL)) !=
- HID_SUCCESS) {
- /* HID connection is up, while SET_PROTO fail */
- conn.status = BTA_HH_ERR_PROTO;
- (*bta_hh_cb.p_cback)(BTA_HH_OPEN_EVT, (tBTA_HH*)&conn);
- } else {
- conn.status = BTA_HH_OK;
- p_cb->w4_evt = BTA_HH_OPEN_EVT;
- }
- } else
- (*bta_hh_cb.p_cback)(BTA_HH_OPEN_EVT, (tBTA_HH*)&conn);
+ /* set protocol mode when not default report mode */
+ if (p_cb->mode != BTA_HH_PROTO_RPT_MODE) {
+ tHID_STATUS status =
+ HID_HostWriteDev(dev_handle, HID_TRANS_SET_PROTOCOL,
+ HID_PAR_PROTOCOL_BOOT_MODE, 0, 0, NULL);
+
+ if (status == HID_SUCCESS) {
+ p_cb->w4_evt = BTA_HH_SET_PROTO_EVT;
+ } else {
+ /* HID connection is up, while SET_PROTO fail */
+ conn.status = BTA_HH_ERR_PROTO;
+ }
+ }
+ }
p_cb->incoming_conn = false;
p_cb->incoming_hid_handle = BTA_HH_INVALID_HANDLE;
+
+ (*bta_hh_cb.p_cback)(BTA_HH_OPEN_EVT, (tBTA_HH*)&conn);
}
/*******************************************************************************
*
diff --git a/system/bta/include/bta_hh_api.h b/system/bta/include/bta_hh_api.h
index 2d09a3e..1265524 100644
--- a/system/bta/include/bta_hh_api.h
+++ b/system/bta/include/bta_hh_api.h
@@ -239,9 +239,11 @@
RawAddress bda; /* HID device bd address */
tBTA_HH_STATUS status; /* operation status */
uint8_t handle; /* device handle */
- bool le_hid; /* is LE devices? */
- bool scps_supported; /* scan parameter service supported */
-
+ bool le_hid; /* is LE devices? */
+ bool scps_supported; /* scan parameter service supported */
+ uint8_t sub_class; /* Cod sub class */
+ uint16_t attr_mask; /* attribute mask */
+ uint8_t app_id;
} tBTA_HH_CONN;
typedef tBTA_HH_CONN tBTA_HH_DEV_INFO;
diff --git a/system/bta/include/bta_hh_co.h b/system/bta/include/bta_hh_co.h
index 21a1662..2814aaa 100644
--- a/system/bta/include/bta_hh_co.h
+++ b/system/bta/include/bta_hh_co.h
@@ -62,10 +62,10 @@
* opened, and application may do some device specific
* initialization.
*
- * Returns void.
+ * Returns True if platform specific initialization is successful
*
******************************************************************************/
-void bta_hh_co_open(uint8_t dev_handle, uint8_t sub_class, uint16_t attr_mask,
+bool bta_hh_co_open(uint8_t dev_handle, uint8_t sub_class, uint16_t attr_mask,
uint8_t app_id);
/*******************************************************************************
diff --git a/system/btif/co/bta_hh_co.cc b/system/btif/co/bta_hh_co.cc
index 8e02889..1fb0668 100644
--- a/system/btif/co/bta_hh_co.cc
+++ b/system/btif/co/bta_hh_co.cc
@@ -352,6 +352,22 @@
return uhid_write(fd, &ev);
}
+static bool uhid_fd_open(btif_hh_device_t* p_dev) {
+ if (p_dev->fd < 0) {
+ p_dev->fd = open(dev_path, O_RDWR | O_CLOEXEC);
+ if (p_dev->fd < 0) {
+ LOG_ERROR("Failed to open uhid, err:%s", strerror(errno));
+ return false;
+ }
+ }
+
+ if (p_dev->hh_keep_polling == 0) {
+ p_dev->hh_keep_polling = 1;
+ p_dev->hh_poll_thread_id = create_thread(btif_hh_poll_event_thread, p_dev);
+ }
+ return true;
+}
+
/*******************************************************************************
*
* Function bta_hh_co_open
@@ -359,17 +375,16 @@
* Description When connection is opened, this call-out function is executed
* by HH to do platform specific initialization.
*
- * Returns void.
+ * Returns True if platform specific initialization is successful
******************************************************************************/
-void bta_hh_co_open(uint8_t dev_handle, uint8_t sub_class,
+bool bta_hh_co_open(uint8_t dev_handle, uint8_t sub_class,
tBTA_HH_ATTR_MASK attr_mask, uint8_t app_id) {
uint32_t i;
btif_hh_device_t* p_dev = NULL;
if (dev_handle == BTA_HH_INVALID_HANDLE) {
- APPL_TRACE_WARNING("%s: Oops, dev_handle (%d) is invalid...", __func__,
- dev_handle);
- return;
+ LOG_WARN("dev_handle (%d) is invalid", dev_handle);
+ return false;
}
for (i = 0; i < BTIF_HH_MAX_HID; i++) {
@@ -377,25 +392,15 @@
if (p_dev->dev_status != BTHH_CONN_STATE_UNKNOWN &&
p_dev->dev_handle == dev_handle) {
// We found a device with the same handle. Must be a device reconnected.
- APPL_TRACE_WARNING(
- "%s: Found an existing device with the same handle dev_status=%d, "
+ LOG_INFO(
+ "Found an existing device with the same handle dev_status=%d, "
"address=%s, attr_mask=0x%04x, sub_class=0x%02x, app_id=%d",
- __func__, p_dev->dev_status, ADDRESS_TO_LOGGABLE_CSTR(p_dev->bd_addr),
+ p_dev->dev_status, ADDRESS_TO_LOGGABLE_CSTR(p_dev->bd_addr),
p_dev->attr_mask, p_dev->sub_class, p_dev->app_id);
- if (p_dev->fd < 0) {
- p_dev->fd = open(dev_path, O_RDWR | O_CLOEXEC);
- if (p_dev->fd < 0) {
- APPL_TRACE_ERROR("%s: Error: failed to open uhid, err:%s", __func__,
- strerror(errno));
- return;
- } else
- APPL_TRACE_DEBUG("%s: uhid fd = %d", __func__, p_dev->fd);
+ if (!uhid_fd_open(p_dev)) {
+ return false;
}
-
- p_dev->hh_keep_polling = 1;
- p_dev->hh_poll_thread_id =
- create_thread(btif_hh_poll_event_thread, p_dev);
break;
}
p_dev = NULL;
@@ -406,6 +411,14 @@
for (i = 0; i < BTIF_HH_MAX_HID; i++) {
if (btif_hh_cb.devices[i].dev_status == BTHH_CONN_STATE_UNKNOWN) {
p_dev = &btif_hh_cb.devices[i];
+ p_dev->fd = -1;
+ p_dev->hh_keep_polling = 0;
+
+ // This is a new device, open the uhid driver now.
+ if (!uhid_fd_open(p_dev)) {
+ return false;
+ }
+
p_dev->dev_handle = dev_handle;
p_dev->attr_mask = attr_mask;
p_dev->sub_class = sub_class;
@@ -413,27 +426,14 @@
p_dev->local_vup = false;
btif_hh_cb.device_num++;
- // This is a new device,open the uhid driver now.
- p_dev->fd = open(dev_path, O_RDWR | O_CLOEXEC);
- if (p_dev->fd < 0) {
- APPL_TRACE_ERROR("%s: Error: failed to open uhid, err:%s", __func__,
- strerror(errno));
- return;
- } else {
- APPL_TRACE_DEBUG("%s: uhid fd = %d", __func__, p_dev->fd);
- p_dev->hh_keep_polling = 1;
- p_dev->hh_poll_thread_id =
- create_thread(btif_hh_poll_event_thread, p_dev);
- }
-
break;
}
}
}
if (p_dev == NULL) {
- APPL_TRACE_ERROR("%s: Error: too many HID devices are connected", __func__);
- return;
+ LOG_ERROR("Too many HID devices are connected");
+ return false;
}
p_dev->dev_status = BTHH_CONN_STATE_CONNECTED;
@@ -444,7 +444,8 @@
CHECK(p_dev->set_rpt_id_queue);
#endif // ENABLE_UHID_SET_REPORT
- APPL_TRACE_DEBUG("%s: Return device status %d", __func__, p_dev->dev_status);
+ LOG_DEBUG("Return device status %d", p_dev->dev_status);
+ return true;
}
/*******************************************************************************
diff --git a/system/btif/src/btif_hh.cc b/system/btif/src/btif_hh.cc
index 87df279..451cee1 100644
--- a/system/btif/src/btif_hh.cc
+++ b/system/btif/src/btif_hh.cc
@@ -63,9 +63,6 @@
static int btif_hh_keylockstates = 0; // The current key state of each key
-// TODO This is duplicated in header file with different value
-#define BTIF_HH_DEV_DISCONNECTED 3
-
#define BTIF_TIMEOUT_VUP_MS (3 * 1000)
/* HH request events */
@@ -360,7 +357,7 @@
*
* Returns void
******************************************************************************/
-void btif_hh_stop_vup_timer(RawAddress* bd_addr) {
+static void btif_hh_stop_vup_timer(RawAddress* bd_addr) {
btif_hh_device_t* p_dev = btif_hh_find_connected_dev_by_bda(*bd_addr);
if (p_dev != NULL) {
@@ -377,7 +374,7 @@
*
* Returns void
******************************************************************************/
-void btif_hh_start_vup_timer(const RawAddress* bd_addr) {
+static void btif_hh_start_vup_timer(const RawAddress* bd_addr) {
BTIF_TRACE_DEBUG("%s", __func__);
btif_hh_device_t* p_dev = btif_hh_find_connected_dev_by_bda(*bd_addr);
@@ -389,6 +386,66 @@
btif_hh_timer_timeout, p_dev);
}
+static void hh_connect_complete(uint8_t handle, RawAddress& bda,
+ BTIF_HH_STATUS status) {
+ bthh_connection_state_t state = BTHH_CONN_STATE_CONNECTED;
+ btif_hh_cb.status = status;
+
+ if (status != BTIF_HH_DEV_CONNECTED) {
+ state = BTHH_CONN_STATE_DISCONNECTED;
+ BTA_HhClose(handle);
+ }
+ HAL_CBACK(bt_hh_callbacks, connection_state_cb, &bda, state);
+}
+
+static void hh_open_handler(tBTA_HH_CONN& conn) {
+ LOG_DEBUG("status = %d, handle = %d", conn.status, conn.handle);
+
+ HAL_CBACK(bt_hh_callbacks, connection_state_cb, (RawAddress*)&conn.bda,
+ BTHH_CONN_STATE_CONNECTING);
+ btif_hh_cb.pending_conn_address = RawAddress::kEmpty;
+
+ if (conn.status != BTA_HH_OK) {
+ btif_dm_hh_open_failed(&conn.bda);
+ btif_hh_device_t* p_dev = btif_hh_find_dev_by_bda(conn.bda);
+ if (p_dev != NULL) {
+ btif_hh_stop_vup_timer(&(p_dev->bd_addr));
+ p_dev->dev_status = BTHH_CONN_STATE_DISCONNECTED;
+ }
+ hh_connect_complete(conn.handle, conn.bda, BTIF_HH_DEV_DISCONNECTED);
+ return;
+ }
+
+ /* Initialize device driver */
+ if (!bta_hh_co_open(conn.handle, conn.sub_class, conn.attr_mask,
+ conn.app_id)) {
+ LOG_WARN("Failed to find the uhid driver");
+ hh_connect_complete(conn.handle, conn.bda, BTIF_HH_DEV_DISCONNECTED);
+ return;
+ }
+
+ btif_hh_device_t* p_dev = btif_hh_find_connected_dev_by_handle(conn.handle);
+ if (p_dev == NULL) {
+ /* The connect request must have come from device side and exceeded the
+ * connected HID device number. */
+ LOG_WARN("Cannot find device with handle %d", conn.handle);
+ hh_connect_complete(conn.handle, conn.bda, BTIF_HH_DEV_DISCONNECTED);
+ return;
+ }
+
+ LOG_INFO("Found device, getting dscp info for handle %d", conn.handle);
+
+ p_dev->bd_addr = conn.bda;
+ p_dev->dev_status = BTHH_CONN_STATE_CONNECTED;
+ hh_connect_complete(conn.handle, conn.bda, BTIF_HH_DEV_CONNECTED);
+ // Send set_idle if the peer_device is a keyboard
+ if (check_cod(&conn.bda, COD_HID_KEYBOARD) ||
+ check_cod(&conn.bda, COD_HID_COMBO)) {
+ BTA_HhSetIdle(conn.handle, 0);
+ }
+ BTA_HhGetDscpInfo(conn.handle);
+}
+
/*******************************************************************************
*
* Function btif_hh_add_added_dev
@@ -801,65 +858,7 @@
break;
case BTA_HH_OPEN_EVT:
- BTIF_TRACE_DEBUG("BTA_HH_OPEN_EVT: status = %d, handle = %d",
- p_data->dev_status.status, p_data->dev_status.handle);
- HAL_CBACK(bt_hh_callbacks, connection_state_cb,
- (RawAddress*)&p_data->conn.bda, BTHH_CONN_STATE_CONNECTING);
- btif_hh_cb.pending_conn_address = RawAddress::kEmpty;
- if (p_data->conn.status == BTA_HH_OK) {
- p_dev = btif_hh_find_connected_dev_by_handle(p_data->conn.handle);
- if (p_dev == NULL) {
- BTIF_TRACE_WARNING(
- "BTA_HH_OPEN_EVT: Error, cannot find device with handle %d",
- p_data->conn.handle);
- btif_hh_cb.status = (BTIF_HH_STATUS)BTIF_HH_DEV_DISCONNECTED;
- // The connect request must come from device side and exceeded the
- // connected
- // HID device number.
- BTA_HhClose(p_data->conn.handle);
- HAL_CBACK(bt_hh_callbacks, connection_state_cb,
- (RawAddress*)&p_data->conn.bda,
- BTHH_CONN_STATE_DISCONNECTED);
- } else if (p_dev->fd < 0) {
- BTIF_TRACE_WARNING(
- "BTA_HH_OPEN_EVT: Error, failed to find the uhid driver...");
- p_dev->bd_addr = p_data->conn.bda;
- // remove the connection and then try again to reconnect from the
- // mouse side to recover
- btif_hh_cb.status = (BTIF_HH_STATUS)BTIF_HH_DEV_DISCONNECTED;
- BTA_HhClose(p_data->conn.handle);
- } else {
- BTIF_TRACE_WARNING(
- "BTA_HH_OPEN_EVT: Found device...Getting dscp info for handle "
- "... %d",
- p_data->conn.handle);
- p_dev->bd_addr = p_data->conn.bda;
- btif_hh_cb.status = (BTIF_HH_STATUS)BTIF_HH_DEV_CONNECTED;
- // Send set_idle if the peer_device is a keyboard
- if (check_cod(&p_data->conn.bda, COD_HID_KEYBOARD) ||
- check_cod(&p_data->conn.bda, COD_HID_COMBO))
- BTA_HhSetIdle(p_data->conn.handle, 0);
- BTA_HhGetDscpInfo(p_data->conn.handle);
- p_dev->dev_status = BTHH_CONN_STATE_CONNECTED;
- HAL_CBACK(bt_hh_callbacks, connection_state_cb, &(p_dev->bd_addr),
- p_dev->dev_status);
- }
- } else {
- RawAddress* bdaddr = &p_data->conn.bda;
- btif_dm_hh_open_failed(bdaddr);
- p_dev = btif_hh_find_dev_by_bda(*bdaddr);
- if (p_dev != NULL) {
- btif_hh_stop_vup_timer(&(p_dev->bd_addr));
- if (p_dev->fd >= 0) {
- bta_hh_co_destroy(p_dev->fd);
- p_dev->fd = -1;
- }
- p_dev->dev_status = BTHH_CONN_STATE_DISCONNECTED;
- }
- HAL_CBACK(bt_hh_callbacks, connection_state_cb,
- (RawAddress*)&p_data->conn.bda, BTHH_CONN_STATE_DISCONNECTED);
- btif_hh_cb.status = (BTIF_HH_STATUS)BTIF_HH_DEV_DISCONNECTED;
- }
+ hh_open_handler(p_data->conn);
break;
case BTA_HH_CLOSE_EVT:
diff --git a/system/test/mock/mock_btif_co_bta_hh_co.cc b/system/test/mock/mock_btif_co_bta_hh_co.cc
index 280fe83..887ef8c 100644
--- a/system/test/mock/mock_btif_co_bta_hh_co.cc
+++ b/system/test/mock/mock_btif_co_bta_hh_co.cc
@@ -57,9 +57,10 @@
const uint8_t* p_rpt, uint16_t len) {
inc_func_call_count(__func__);
}
-void bta_hh_co_open(uint8_t dev_handle, uint8_t sub_class,
+bool bta_hh_co_open(uint8_t dev_handle, uint8_t sub_class,
tBTA_HH_ATTR_MASK attr_mask, uint8_t app_id) {
inc_func_call_count(__func__);
+ return true;
}
void bta_hh_co_send_hid_info(btif_hh_device_t* p_dev, const char* dev_name,
uint16_t vendor_id, uint16_t product_id,
