From 0cacb87f022cc2bfa6c3b40217f9b230628f3fd7 Mon Sep 17 00:00:00 2001 From: Jeff Brown Date: Tue, 17 Aug 2010 15:59:26 -0700 Subject: [PATCH] Fix possible race conditions during channel unregistration. Previously, the input dispatcher assumed that the input channel's receive pipe file descriptor was a sufficiently unique identifier for looking up input channels in its various tables. However, it can happen that an input channel is disposed and then a new input channel is immediately created that reuses the same file descriptor. Ordinarily this is not a problem, however there is a small opportunity for a race to arise in InputQueue. When InputQueue receives an input event from the dispatcher, it generates a finishedToken that encodes the channel's receive pipe fd, and a sequence number. The finishedToken is used by the ViewRoot as a handle for the event so that it can tell the InputQueue when the event has finished being processed. Here is the race: 1. InputQueue receives an input event, assigns a new finishedToken. 2. ViewRoot begins processing the input event. 3. During processing, ViewRoot unregisters the InputChannel. 4. A new InputChannel is created and is registered with the Input Queue. This InputChannel happens to have the same receive pipe fd as the one previously registered. 5. ViewRoot tells the InputQueue that it has finished processing the input event, passing along the original finishedToken. 6. InputQueue throws an exception because the finishedToken's receive pipe fd is registered but the sequence number is incorrect so it assumes that the client has called finish spuriously. The fix is to include a unique connection id within the finishedToken so that the InputQueue can accurately confirm that the token belongs to the currently registered InputChannel rather than to an old one that happened to have the same receive pipe fd. When it notices this, it ignores the spurious finish. I've also made a couple of other small changes to avoid similar races elsewhere. This patch set also includes a fix to synthesize a finished signal when the input channel is unregistered on the client side to help keep the server and client in sync. Bug: 2834068 Change-Id: I1de34a36249ab74c359c2c67a57e333543400f7b --- include/ui/InputDispatcher.h | 2 ++ libs/ui/InputDispatcher.cpp | 32 ++++++++++++++++++++------------ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/include/ui/InputDispatcher.h b/include/ui/InputDispatcher.h index d3495fee5..2505cb0a8 100644 --- a/include/ui/InputDispatcher.h +++ b/include/ui/InputDispatcher.h @@ -554,6 +554,8 @@ private: // All registered connections mapped by receive pipe file descriptor. KeyedVector > mConnectionsByReceiveFd; + ssize_t getConnectionIndex(const sp& inputChannel); + // Active connections are connections that have a non-empty outbound queue. // We don't use a ref-counted pointer here because we explicitly abort connections // during unregistration which causes the connection's outbound queue to be cleared diff --git a/libs/ui/InputDispatcher.cpp b/libs/ui/InputDispatcher.cpp index b53f140f6..13030b56e 100644 --- a/libs/ui/InputDispatcher.cpp +++ b/libs/ui/InputDispatcher.cpp @@ -433,8 +433,7 @@ void InputDispatcher::dispatchEventToCurrentInputTargetsLocked(nsecs_t currentTi for (size_t i = 0; i < mCurrentInputTargets.size(); i++) { const InputTarget& inputTarget = mCurrentInputTargets.itemAt(i); - ssize_t connectionIndex = mConnectionsByReceiveFd.indexOfKey( - inputTarget.inputChannel->getReceivePipeFd()); + ssize_t connectionIndex = getConnectionIndex(inputTarget.inputChannel); if (connectionIndex >= 0) { sp connection = mConnectionsByReceiveFd.valueAt(connectionIndex); prepareDispatchCycleLocked(currentTime, connection, eventEntry, & inputTarget, @@ -1367,12 +1366,10 @@ status_t InputDispatcher::registerInputChannel(const sp& inputChan LOGD("channel '%s' ~ registerInputChannel", inputChannel->getName().string()); #endif - int receiveFd; { // acquire lock AutoMutex _l(mLock); - receiveFd = inputChannel->getReceivePipeFd(); - if (mConnectionsByReceiveFd.indexOfKey(receiveFd) >= 0) { + if (getConnectionIndex(inputChannel) >= 0) { LOGW("Attempted to register already registered input channel '%s'", inputChannel->getName().string()); return BAD_VALUE; @@ -1386,12 +1383,13 @@ status_t InputDispatcher::registerInputChannel(const sp& inputChan return status; } + int32_t receiveFd = inputChannel->getReceivePipeFd(); mConnectionsByReceiveFd.add(receiveFd, connection); + mPollLoop->setCallback(receiveFd, POLLIN, handleReceiveCallback, this); + runCommandsLockedInterruptible(); } // release lock - - mPollLoop->setCallback(receiveFd, POLLIN, handleReceiveCallback, this); return OK; } @@ -1400,12 +1398,10 @@ status_t InputDispatcher::unregisterInputChannel(const sp& inputCh LOGD("channel '%s' ~ unregisterInputChannel", inputChannel->getName().string()); #endif - int32_t receiveFd; { // acquire lock AutoMutex _l(mLock); - receiveFd = inputChannel->getReceivePipeFd(); - ssize_t connectionIndex = mConnectionsByReceiveFd.indexOfKey(receiveFd); + ssize_t connectionIndex = getConnectionIndex(inputChannel); if (connectionIndex < 0) { LOGW("Attempted to unregister already unregistered input channel '%s'", inputChannel->getName().string()); @@ -1417,20 +1413,32 @@ status_t InputDispatcher::unregisterInputChannel(const sp& inputCh connection->status = Connection::STATUS_ZOMBIE; + mPollLoop->removeCallback(inputChannel->getReceivePipeFd()); + nsecs_t currentTime = now(); abortDispatchCycleLocked(currentTime, connection, true /*broken*/); runCommandsLockedInterruptible(); } // release lock - mPollLoop->removeCallback(receiveFd); - // Wake the poll loop because removing the connection may have changed the current // synchronization state. mPollLoop->wake(); return OK; } +ssize_t InputDispatcher::getConnectionIndex(const sp& inputChannel) { + ssize_t connectionIndex = mConnectionsByReceiveFd.indexOfKey(inputChannel->getReceivePipeFd()); + if (connectionIndex >= 0) { + sp connection = mConnectionsByReceiveFd.valueAt(connectionIndex); + if (connection->inputChannel.get() == inputChannel.get()) { + return connectionIndex; + } + } + + return -1; +} + void InputDispatcher::activateConnectionLocked(Connection* connection) { for (size_t i = 0; i < mActiveConnections.size(); i++) { if (mActiveConnections.itemAt(i) == connection) {