From 1983a71e2a29934ec3a7c8a0fb20e0ced0ad44dc Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Fri, 4 Jun 2021 19:27:09 +0000 Subject: Return a copy of InputDeviceLightInfo instead of pointer Refactor InputReaderInterface to return a copy instead of pointer. This will ensure that we don't read from memory that's been freed. Also, refactor InputDevice api's to return a list of infos instead of having to query each individually. In all usages so far, there's no need to get a specific info by type. Bug: 190126442 Test: atest inputflinger_tests libinput_tests Change-Id: I7f993a14259bb802e2631663c1c8bb65cc9b6702 --- include/input/InputDevice.h | 8 +-- libs/input/InputDevice.cpp | 34 +++-------- services/inputflinger/include/InputReaderBase.h | 4 +- services/inputflinger/reader/InputDevice.cpp | 15 ++--- services/inputflinger/reader/InputReader.cpp | 30 ++++----- services/inputflinger/reader/include/InputDevice.h | 2 +- services/inputflinger/reader/include/InputReader.h | 30 ++++----- services/inputflinger/tests/InputReader_test.cpp | 71 ++++++++-------------- 8 files changed, 77 insertions(+), 117 deletions(-) diff --git a/include/input/InputDevice.h b/include/input/InputDevice.h index 1fec08027f..7f0324a4a8 100644 --- a/include/input/InputDevice.h +++ b/include/input/InputDevice.h @@ -257,13 +257,9 @@ public: return mMotionRanges; } - const InputDeviceSensorInfo* getSensorInfo(InputDeviceSensorType type); + std::vector getSensors(); - const std::vector getSensorTypes(); - - const std::vector getLightIds(); - - const InputDeviceLightInfo* getLightInfo(int32_t id); + std::vector getLights(); private: int32_t mId; diff --git a/libs/input/InputDevice.cpp b/libs/input/InputDevice.cpp index 61d72adb33..30c42a3daa 100644 --- a/libs/input/InputDevice.cpp +++ b/libs/input/InputDevice.cpp @@ -247,36 +247,22 @@ void InputDeviceInfo::addLightInfo(const InputDeviceLightInfo& info) { mLights.insert_or_assign(info.id, info); } -const std::vector InputDeviceInfo::getSensorTypes() { - std::vector types; +std::vector InputDeviceInfo::getSensors() { + std::vector infos; + infos.reserve(mSensors.size()); for (const auto& [type, info] : mSensors) { - types.push_back(type); + infos.push_back(info); } - return types; + return infos; } -const InputDeviceSensorInfo* InputDeviceInfo::getSensorInfo(InputDeviceSensorType type) { - auto it = mSensors.find(type); - if (it == mSensors.end()) { - return nullptr; - } - return &it->second; -} - -const std::vector InputDeviceInfo::getLightIds() { - std::vector ids; +std::vector InputDeviceInfo::getLights() { + std::vector infos; + infos.reserve(mLights.size()); for (const auto& [id, info] : mLights) { - ids.push_back(id); - } - return ids; -} - -const InputDeviceLightInfo* InputDeviceInfo::getLightInfo(int32_t id) { - auto it = mLights.find(id); - if (it == mLights.end()) { - return nullptr; + infos.push_back(info); } - return &it->second; + return infos; } } // namespace android diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 19abfd9adf..7fdbbfdac4 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -113,9 +113,9 @@ public: /* Get battery status of a particular input device. */ virtual std::optional getBatteryStatus(int32_t deviceId) = 0; - virtual std::vector getLightIds(int32_t deviceId) = 0; + virtual std::vector getLights(int32_t deviceId) = 0; - virtual const InputDeviceLightInfo* getLightInfo(int32_t deviceId, int32_t lightId) = 0; + virtual std::vector getSensors(int32_t deviceId) = 0; /* Return true if the device can send input events to the specified display. */ virtual bool canDispatchToDisplay(int32_t deviceId, int32_t displayId) = 0; diff --git a/services/inputflinger/reader/InputDevice.cpp b/services/inputflinger/reader/InputDevice.cpp index ad503fde99..7af014cb34 100644 --- a/services/inputflinger/reader/InputDevice.cpp +++ b/services/inputflinger/reader/InputDevice.cpp @@ -89,8 +89,7 @@ void InputDevice::setEnabled(bool enabled, nsecs_t when) { } void InputDevice::dump(std::string& dump, const std::string& eventHubDevStr) { - InputDeviceInfo deviceInfo; - getDeviceInfo(&deviceInfo); + InputDeviceInfo deviceInfo = getDeviceInfo(); dump += StringPrintf(INDENT "Device %d: %s\n", deviceInfo.getId(), deviceInfo.getDisplayName().c_str()); @@ -417,15 +416,17 @@ void InputDevice::updateExternalStylusState(const StylusState& state) { for_each_mapper([state](InputMapper& mapper) { mapper.updateExternalStylusState(state); }); } -void InputDevice::getDeviceInfo(InputDeviceInfo* outDeviceInfo) { - outDeviceInfo->initialize(mId, mGeneration, mControllerNumber, mIdentifier, mAlias, mIsExternal, - mHasMic); +InputDeviceInfo InputDevice::getDeviceInfo() { + InputDeviceInfo outDeviceInfo; + outDeviceInfo.initialize(mId, mGeneration, mControllerNumber, mIdentifier, mAlias, mIsExternal, + mHasMic); for_each_mapper( - [outDeviceInfo](InputMapper& mapper) { mapper.populateDeviceInfo(outDeviceInfo); }); + [&outDeviceInfo](InputMapper& mapper) { mapper.populateDeviceInfo(&outDeviceInfo); }); if (mController) { - mController->populateDeviceInfo(outDeviceInfo); + mController->populateDeviceInfo(&outDeviceInfo); } + return outDeviceInfo; } int32_t InputDevice::getKeyCodeState(uint32_t sourceMask, int32_t keyCode) { diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index e91f84e876..10c04f606c 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -406,9 +406,7 @@ void InputReader::getExternalStylusDevicesLocked(std::vector& o for (auto& devicePair : mDevices) { std::shared_ptr& device = devicePair.second; if (device->getClasses().test(InputDeviceClass::EXTERNAL_STYLUS) && !device->isIgnored()) { - InputDeviceInfo info; - device->getDeviceInfo(&info); - outDevices.push_back(info); + outDevices.push_back(device->getDeviceInfo()); } } } @@ -498,9 +496,7 @@ std::vector InputReader::getInputDevicesLocked() const { for (const auto& [device, eventHubIds] : mDeviceToEventHubIdsMap) { if (!device->isIgnored()) { - InputDeviceInfo info; - device->getDeviceInfo(&info); - outInputDevices.push_back(info); + outInputDevices.push_back(device->getDeviceInfo()); } } return outInputDevices; @@ -695,28 +691,26 @@ std::optional InputReader::getBatteryStatus(int32_t deviceId) { return std::nullopt; } -std::vector InputReader::getLightIds(int32_t deviceId) { +std::vector InputReader::getLights(int32_t deviceId) { std::scoped_lock _l(mLock); InputDevice* device = findInputDeviceLocked(deviceId); - if (device) { - InputDeviceInfo info; - device->getDeviceInfo(&info); - return info.getLightIds(); + if (device == nullptr) { + return {}; } - return {}; + + return device->getDeviceInfo().getLights(); } -const InputDeviceLightInfo* InputReader::getLightInfo(int32_t deviceId, int32_t lightId) { +std::vector InputReader::getSensors(int32_t deviceId) { std::scoped_lock _l(mLock); InputDevice* device = findInputDeviceLocked(deviceId); - if (device) { - InputDeviceInfo info; - device->getDeviceInfo(&info); - return info.getLightInfo(lightId); + if (device == nullptr) { + return {}; } - return nullptr; + + return device->getDeviceInfo().getSensors(); } bool InputReader::setLightColor(int32_t deviceId, int32_t lightId, int32_t color) { diff --git a/services/inputflinger/reader/include/InputDevice.h b/services/inputflinger/reader/include/InputDevice.h index 291f1059c5..2f2eba78b1 100644 --- a/services/inputflinger/reader/include/InputDevice.h +++ b/services/inputflinger/reader/include/InputDevice.h @@ -80,7 +80,7 @@ public: void timeoutExpired(nsecs_t when); void updateExternalStylusState(const StylusState& state); - void getDeviceInfo(InputDeviceInfo* outDeviceInfo); + InputDeviceInfo getDeviceInfo(); int32_t getKeyCodeState(uint32_t sourceMask, int32_t keyCode); int32_t getScanCodeState(uint32_t sourceMask, int32_t scanCode); int32_t getSwitchState(uint32_t sourceMask, int32_t switchCode); diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h index bc79ccf652..a00c5af4dc 100644 --- a/services/inputflinger/reader/include/InputReader.h +++ b/services/inputflinger/reader/include/InputReader.h @@ -99,9 +99,9 @@ public: std::optional getBatteryStatus(int32_t deviceId) override; - std::vector getLightIds(int32_t deviceId) override; + std::vector getLights(int32_t deviceId) override; - const InputDeviceLightInfo* getLightInfo(int32_t deviceId, int32_t lightId) override; + std::vector getSensors(int32_t deviceId) override; bool setLightColor(int32_t deviceId, int32_t lightId, int32_t color) override; @@ -130,24 +130,24 @@ protected: // lock is already held by the input loop void updateGlobalMetaState() NO_THREAD_SAFETY_ANALYSIS override; int32_t getGlobalMetaState() NO_THREAD_SAFETY_ANALYSIS override; - void disableVirtualKeysUntil(nsecs_t time) NO_THREAD_SAFETY_ANALYSIS override; - bool shouldDropVirtualKey(nsecs_t now, int32_t keyCode, - int32_t scanCode) NO_THREAD_SAFETY_ANALYSIS override; - void fadePointer() NO_THREAD_SAFETY_ANALYSIS override; + void disableVirtualKeysUntil(nsecs_t time) REQUIRES(mReader->mLock) override; + bool shouldDropVirtualKey(nsecs_t now, int32_t keyCode, int32_t scanCode) + REQUIRES(mReader->mLock) override; + void fadePointer() REQUIRES(mReader->mLock) override; std::shared_ptr getPointerController(int32_t deviceId) - NO_THREAD_SAFETY_ANALYSIS override; - void requestTimeoutAtTime(nsecs_t when) NO_THREAD_SAFETY_ANALYSIS override; + REQUIRES(mReader->mLock) override; + void requestTimeoutAtTime(nsecs_t when) REQUIRES(mReader->mLock) override; int32_t bumpGeneration() NO_THREAD_SAFETY_ANALYSIS override; void getExternalStylusDevices(std::vector& outDevices) - NO_THREAD_SAFETY_ANALYSIS override; + REQUIRES(mReader->mLock) override; void dispatchExternalStylusState(const StylusState& outState) - NO_THREAD_SAFETY_ANALYSIS override; - InputReaderPolicyInterface* getPolicy() NO_THREAD_SAFETY_ANALYSIS override; - InputListenerInterface* getListener() NO_THREAD_SAFETY_ANALYSIS override; - EventHubInterface* getEventHub() NO_THREAD_SAFETY_ANALYSIS override; + REQUIRES(mReader->mLock) override; + InputReaderPolicyInterface* getPolicy() REQUIRES(mReader->mLock) override; + InputListenerInterface* getListener() REQUIRES(mReader->mLock) override; + EventHubInterface* getEventHub() REQUIRES(mReader->mLock) override; int32_t getNextId() NO_THREAD_SAFETY_ANALYSIS override; - void updateLedMetaState(int32_t metaState) NO_THREAD_SAFETY_ANALYSIS override; - int32_t getLedMetaState() NO_THREAD_SAFETY_ANALYSIS override; + void updateLedMetaState(int32_t metaState) REQUIRES(mReader->mLock) override; + int32_t getLedMetaState() REQUIRES(mReader->mLock) REQUIRES(mLock) override; } mContext; friend class ContextImpl; diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 7a11ca7396..73198bc5a1 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -1524,20 +1524,6 @@ protected: } }; -TEST_F(InputReaderTest, ReaderGetInputDevices) { - ASSERT_NO_FATAL_FAILURE(addDevice(1, "keyboard", InputDeviceClass::KEYBOARD, nullptr)); - ASSERT_NO_FATAL_FAILURE(addDevice(2, "ignored", Flags(0), - nullptr)); // no classes so device will be ignored - - const std::vector inputDevices = mReader->getInputDevices(); - ASSERT_EQ(1U, inputDevices.size()); - ASSERT_EQ(END_RESERVED_ID + 1, inputDevices[0].getId()); - ASSERT_STREQ("keyboard", inputDevices[0].getIdentifier().name.c_str()); - ASSERT_EQ(AINPUT_KEYBOARD_TYPE_NON_ALPHABETIC, inputDevices[0].getKeyboardType()); - ASSERT_EQ(AINPUT_SOURCE_KEYBOARD, inputDevices[0].getSources()); - ASSERT_EQ(size_t(0), inputDevices[0].getMotionRanges().size()); -} - TEST_F(InputReaderTest, PolicyGetInputDevices) { ASSERT_NO_FATAL_FAILURE(addDevice(1, "keyboard", InputDeviceClass::KEYBOARD, nullptr)); ASSERT_NO_FATAL_FAILURE(addDevice(2, "ignored", Flags(0), @@ -1550,7 +1536,7 @@ TEST_F(InputReaderTest, PolicyGetInputDevices) { ASSERT_STREQ("keyboard", inputDevices[0].getIdentifier().name.c_str()); ASSERT_EQ(AINPUT_KEYBOARD_TYPE_NON_ALPHABETIC, inputDevices[0].getKeyboardType()); ASSERT_EQ(AINPUT_SOURCE_KEYBOARD, inputDevices[0].getSources()); - ASSERT_EQ(size_t(0), inputDevices[0].getMotionRanges().size()); + ASSERT_EQ(0U, inputDevices[0].getMotionRanges().size()); } TEST_F(InputReaderTest, GetMergedInputDevices) { @@ -1571,7 +1557,7 @@ TEST_F(InputReaderTest, GetMergedInputDevices) { addDevice(eventHubIds[1], "fake2", InputDeviceClass::KEYBOARD, nullptr)); // Two devices will be merged to one input device as they have same identifier - ASSERT_EQ(1U, mReader->getInputDevices().size()); + ASSERT_EQ(1U, mFakePolicy->getInputDevices().size()); } TEST_F(InputReaderTest, GetMergedInputDevicesEnabled) { @@ -2189,7 +2175,7 @@ TEST_F(InputReaderIntegrationTest, AddNewDevice) { ASSERT_EQ(initialNumDevices + 1, mFakePolicy->getInputDevices().size()); // Find the test device by its name. - const std::vector inputDevices = mReader->getInputDevices(); + const std::vector inputDevices = mFakePolicy->getInputDevices(); const auto& it = std::find_if(inputDevices.begin(), inputDevices.end(), [&keyboard](const InputDeviceInfo& info) { @@ -2463,8 +2449,7 @@ TEST_F(InputDeviceTest, WhenNoMappersAreRegistered_DeviceIsIgnored) { ASSERT_TRUE(mDevice->isIgnored()); ASSERT_EQ(AINPUT_SOURCE_UNKNOWN, mDevice->getSources()); - InputDeviceInfo info; - mDevice->getDeviceInfo(&info); + InputDeviceInfo info = mDevice->getDeviceInfo(); ASSERT_EQ(DEVICE_ID, info.getId()); ASSERT_STREQ(DEVICE_NAME, info.getIdentifier().name.c_str()); ASSERT_EQ(AINPUT_KEYBOARD_TYPE_NONE, info.getKeyboardType()); @@ -2533,8 +2518,7 @@ TEST_F(InputDeviceTest, WhenMappersAreRegistered_DeviceIsNotIgnoredAndForwardsRe ASSERT_FALSE(mDevice->isIgnored()); ASSERT_EQ(uint32_t(AINPUT_SOURCE_KEYBOARD | AINPUT_SOURCE_TOUCHSCREEN), mDevice->getSources()); - InputDeviceInfo info; - mDevice->getDeviceInfo(&info); + InputDeviceInfo info = mDevice->getDeviceInfo(); ASSERT_EQ(DEVICE_ID, info.getId()); ASSERT_STREQ(DEVICE_NAME, info.getIdentifier().name.c_str()); ASSERT_EQ(AINPUT_KEYBOARD_TYPE_ALPHABETIC, info.getKeyboardType()); @@ -8444,8 +8428,7 @@ TEST_F(MultiTouchInputMapperTest, Process_TouchpadCapture) { ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_EQ(AINPUT_SOURCE_TOUCHPAD, mapper.getSources()); - InputDeviceInfo deviceInfo; - mDevice->getDeviceInfo(&deviceInfo); + InputDeviceInfo deviceInfo = mDevice->getDeviceInfo(); const InputDeviceInfo::MotionRange* relRangeX = deviceInfo.getMotionRange(AMOTION_EVENT_AXIS_RELATIVE_X, AINPUT_SOURCE_TOUCHPAD); @@ -8755,12 +8738,12 @@ TEST_F(LightControllerTest, MonoLight) { PeripheralController& controller = addControllerAndConfigure(); InputDeviceInfo info; controller.populateDeviceInfo(&info); - const auto& ids = info.getLightIds(); - ASSERT_EQ(1UL, ids.size()); - ASSERT_EQ(InputDeviceLightType::MONO, info.getLightInfo(ids[0])->type); + std::vector lights = info.getLights(); + ASSERT_EQ(1U, lights.size()); + ASSERT_EQ(InputDeviceLightType::MONO, lights[0].type); - ASSERT_TRUE(controller.setLightColor(ids[0], LIGHT_BRIGHTNESS)); - ASSERT_EQ(controller.getLightColor(ids[0]).value_or(-1), LIGHT_BRIGHTNESS); + ASSERT_TRUE(controller.setLightColor(lights[0].id, LIGHT_BRIGHTNESS)); + ASSERT_EQ(controller.getLightColor(lights[0].id).value_or(-1), LIGHT_BRIGHTNESS); } TEST_F(LightControllerTest, RGBLight) { @@ -8786,12 +8769,12 @@ TEST_F(LightControllerTest, RGBLight) { PeripheralController& controller = addControllerAndConfigure(); InputDeviceInfo info; controller.populateDeviceInfo(&info); - const auto& ids = info.getLightIds(); - ASSERT_EQ(1UL, ids.size()); - ASSERT_EQ(InputDeviceLightType::RGB, info.getLightInfo(ids[0])->type); + std::vector lights = info.getLights(); + ASSERT_EQ(1U, lights.size()); + ASSERT_EQ(InputDeviceLightType::RGB, lights[0].type); - ASSERT_TRUE(controller.setLightColor(ids[0], LIGHT_COLOR)); - ASSERT_EQ(controller.getLightColor(ids[0]).value_or(-1), LIGHT_COLOR); + ASSERT_TRUE(controller.setLightColor(lights[0].id, LIGHT_COLOR)); + ASSERT_EQ(controller.getLightColor(lights[0].id).value_or(-1), LIGHT_COLOR); } TEST_F(LightControllerTest, MultiColorRGBLight) { @@ -8808,12 +8791,12 @@ TEST_F(LightControllerTest, MultiColorRGBLight) { PeripheralController& controller = addControllerAndConfigure(); InputDeviceInfo info; controller.populateDeviceInfo(&info); - const auto& ids = info.getLightIds(); - ASSERT_EQ(1UL, ids.size()); - ASSERT_EQ(InputDeviceLightType::MULTI_COLOR, info.getLightInfo(ids[0])->type); + std::vector lights = info.getLights(); + ASSERT_EQ(1U, lights.size()); + ASSERT_EQ(InputDeviceLightType::MULTI_COLOR, lights[0].type); - ASSERT_TRUE(controller.setLightColor(ids[0], LIGHT_COLOR)); - ASSERT_EQ(controller.getLightColor(ids[0]).value_or(-1), LIGHT_COLOR); + ASSERT_TRUE(controller.setLightColor(lights[0].id, LIGHT_COLOR)); + ASSERT_EQ(controller.getLightColor(lights[0].id).value_or(-1), LIGHT_COLOR); } TEST_F(LightControllerTest, PlayerIdLight) { @@ -8845,13 +8828,13 @@ TEST_F(LightControllerTest, PlayerIdLight) { PeripheralController& controller = addControllerAndConfigure(); InputDeviceInfo info; controller.populateDeviceInfo(&info); - const auto& ids = info.getLightIds(); - ASSERT_EQ(1UL, ids.size()); - ASSERT_EQ(InputDeviceLightType::PLAYER_ID, info.getLightInfo(ids[0])->type); + std::vector lights = info.getLights(); + ASSERT_EQ(1U, lights.size()); + ASSERT_EQ(InputDeviceLightType::PLAYER_ID, lights[0].type); - ASSERT_FALSE(controller.setLightColor(ids[0], LIGHT_COLOR)); - ASSERT_TRUE(controller.setLightPlayerId(ids[0], LIGHT_PLAYER_ID)); - ASSERT_EQ(controller.getLightPlayerId(ids[0]).value_or(-1), LIGHT_PLAYER_ID); + ASSERT_FALSE(controller.setLightColor(lights[0].id, LIGHT_COLOR)); + ASSERT_TRUE(controller.setLightPlayerId(lights[0].id, LIGHT_PLAYER_ID)); + ASSERT_EQ(controller.getLightPlayerId(lights[0].id).value_or(-1), LIGHT_PLAYER_ID); } } // namespace android -- cgit v1.2.3-59-g8ed1b