Merge the 2021-03-05 SPL branch from AOSP-Partner
* security-aosp-pi-release:
Ensure looper isn't used after invalidateSensorQueue
Use strong pointer in ALooper to avoid use-after-free
Change-Id: I66e2f8892fce4adf3a61e227c6e023961aec036e
diff --git a/sensorservice/libsensorndkbridge/ALooper.cpp b/sensorservice/libsensorndkbridge/ALooper.cpp
index 1d70b5f..22ae857 100644
--- a/sensorservice/libsensorndkbridge/ALooper.cpp
+++ b/sensorservice/libsensorndkbridge/ALooper.cpp
@@ -23,12 +23,14 @@
#include <android-base/logging.h>
using android::Mutex;
+using android::sp;
+using android::wp;
ALooper::ALooper()
: mAwoken(false) {
}
-void ALooper::signalSensorEvents(ASensorEventQueue *queue) {
+void ALooper::signalSensorEvents(wp<ASensorEventQueue> queue) {
Mutex::Autolock autoLock(mLock);
mReadyQueues.insert(queue);
mCondition.signal();
@@ -72,8 +74,11 @@
if (!mReadyQueues.empty()) {
result = ALOOPER_POLL_CALLBACK;
- for (auto queue : mReadyQueues) {
- queue->dispatchCallback();
+ for (auto& queue : mReadyQueues) {
+ sp<ASensorEventQueue> promotedQueue = queue.promote();
+ if (promotedQueue != nullptr) {
+ promotedQueue->dispatchCallback();
+ }
}
mReadyQueues.clear();
@@ -87,8 +92,7 @@
return result;
}
-void ALooper::invalidateSensorQueue(ASensorEventQueue *queue) {
+void ALooper::invalidateSensorQueue(wp<ASensorEventQueue> queue) {
Mutex::Autolock autoLock(mLock);
mReadyQueues.erase(queue);
}
-
diff --git a/sensorservice/libsensorndkbridge/ALooper.h b/sensorservice/libsensorndkbridge/ALooper.h
index 601eca9..aa14e03 100644
--- a/sensorservice/libsensorndkbridge/ALooper.h
+++ b/sensorservice/libsensorndkbridge/ALooper.h
@@ -21,6 +21,7 @@
#include <android-base/macros.h>
#include <utils/Condition.h>
#include <utils/Mutex.h>
+#include <utils/RefBase.h>
#include <set>
@@ -29,18 +30,18 @@
struct ALooper {
ALooper();
- void signalSensorEvents(ASensorEventQueue *queue);
+ void signalSensorEvents(android::wp<ASensorEventQueue> queue);
void wake();
int pollOnce(int timeoutMillis, int *outFd, int *outEvents, void **outData);
- void invalidateSensorQueue(ASensorEventQueue *queue);
+ void invalidateSensorQueue(android::wp<ASensorEventQueue> queue);
private:
android::Mutex mLock;
android::Condition mCondition;
- std::set<ASensorEventQueue *> mReadyQueues;
+ std::set<android::wp<ASensorEventQueue>> mReadyQueues;
bool mAwoken;
DISALLOW_COPY_AND_ASSIGN(ALooper);
diff --git a/sensorservice/libsensorndkbridge/ASensorEventQueue.cpp b/sensorservice/libsensorndkbridge/ASensorEventQueue.cpp
index 108193a..bb31653 100644
--- a/sensorservice/libsensorndkbridge/ASensorEventQueue.cpp
+++ b/sensorservice/libsensorndkbridge/ASensorEventQueue.cpp
@@ -33,8 +33,8 @@
ALooper *looper, ALooper_callbackFunc callback, void *data)
: mLooper(looper),
mCallback(callback),
- mData(data) {
-}
+ mData(data),
+ mValid(true) {}
void ASensorEventQueue::setImpl(const sp<IEventQueue> &queueImpl) {
mQueueImpl = queueImpl;
@@ -105,15 +105,27 @@
LOG(VERBOSE) << "ASensorEventQueue::onEvent";
{
+ // Only lock the mutex in this block to avoid the following deadlock scenario:
+ //
+ // ASensorEventQueue::onEvent is called which grabs ASensorEventQueue::mLock followed
+ // by ALooper::mLock via ALooper::signalSensorEvents.
+ //
+ // Meanwhile
+ //
+ // ASensorEventQueue::dispatchCallback is invoked from ALooper::pollOnce which has
+ // has ALooper::mLock locked and the dispatched callback invokes
+ // ASensorEventQueue::getEvents which would try to grab ASensorEventQueue::mLock
+ // resulting in a deadlock.
Mutex::Autolock autoLock(mLock);
-
mQueue.emplace_back();
- sensors_event_t *sensorEvent = &mQueue[mQueue.size() - 1];
- android::hardware::sensors::V1_0::implementation::convertToSensorEvent(
- event, sensorEvent);
+ sensors_event_t* sensorEvent = &mQueue[mQueue.size() - 1];
+ android::hardware::sensors::V1_0::implementation::convertToSensorEvent(event, sensorEvent);
}
- mLooper->signalSensorEvents(this);
+ Mutex::Autolock autoLock(mValidLock);
+ if (mValid) {
+ mLooper->signalSensorEvents(this);
+ }
return android::hardware::Void();
}
@@ -130,6 +142,13 @@
}
void ASensorEventQueue::invalidate() {
+ {
+ // Only lock within this context to avoid locking while calling invalidateSensorQueue which
+ // also holds a lock. This is safe to do because mValid can't be made true after it's false
+ // so onEvent will never signal new sensor events after mValid is false.
+ Mutex::Autolock autoLock(mValidLock);
+ mValid = false;
+ }
mLooper->invalidateSensorQueue(this);
setImpl(nullptr);
}
diff --git a/sensorservice/libsensorndkbridge/ASensorEventQueue.h b/sensorservice/libsensorndkbridge/ASensorEventQueue.h
index 3321067..6d9f9ca 100644
--- a/sensorservice/libsensorndkbridge/ASensorEventQueue.h
+++ b/sensorservice/libsensorndkbridge/ASensorEventQueue.h
@@ -68,6 +68,9 @@
android::Mutex mLock;
std::vector<sensors_event_t> mQueue;
+ android::Mutex mValidLock;
+ bool mValid;
+
DISALLOW_COPY_AND_ASSIGN(ASensorEventQueue);
};
diff --git a/sensorservice/libsensorndkbridge/ASensorManager.h b/sensorservice/libsensorndkbridge/ASensorManager.h
index 818429d..4e91122 100644
--- a/sensorservice/libsensorndkbridge/ASensorManager.h
+++ b/sensorservice/libsensorndkbridge/ASensorManager.h
@@ -44,6 +44,8 @@
ALooper_callbackFunc callback,
void *data);
+ // This must not be called from inside ALooper_callbackFunc to avoid deadlocking inside of the
+ // ALooper.
void destroyEventQueue(ASensorEventQueue *queue);
private: