Add lock to protect UnwantedInteractionBlocker
The call to 'dump' may come from any thread, and therefore could cause a
crash. Add a lock to protect this input stage.
To run the test:
adb shell -t "/data/nativetest64/inputflinger_tests/inputflinger_tests --gtest_filter='*Dump*' --gtest_repeat=100000 --gtest_break_on_failure"
Before this patch, the test failed after ~5K - ~13K iterations (took
10-20 seconds to crash).
Bug: 232645962
Test: m inputflinger_tests && adb sync data && <run the test>
Change-Id: I2a199690450bc5bb4a8576aa59075e99d37a531b
diff --git a/include/input/PrintTools.h b/include/input/PrintTools.h
index 7c3b29b..0a75278 100644
--- a/include/input/PrintTools.h
+++ b/include/input/PrintTools.h
@@ -58,4 +58,13 @@
const char* toString(bool value);
+/**
+ * Add "prefix" to the beginning of each line in the provided string
+ * "str".
+ * The string 'str' is typically multi-line.
+ * The most common use case for this function is to add some padding
+ * when dumping state.
+ */
+std::string addLinePrefix(std::string str, const std::string& prefix);
+
} // namespace android
\ No newline at end of file
diff --git a/libs/input/PrintTools.cpp b/libs/input/PrintTools.cpp
index 5d6ae4e..01f6bf5 100644
--- a/libs/input/PrintTools.cpp
+++ b/libs/input/PrintTools.cpp
@@ -17,6 +17,7 @@
#define LOG_TAG "PrintTools"
#include <input/PrintTools.h>
+#include <sstream>
namespace android {
@@ -24,4 +25,20 @@
return value ? "true" : "false";
}
+std::string addLinePrefix(std::string str, const std::string& prefix) {
+ std::stringstream ss;
+ bool newLineStarted = true;
+ for (const auto& ch : str) {
+ if (newLineStarted) {
+ ss << prefix;
+ newLineStarted = false;
+ }
+ if (ch == '\n') {
+ newLineStarted = true;
+ }
+ ss << ch;
+ }
+ return ss.str();
+}
+
} // namespace android
diff --git a/services/inputflinger/InputClassifier.cpp b/services/inputflinger/InputClassifier.cpp
index 3ea0986..8ce2f35 100644
--- a/services/inputflinger/InputClassifier.cpp
+++ b/services/inputflinger/InputClassifier.cpp
@@ -367,7 +367,7 @@
// --- InputClassifier ---
-InputClassifier::InputClassifier(InputListenerInterface& listener) : mListener(listener) {}
+InputClassifier::InputClassifier(InputListenerInterface& listener) : mQueuedListener(listener) {}
void InputClassifier::onBinderDied(void* cookie) {
InputClassifier* classifier = static_cast<InputClassifier*>(cookie);
@@ -417,55 +417,67 @@
void InputClassifier::notifyConfigurationChanged(const NotifyConfigurationChangedArgs* args) {
// pass through
- mListener.notifyConfigurationChanged(args);
+ mQueuedListener.notifyConfigurationChanged(args);
+ mQueuedListener.flush();
}
void InputClassifier::notifyKey(const NotifyKeyArgs* args) {
// pass through
- mListener.notifyKey(args);
+ mQueuedListener.notifyKey(args);
+ mQueuedListener.flush();
}
void InputClassifier::notifyMotion(const NotifyMotionArgs* args) {
- std::scoped_lock lock(mLock);
- // MotionClassifier is only used for touch events, for now
- const bool sendToMotionClassifier = mMotionClassifier && isTouchEvent(*args);
- if (!sendToMotionClassifier) {
- mListener.notifyMotion(args);
- return;
- }
-
- NotifyMotionArgs newArgs(*args);
- newArgs.classification = mMotionClassifier->classify(newArgs);
- mListener.notifyMotion(&newArgs);
+ { // acquire lock
+ std::scoped_lock lock(mLock);
+ // MotionClassifier is only used for touch events, for now
+ const bool sendToMotionClassifier = mMotionClassifier && isTouchEvent(*args);
+ if (!sendToMotionClassifier) {
+ mQueuedListener.notifyMotion(args);
+ } else {
+ NotifyMotionArgs newArgs(*args);
+ newArgs.classification = mMotionClassifier->classify(newArgs);
+ mQueuedListener.notifyMotion(&newArgs);
+ }
+ } // release lock
+ mQueuedListener.flush();
}
void InputClassifier::notifySensor(const NotifySensorArgs* args) {
// pass through
- mListener.notifySensor(args);
+ mQueuedListener.notifySensor(args);
+ mQueuedListener.flush();
}
void InputClassifier::notifyVibratorState(const NotifyVibratorStateArgs* args) {
// pass through
- mListener.notifyVibratorState(args);
+ mQueuedListener.notifyVibratorState(args);
+ mQueuedListener.flush();
}
void InputClassifier::notifySwitch(const NotifySwitchArgs* args) {
// pass through
- mListener.notifySwitch(args);
+ mQueuedListener.notifySwitch(args);
+ mQueuedListener.flush();
}
void InputClassifier::notifyDeviceReset(const NotifyDeviceResetArgs* args) {
- std::scoped_lock lock(mLock);
- if (mMotionClassifier) {
- mMotionClassifier->reset(*args);
- }
+ { // acquire lock
+ std::scoped_lock lock(mLock);
+ if (mMotionClassifier) {
+ mMotionClassifier->reset(*args);
+ }
+ } // release lock
+
// continue to next stage
- mListener.notifyDeviceReset(args);
+ mQueuedListener.notifyDeviceReset(args);
+ mQueuedListener.flush();
}
void InputClassifier::notifyPointerCaptureChanged(const NotifyPointerCaptureChangedArgs* args) {
// pass through
- mListener.notifyPointerCaptureChanged(args);
+ mQueuedListener.notifyPointerCaptureChanged(args);
+ mQueuedListener.flush();
}
void InputClassifier::setMotionClassifierLocked(
@@ -490,6 +502,10 @@
dump += "\n";
}
+void InputClassifier::monitor() {
+ std::scoped_lock lock(mLock);
+}
+
InputClassifier::~InputClassifier() {
}
diff --git a/services/inputflinger/InputClassifier.h b/services/inputflinger/InputClassifier.h
index e2a0bc2..56cf760 100644
--- a/services/inputflinger/InputClassifier.h
+++ b/services/inputflinger/InputClassifier.h
@@ -96,6 +96,9 @@
*/
virtual void dump(std::string& dump) = 0;
+ /* Called by the heatbeat to ensures that the classifier has not deadlocked. */
+ virtual void monitor() = 0;
+
InputClassifierInterface() { }
virtual ~InputClassifierInterface() { }
};
@@ -247,6 +250,7 @@
void notifyPointerCaptureChanged(const NotifyPointerCaptureChangedArgs* args) override;
void dump(std::string& dump) override;
+ void monitor() override;
~InputClassifier();
@@ -257,7 +261,7 @@
// Protect access to mMotionClassifier, since it may become null via a hidl callback
std::mutex mLock;
// The next stage to pass input events to
- InputListenerInterface& mListener;
+ QueuedInputListener mQueuedListener;
std::unique_ptr<MotionClassifierInterface> mMotionClassifier GUARDED_BY(mLock);
std::future<void> mInitializeMotionClassifier GUARDED_BY(mLock);
diff --git a/services/inputflinger/InputManager.cpp b/services/inputflinger/InputManager.cpp
index 7b03631..9767cd9 100644
--- a/services/inputflinger/InputManager.cpp
+++ b/services/inputflinger/InputManager.cpp
@@ -62,8 +62,8 @@
const sp<InputDispatcherPolicyInterface>& dispatcherPolicy) {
mDispatcher = createInputDispatcher(dispatcherPolicy);
mClassifier = std::make_unique<InputClassifier>(*mDispatcher);
- mUnwantedInteractionBlocker = std::make_unique<UnwantedInteractionBlocker>(*mClassifier);
- mReader = createInputReader(readerPolicy, *mUnwantedInteractionBlocker);
+ mBlocker = std::make_unique<UnwantedInteractionBlocker>(*mClassifier);
+ mReader = createInputReader(readerPolicy, *mBlocker);
}
InputManager::~InputManager() {
@@ -111,7 +111,7 @@
}
UnwantedInteractionBlockerInterface& InputManager::getUnwantedInteractionBlocker() {
- return *mUnwantedInteractionBlocker;
+ return *mBlocker;
}
InputClassifierInterface& InputManager::getClassifier() {
@@ -122,6 +122,13 @@
return *mDispatcher;
}
+void InputManager::monitor() {
+ mReader->monitor();
+ mBlocker->monitor();
+ mClassifier->monitor();
+ mDispatcher->monitor();
+}
+
// Used by tests only.
binder::Status InputManager::createInputChannel(const std::string& name, InputChannel* outChannel) {
IPCThreadState* ipc = IPCThreadState::self();
diff --git a/services/inputflinger/InputManager.h b/services/inputflinger/InputManager.h
index 35d2b0f..8aad35b 100644
--- a/services/inputflinger/InputManager.h
+++ b/services/inputflinger/InputManager.h
@@ -90,6 +90,9 @@
/* Gets the input dispatcher. */
virtual InputDispatcherInterface& getDispatcher() = 0;
+
+ /* Check that the input stages have not deadlocked. */
+ virtual void monitor() = 0;
};
class InputManager : public InputManagerInterface, public BnInputFlinger {
@@ -108,6 +111,7 @@
UnwantedInteractionBlockerInterface& getUnwantedInteractionBlocker() override;
InputClassifierInterface& getClassifier() override;
InputDispatcherInterface& getDispatcher() override;
+ void monitor() override;
status_t dump(int fd, const Vector<String16>& args) override;
binder::Status createInputChannel(const std::string& name, InputChannel* outChannel) override;
@@ -117,7 +121,7 @@
private:
std::unique_ptr<InputReaderInterface> mReader;
- std::unique_ptr<UnwantedInteractionBlockerInterface> mUnwantedInteractionBlocker;
+ std::unique_ptr<UnwantedInteractionBlockerInterface> mBlocker;
std::unique_ptr<InputClassifierInterface> mClassifier;
diff --git a/services/inputflinger/UnwantedInteractionBlocker.cpp b/services/inputflinger/UnwantedInteractionBlocker.cpp
index b69e16a..f57ff33 100644
--- a/services/inputflinger/UnwantedInteractionBlocker.cpp
+++ b/services/inputflinger/UnwantedInteractionBlocker.cpp
@@ -18,6 +18,7 @@
#include "UnwantedInteractionBlocker.h"
#include <android-base/stringprintf.h>
+#include <input/PrintTools.h>
#include <inttypes.h>
#include <linux/input-event-codes.h>
#include <linux/input.h>
@@ -80,47 +81,6 @@
return MT_TOOL_FINGER;
}
-static std::string addPrefix(std::string str, const std::string& prefix) {
- std::stringstream ss;
- bool newLineStarted = true;
- for (const auto& ch : str) {
- if (newLineStarted) {
- ss << prefix;
- newLineStarted = false;
- }
- if (ch == '\n') {
- newLineStarted = true;
- }
- ss << ch;
- }
- return ss.str();
-}
-
-template <typename T>
-static std::string dumpSet(const std::set<T>& v) {
- static_assert(std::is_integral<T>::value, "Only integral types can be printed.");
- std::string out;
- for (const T& entry : v) {
- out += out.empty() ? "{" : ", ";
- out += android::base::StringPrintf("%i", entry);
- }
- return out.empty() ? "{}" : (out + "}");
-}
-
-template <typename K, typename V>
-static std::string dumpMap(const std::map<K, V>& map) {
- static_assert(std::is_integral<K>::value, "Keys should have integral type to be printed.");
- static_assert(std::is_integral<V>::value, "Values should have integral type to be printed.");
- std::string out;
- for (const auto& [k, v] : map) {
- if (!out.empty()) {
- out += "\n";
- }
- out += android::base::StringPrintf("%i : %i", static_cast<int>(k), static_cast<int>(v));
- }
- return out;
-}
-
static std::string dumpDeviceInfo(const AndroidPalmFilterDeviceInfo& info) {
std::string out;
out += StringPrintf("max_x = %.2f\n", info.max_x);
@@ -168,10 +128,6 @@
return AMOTION_EVENT_ACTION_MOVE;
}
-static const char* toString(bool value) {
- return value ? "true" : "false";
-}
-
std::string toString(const ::ui::InProgressTouchEvdev& touch) {
return StringPrintf("x=%.1f, y=%.1f, tracking_id=%i, slot=%zu,"
" pressure=%.1f, major=%i, minor=%i, "
@@ -356,69 +312,87 @@
UnwantedInteractionBlocker::UnwantedInteractionBlocker(InputListenerInterface& listener,
bool enablePalmRejection)
- : mListener(listener), mEnablePalmRejection(enablePalmRejection) {}
+ : mQueuedListener(listener), mEnablePalmRejection(enablePalmRejection) {}
void UnwantedInteractionBlocker::notifyConfigurationChanged(
const NotifyConfigurationChangedArgs* args) {
- mListener.notifyConfigurationChanged(args);
+ mQueuedListener.notifyConfigurationChanged(args);
+ mQueuedListener.flush();
}
void UnwantedInteractionBlocker::notifyKey(const NotifyKeyArgs* args) {
- mListener.notifyKey(args);
+ mQueuedListener.notifyKey(args);
+ mQueuedListener.flush();
}
void UnwantedInteractionBlocker::notifyMotion(const NotifyMotionArgs* args) {
- const std::vector<NotifyMotionArgs> processedArgs =
- mPreferStylusOverTouchBlocker.processMotion(*args);
- for (const NotifyMotionArgs& loopArgs : processedArgs) {
- notifyMotionInner(&loopArgs);
- }
+ { // acquire lock
+ std::scoped_lock lock(mLock);
+ const std::vector<NotifyMotionArgs> processedArgs =
+ mPreferStylusOverTouchBlocker.processMotion(*args);
+ for (const NotifyMotionArgs& loopArgs : processedArgs) {
+ notifyMotionLocked(&loopArgs);
+ }
+ } // release lock
+
+ // Call out to the next stage without holding the lock
+ mQueuedListener.flush();
}
-void UnwantedInteractionBlocker::notifyMotionInner(const NotifyMotionArgs* args) {
+void UnwantedInteractionBlocker::notifyMotionLocked(const NotifyMotionArgs* args) {
auto it = mPalmRejectors.find(args->deviceId);
const bool sendToPalmRejector = it != mPalmRejectors.end() && isFromTouchscreen(args->source);
if (!sendToPalmRejector) {
- mListener.notifyMotion(args);
+ mQueuedListener.notifyMotion(args);
return;
}
- const std::vector<NotifyMotionArgs> newMotions = it->second.processMotion(*args);
- for (const NotifyMotionArgs& newArgs : newMotions) {
- mListener.notifyMotion(&newArgs);
+ std::vector<NotifyMotionArgs> processedArgs = it->second.processMotion(*args);
+ for (const NotifyMotionArgs& loopArgs : processedArgs) {
+ mQueuedListener.notifyMotion(&loopArgs);
}
}
void UnwantedInteractionBlocker::notifySwitch(const NotifySwitchArgs* args) {
- mListener.notifySwitch(args);
+ mQueuedListener.notifySwitch(args);
+ mQueuedListener.flush();
}
void UnwantedInteractionBlocker::notifySensor(const NotifySensorArgs* args) {
- mListener.notifySensor(args);
+ mQueuedListener.notifySensor(args);
+ mQueuedListener.flush();
}
void UnwantedInteractionBlocker::notifyVibratorState(const NotifyVibratorStateArgs* args) {
- mListener.notifyVibratorState(args);
+ mQueuedListener.notifyVibratorState(args);
+ mQueuedListener.flush();
}
void UnwantedInteractionBlocker::notifyDeviceReset(const NotifyDeviceResetArgs* args) {
- auto it = mPalmRejectors.find(args->deviceId);
- if (it != mPalmRejectors.end()) {
- AndroidPalmFilterDeviceInfo info = it->second.getPalmFilterDeviceInfo();
- // Re-create the object instead of resetting it
- mPalmRejectors.erase(it);
- mPalmRejectors.emplace(args->deviceId, info);
- }
- mListener.notifyDeviceReset(args);
- mPreferStylusOverTouchBlocker.notifyDeviceReset(*args);
+ { // acquire lock
+ std::scoped_lock lock(mLock);
+ auto it = mPalmRejectors.find(args->deviceId);
+ if (it != mPalmRejectors.end()) {
+ AndroidPalmFilterDeviceInfo info = it->second.getPalmFilterDeviceInfo();
+ // Re-create the object instead of resetting it
+ mPalmRejectors.erase(it);
+ mPalmRejectors.emplace(args->deviceId, info);
+ }
+ mQueuedListener.notifyDeviceReset(args);
+ mPreferStylusOverTouchBlocker.notifyDeviceReset(*args);
+ } // release lock
+ // Send events to the next stage without holding the lock
+ mQueuedListener.flush();
}
void UnwantedInteractionBlocker::notifyPointerCaptureChanged(
const NotifyPointerCaptureChangedArgs* args) {
- mListener.notifyPointerCaptureChanged(args);
+ mQueuedListener.notifyPointerCaptureChanged(args);
+ mQueuedListener.flush();
}
void UnwantedInteractionBlocker::notifyInputDevicesChanged(
const std::vector<InputDeviceInfo>& inputDevices) {
+ std::scoped_lock lock(mLock);
if (!mEnablePalmRejection) {
// Palm rejection is disabled. Don't create any palm rejector objects.
return;
@@ -450,20 +424,23 @@
}
void UnwantedInteractionBlocker::dump(std::string& dump) {
+ std::scoped_lock lock(mLock);
dump += "UnwantedInteractionBlocker:\n";
dump += " mPreferStylusOverTouchBlocker:\n";
- dump += addPrefix(mPreferStylusOverTouchBlocker.dump(), " ");
+ dump += addLinePrefix(mPreferStylusOverTouchBlocker.dump(), " ");
dump += StringPrintf(" mEnablePalmRejection: %s\n", toString(mEnablePalmRejection));
dump += StringPrintf(" isPalmRejectionEnabled (flag value): %s\n",
toString(isPalmRejectionEnabled()));
dump += mPalmRejectors.empty() ? " mPalmRejectors: None\n" : " mPalmRejectors:\n";
for (const auto& [deviceId, palmRejector] : mPalmRejectors) {
dump += StringPrintf(" deviceId = %" PRId32 ":\n", deviceId);
- dump += addPrefix(palmRejector.dump(), " ");
+ dump += addLinePrefix(palmRejector.dump(), " ");
}
}
-void UnwantedInteractionBlocker::monitor() {}
+void UnwantedInteractionBlocker::monitor() {
+ std::scoped_lock lock(mLock);
+}
UnwantedInteractionBlocker::~UnwantedInteractionBlocker() {}
@@ -529,9 +506,9 @@
std::string SlotState::dump() const {
std::string out = "mSlotsByPointerId:\n";
- out += addPrefix(dumpMap(mSlotsByPointerId), " ") + "\n";
+ out += addLinePrefix(dumpMap(mSlotsByPointerId), " ") + "\n";
out += "mPointerIdsBySlot:\n";
- out += addPrefix(dumpMap(mPointerIdsBySlot), " ") + "\n";
+ out += addLinePrefix(dumpMap(mPointerIdsBySlot), " ") + "\n";
return out;
}
@@ -689,9 +666,9 @@
std::string PalmRejector::dump() const {
std::string out;
out += "mDeviceInfo:\n";
- out += addPrefix(dumpDeviceInfo(mDeviceInfo), " ");
+ out += addLinePrefix(dumpDeviceInfo(mDeviceInfo), " ");
out += "mSlotState:\n";
- out += addPrefix(mSlotState.dump(), " ");
+ out += addLinePrefix(mSlotState.dump(), " ");
out += "mSuppressedPointerIds: ";
out += dumpSet(mSuppressedPointerIds) + "\n";
return out;
diff --git a/services/inputflinger/UnwantedInteractionBlocker.h b/services/inputflinger/UnwantedInteractionBlocker.h
index 8a1cd72..a433764 100644
--- a/services/inputflinger/UnwantedInteractionBlocker.h
+++ b/services/inputflinger/UnwantedInteractionBlocker.h
@@ -19,6 +19,7 @@
#include <map>
#include <set>
+#include <android-base/thread_annotations.h>
#include "include/UnwantedInteractionBlockerInterface.h"
#include "ui/events/ozone/evdev/touch_filter/neural_stylus_palm_detection_filter_util.h"
#include "ui/events/ozone/evdev/touch_filter/palm_detection_filter.h"
@@ -86,18 +87,20 @@
~UnwantedInteractionBlocker();
private:
+ std::mutex mLock;
// The next stage to pass input events to
- InputListenerInterface& mListener;
+
+ QueuedInputListener mQueuedListener;
const bool mEnablePalmRejection;
// When stylus is down, ignore touch
- PreferStylusOverTouchBlocker mPreferStylusOverTouchBlocker;
+ PreferStylusOverTouchBlocker mPreferStylusOverTouchBlocker GUARDED_BY(mLock);
// Detect and reject unwanted palms on screen
// Use a separate palm rejector for every touch device.
- std::map<int32_t /*deviceId*/, PalmRejector> mPalmRejectors;
+ std::map<int32_t /*deviceId*/, PalmRejector> mPalmRejectors GUARDED_BY(mLock);
// TODO(b/210159205): delete this when simultaneous stylus and touch is supported
- void notifyMotionInner(const NotifyMotionArgs* args);
+ void notifyMotionLocked(const NotifyMotionArgs* args) REQUIRES(mLock);
};
class SlotState {
diff --git a/services/inputflinger/include/UnwantedInteractionBlockerInterface.h b/services/inputflinger/include/UnwantedInteractionBlockerInterface.h
index 2327266..1a6f847 100644
--- a/services/inputflinger/include/UnwantedInteractionBlockerInterface.h
+++ b/services/inputflinger/include/UnwantedInteractionBlockerInterface.h
@@ -39,11 +39,11 @@
/**
* Dump the state of the interaction blocker.
- * This method may be called on any thread (usually by the input manager).
+ * This method may be called on any thread (usually by the input manager on a binder thread).
*/
virtual void dump(std::string& dump) = 0;
- /* Called by the heatbeat to ensures that the dispatcher has not deadlocked. */
+ /* Called by the heatbeat to ensures that the blocker has not deadlocked. */
virtual void monitor() = 0;
UnwantedInteractionBlockerInterface() {}
diff --git a/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp b/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp
index e378096..0062f42 100644
--- a/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp
+++ b/services/inputflinger/tests/UnwantedInteractionBlocker_test.cpp
@@ -19,6 +19,7 @@
#include <gtest/gtest.h>
#include <gui/constants.h>
#include <linux/input.h>
+#include <thread>
#include "TestInputListener.h"
@@ -547,6 +548,27 @@
mBlocker->notifyMotion(&args);
}
+/**
+ * Call dump, and on another thread, try to send some motions. The blocker should
+ * not crash. On 2022 hardware, this test requires ~ 13K executions (about 20 seconds) to reproduce
+ * the original bug. This is meant to be run with "--gtest_repeat=100000 --gtest_break_on_failure"
+ * options
+ */
+TEST_F(UnwantedInteractionBlockerTest, DumpCanBeAccessedOnAnotherThread) {
+ mBlocker->notifyInputDevicesChanged({generateTestDeviceInfo()});
+ NotifyMotionArgs args1 = generateMotionArgs(0 /*downTime*/, 0 /*eventTime*/, DOWN, {{1, 2, 3}});
+ mBlocker->notifyMotion(&args1);
+ std::thread dumpThread([this]() {
+ std::string dump;
+ mBlocker->dump(dump);
+ });
+ NotifyMotionArgs args2 = generateMotionArgs(0 /*downTime*/, 1 /*eventTime*/, MOVE, {{4, 5, 6}});
+ mBlocker->notifyMotion(&args2);
+ NotifyMotionArgs args3 = generateMotionArgs(0 /*downTime*/, 2 /*eventTime*/, UP, {{4, 5, 6}});
+ mBlocker->notifyMotion(&args3);
+ dumpThread.join();
+}
+
using UnwantedInteractionBlockerTestDeathTest = UnwantedInteractionBlockerTest;
/**