Accumulate NotifyArgs inside the lock before notifying
Currently, mQueuedListener is used both with and without a lock inside
InputReader. This is problematic because things like 'vibrate' could
modify the collection inside QueuedInputListener while it's accessing
it.
This causes a crash.
To avoid this, explicitly accumulate the events inside a lock, and then
notify all args when the lock is released, but from a swapped variable.
Bug: 293260512
Test: atest inputflinger_tests
Change-Id: Ib9891b22e6c2632395ddfcec6af3aefe3151b4c4
diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp
index 08600b2..7f63355 100644
--- a/services/inputflinger/reader/InputReader.cpp
+++ b/services/inputflinger/reader/InputReader.cpp
@@ -77,7 +77,7 @@
: mContext(this),
mEventHub(eventHub),
mPolicy(policy),
- mQueuedListener(listener),
+ mNextListener(listener),
mGlobalMetaState(AMETA_NONE),
mLedMetaState(AMETA_NONE),
mGeneration(1),
@@ -140,7 +140,7 @@
mReaderIsAliveCondition.notify_all();
if (!events.empty()) {
- notifyArgs += processEventsLocked(events.data(), events.size());
+ mPendingArgs += processEventsLocked(events.data(), events.size());
}
if (mNextTimeout != LLONG_MAX) {
@@ -150,16 +150,18 @@
ALOGD("Timeout expired, latency=%0.3fms", (now - mNextTimeout) * 0.000001f);
}
mNextTimeout = LLONG_MAX;
- notifyArgs += timeoutExpiredLocked(now);
+ mPendingArgs += timeoutExpiredLocked(now);
}
}
if (oldGeneration != mGeneration) {
inputDevicesChanged = true;
inputDevices = getInputDevicesLocked();
- notifyArgs.emplace_back(
+ mPendingArgs.emplace_back(
NotifyInputDevicesChangedArgs{mContext.getNextId(), inputDevices});
}
+
+ std::swap(notifyArgs, mPendingArgs);
} // release lock
// Send out a message that the describes the changed input devices.
@@ -175,8 +177,6 @@
}
}
- notifyAll(std::move(notifyArgs));
-
// Flush queued events out to the listener.
// This must happen outside of the lock because the listener could potentially call
// back into the InputReader's methods, such as getScanCodeState, or become blocked
@@ -184,7 +184,9 @@
// resulting in a deadlock. This situation is actually quite plausible because the
// listener is actually the input dispatcher, which calls into the window manager,
// which occasionally calls into the input reader.
- mQueuedListener.flush();
+ for (const NotifyArgs& args : notifyArgs) {
+ mNextListener.notify(args);
+ }
}
std::list<NotifyArgs> InputReader::processEventsLocked(const RawEvent* rawEvents, size_t count) {
@@ -236,8 +238,8 @@
InputDeviceIdentifier identifier = mEventHub->getDeviceIdentifier(eventHubId);
std::shared_ptr<InputDevice> device = createDeviceLocked(eventHubId, identifier);
- notifyAll(device->configure(when, mConfig, /*changes=*/{}));
- notifyAll(device->reset(when));
+ mPendingArgs += device->configure(when, mConfig, /*changes=*/{});
+ mPendingArgs += device->reset(when);
if (device->isIgnored()) {
ALOGI("Device added: id=%d, eventHubId=%d, name='%s', descriptor='%s' "
@@ -310,12 +312,10 @@
notifyExternalStylusPresenceChangedLocked();
}
- std::list<NotifyArgs> resetEvents;
if (device->hasEventHubDevices()) {
- resetEvents += device->configure(when, mConfig, /*changes=*/{});
+ mPendingArgs += device->configure(when, mConfig, /*changes=*/{});
}
- resetEvents += device->reset(when);
- notifyAll(std::move(resetEvents));
+ mPendingArgs += device->reset(when);
}
std::shared_ptr<InputDevice> InputReader::createDeviceLocked(
@@ -387,7 +387,7 @@
updateGlobalMetaStateLocked();
// Enqueue configuration changed.
- mQueuedListener.notifyConfigurationChanged({mContext.getNextId(), when});
+ mPendingArgs.emplace_back(NotifyConfigurationChangedArgs{mContext.getNextId(), when});
}
void InputReader::refreshConfigurationLocked(ConfigurationChanges changes) {
@@ -409,7 +409,7 @@
} else {
for (auto& devicePair : mDevices) {
std::shared_ptr<InputDevice>& device = devicePair.second;
- notifyAll(device->configure(now, mConfig, changes));
+ mPendingArgs += device->configure(now, mConfig, changes);
}
}
@@ -419,18 +419,13 @@
"There was no change in the pointer capture state.");
} else {
mCurrentPointerCaptureRequest = mConfig.pointerCaptureRequest;
- mQueuedListener.notifyPointerCaptureChanged(
- {mContext.getNextId(), now, mCurrentPointerCaptureRequest});
+ mPendingArgs.emplace_back(
+ NotifyPointerCaptureChangedArgs{mContext.getNextId(), now,
+ mCurrentPointerCaptureRequest});
}
}
}
-void InputReader::notifyAll(std::list<NotifyArgs>&& argsList) {
- for (const NotifyArgs& args : argsList) {
- mQueuedListener.notify(args);
- }
-}
-
void InputReader::updateGlobalMetaStateLocked() {
mGlobalMetaState = 0;
@@ -690,7 +685,7 @@
InputDevice* device = findInputDeviceLocked(deviceId);
if (device) {
- notifyAll(device->vibrate(sequence, repeat, token));
+ mPendingArgs += device->vibrate(sequence, repeat, token);
}
}
@@ -699,7 +694,7 @@
InputDevice* device = findInputDeviceLocked(deviceId);
if (device) {
- notifyAll(device->cancelVibrate(token));
+ mPendingArgs += device->cancelVibrate(token);
}
}
diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h
index 01ec7c1..e21715e 100644
--- a/services/inputflinger/reader/include/InputReader.h
+++ b/services/inputflinger/reader/include/InputReader.h
@@ -174,7 +174,14 @@
// in parallel to passing it to the InputReader.
std::shared_ptr<EventHubInterface> mEventHub;
sp<InputReaderPolicyInterface> mPolicy;
- QueuedInputListener mQueuedListener;
+
+ // The next stage that should receive the events generated inside InputReader.
+ InputListenerInterface& mNextListener;
+ // As various events are generated inside InputReader, they are stored inside this list. The
+ // list can only be accessed with the lock, so the events inside it are well-ordered.
+ // Once the reader is done working, these events will be swapped into a temporary storage and
+ // sent to the 'mNextListener' without holding the lock.
+ std::list<NotifyArgs> mPendingArgs GUARDED_BY(mLock);
InputReaderConfiguration mConfig GUARDED_BY(mLock);
@@ -242,8 +249,6 @@
ConfigurationChanges mConfigurationChangesToRefresh GUARDED_BY(mLock);
void refreshConfigurationLocked(ConfigurationChanges changes) REQUIRES(mLock);
- void notifyAll(std::list<NotifyArgs>&& argsList);
-
PointerCaptureRequest mCurrentPointerCaptureRequest GUARDED_BY(mLock);
// state queries