Merge "EventHub: Prevent duplicate descriptors" into android12L-devandroid12L-dev

4 files changed, 65 insertions, 14 deletions

diff --git a/services/inputflinger/reader/EventHub.cpp b/services/inputflinger/reader/EventHub.cpp
index 1c76dbb386..b87964932b 100644
--- a/services/inputflinger/reader/EventHub.cpp
+++ b/services/inputflinger/reader/EventHub.cpp
@@ -1241,7 +1241,8 @@ static std::string generateDescriptor(InputDeviceIdentifier& identifier) {
     if (!identifier.uniqueId.empty()) {
         rawDescriptor += "uniqueId:";
         rawDescriptor += identifier.uniqueId;
-    } else if (identifier.nonce != 0) {
+    }
+    if (identifier.nonce != 0) {
         rawDescriptor += StringPrintf("nonce:%04x", identifier.nonce);
     }
 
@@ -1269,16 +1270,20 @@ void EventHub::assignDescriptorLocked(InputDeviceIdentifier& identifier) {
     // of Android. In practice we sometimes get devices that cannot be uniquely
     // identified. In this case we enforce uniqueness between connected devices.
     // Ideally, we also want the descriptor to be short and relatively opaque.
+    // Note that we explicitly do not use the path or location for external devices
+    // as their path or location will change as they are plugged/unplugged or moved
+    // to different ports. We do fallback to using name and location in the case of
+    // internal devices which are detected by the vendor and product being 0 in
+    // generateDescriptor. If two identical descriptors are detected we will fallback
+    // to using a 'nonce' and incrementing it until the new descriptor no longer has
+    // a match with any existing descriptors.
 
     identifier.nonce = 0;
     std::string rawDescriptor = generateDescriptor(identifier);
-    if (identifier.uniqueId.empty()) {
-        // If it didn't have a unique id check for conflicts and enforce
-        // uniqueness if necessary.
-        while (getDeviceByDescriptorLocked(identifier.descriptor) != nullptr) {
-            identifier.nonce++;
-            rawDescriptor = generateDescriptor(identifier);
-        }
+    // Enforce that the generated descriptor is unique.
+    while (hasDeviceWithDescriptorLocked(identifier.descriptor)) {
+        identifier.nonce++;
+        rawDescriptor = generateDescriptor(identifier);
     }
     ALOGV("Created descriptor: raw=%s, cooked=%s", rawDescriptor.c_str(),
           identifier.descriptor.c_str());
@@ -1353,13 +1358,22 @@ std::vector<int32_t> EventHub::getVibratorIds(int32_t deviceId) {
     return vibrators;
 }
 
-EventHub::Device* EventHub::getDeviceByDescriptorLocked(const std::string& descriptor) const {
+/**
+ * Checks both mDevices and mOpeningDevices for a device with the descriptor passed.
+ */
+bool EventHub::hasDeviceWithDescriptorLocked(const std::string& descriptor) const {
+    for (const auto& device : mOpeningDevices) {
+        if (descriptor == device->identifier.descriptor) {
+            return true;
+        }
+    }
+
     for (const auto& [id, device] : mDevices) {
         if (descriptor == device->identifier.descriptor) {
-            return device.get();
+            return true;
         }
     }
-    return nullptr;
+    return false;
 }
 
 EventHub::Device* EventHub::getDeviceLocked(int32_t deviceId) const {
diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp
index 5120860503..fc398a2513 100644
--- a/services/inputflinger/reader/InputReader.cpp
+++ b/services/inputflinger/reader/InputReader.cpp
@@ -38,6 +38,26 @@ using android::base::StringPrintf;
 
 namespace android {
 
+/**
+ * Determines if the identifiers passed are a sub-devices. Sub-devices are physical devices
+ * that expose multiple input device paths such a keyboard that also has a touchpad input.
+ * These are separate devices with unique descriptors in EventHub, but InputReader should
+ * create a single InputDevice for them.
+ * Sub-devices are detected by the following criteria:
+ * 1. The vendor, product, bus, version, and unique id match
+ * 2. The location matches. The location is used to distinguish a single device with multiple
+ *    inputs versus the same device plugged into multiple ports.
+ */
+
+static bool isSubDevice(const InputDeviceIdentifier& identifier1,
+                        const InputDeviceIdentifier& identifier2) {
+    return (identifier1.vendor == identifier2.vendor &&
+            identifier1.product == identifier2.product && identifier1.bus == identifier2.bus &&
+            identifier1.version == identifier2.version &&
+            identifier1.uniqueId == identifier2.uniqueId &&
+            identifier1.location == identifier2.location);
+}
+
 // --- InputReader ---
 
 InputReader::InputReader(std::shared_ptr<EventHubInterface> eventHub,
@@ -277,8 +297,9 @@ void InputReader::removeDeviceLocked(nsecs_t when, int32_t eventHubId) {
 std::shared_ptr<InputDevice> InputReader::createDeviceLocked(
         int32_t eventHubId, const InputDeviceIdentifier& identifier) {
     auto deviceIt = std::find_if(mDevices.begin(), mDevices.end(), [identifier](auto& devicePair) {
-        return devicePair.second->getDescriptor().size() && identifier.descriptor.size() &&
-                devicePair.second->getDescriptor() == identifier.descriptor;
+        const InputDeviceIdentifier identifier2 =
+                devicePair.second->getDeviceInfo().getIdentifier();
+        return isSubDevice(identifier, identifier2);
     });
 
     std::shared_ptr<InputDevice> device;
diff --git a/services/inputflinger/reader/include/EventHub.h b/services/inputflinger/reader/include/EventHub.h
index 1016a2c409..1a39a5cc4d 100644
--- a/services/inputflinger/reader/include/EventHub.h
+++ b/services/inputflinger/reader/include/EventHub.h
@@ -643,7 +643,6 @@ private:
     void scanDevicesLocked() REQUIRES(mLock);
     status_t readNotifyLocked() REQUIRES(mLock);
 
-    Device* getDeviceByDescriptorLocked(const std::string& descriptor) const REQUIRES(mLock);
     Device* getDeviceLocked(int32_t deviceId) const REQUIRES(mLock);
     Device* getDeviceByPathLocked(const std::string& devicePath) const REQUIRES(mLock);
     /**
@@ -653,6 +652,9 @@ private:
     Device* getDeviceByFdLocked(int fd) const REQUIRES(mLock);
 
     int32_t getNextControllerNumberLocked(const std::string& name) REQUIRES(mLock);
+
+    bool hasDeviceWithDescriptorLocked(const std::string& descriptor) const REQUIRES(mLock);
+
     void releaseControllerNumberLocked(int32_t num) REQUIRES(mLock);
     void reportDeviceAddedForStatisticsLocked(const InputDeviceIdentifier& identifier,
                                               Flags<InputDeviceClass> classes) REQUIRES(mLock);
diff --git a/services/inputflinger/tests/EventHub_test.cpp b/services/inputflinger/tests/EventHub_test.cpp
index ef68a84fdb..6ef6e4498c 100644
--- a/services/inputflinger/tests/EventHub_test.cpp
+++ b/services/inputflinger/tests/EventHub_test.cpp
@@ -180,6 +180,20 @@ void EventHubTest::assertNoMoreEvents() {
 }
 
 /**
+ * Ensure that two identical devices get assigned unique descriptors from EventHub.
+ */
+TEST_F(EventHubTest, DevicesWithMatchingUniqueIdsAreUnique) {
+    std::unique_ptr<UinputHomeKey> keyboard2 = createUinputDevice<UinputHomeKey>();
+    int32_t deviceId2;
+    ASSERT_NO_FATAL_FAILURE(deviceId2 = waitForDeviceCreation());
+
+    ASSERT_NE(mEventHub->getDeviceIdentifier(mDeviceId).descriptor,
+              mEventHub->getDeviceIdentifier(deviceId2).descriptor);
+    keyboard2.reset();
+    waitForDeviceClose(deviceId2);
+}
+
+/**
  * Ensure that input_events are generated with monotonic clock.
  * That means input_event should receive a timestamp that is in the future of the time
  * before the event was sent.