Avoid potential deadlocks in SensorService

Sensor connections are held as weak pointers in SensorService, but
promoted to strong pointer with a lock held when the service needs to
operate on them. They are typically destroyed by Binder when the remote
end goes away, dropping the final reference count to 0. However, if a
remote dies while the service is holding an sp on the connection, the
destructor will be invoked when the local object goes out of scope. If
this happens with the lock held, the system will deadlock as the
destructor attempts to acquire the same lock.

Fix all occurrences of this in SensorService by creating two wrapper
classes: one that exposes only write access to the connection lists, and
one that returns a copy of a connection list, with elements promoted to
sp, and holds onto the sp references until the lock is released.

Bug: 137369322
Test: run sensors CTS
Change-Id: I68136819cc63ab54071f523d3e8b1318adf03e7e

2 files changed, 193 insertions, 129 deletions

diff --git a/services/sensorservice/SensorService.cpp b/services/sensorservice/SensorService.cpp
index c11b88eac2..14ed73deaf 100644
--- a/services/sensorservice/SensorService.cpp
+++ b/services/sensorservice/SensorService.cpp
@@ -299,14 +299,10 @@ void SensorService::onFirstRef() {
 }
 
 void SensorService::setSensorAccess(uid_t uid, bool hasAccess) {
-    SortedVector< sp<SensorEventConnection> > activeConnections;
-    populateActiveConnections(&activeConnections);
-    {
-        Mutex::Autolock _l(mLock);
-        for (size_t i = 0 ; i < activeConnections.size(); i++) {
-            if (activeConnections[i] != nullptr && activeConnections[i]->getUid() == uid) {
-                activeConnections[i]->setSensorAccess(hasAccess);
-            }
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
+    for (const sp<SensorEventConnection>& conn : connLock.getActiveConnections()) {
+        if (conn->getUid() == uid) {
+            conn->setSensorAccess(hasAccess);
         }
     }
 }
@@ -360,7 +356,7 @@ status_t SensorService::dump(int fd, const Vector<String16>& args) {
         if (args.size() > 2) {
            return INVALID_OPERATION;
         }
-        Mutex::Autolock _l(mLock);
+        ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
         SensorDevice& dev(SensorDevice::getInstance());
         if (args.size() == 2 && args[0] == String16("restrict")) {
             // If already in restricted mode. Ignore.
@@ -374,7 +370,7 @@ status_t SensorService::dump(int fd, const Vector<String16>& args) {
 
             mCurrentOperatingMode = RESTRICTED;
             // temporarily stop all sensor direct report and disable sensors
-            disableAllSensorsLocked();
+            disableAllSensorsLocked(&connLock);
             mWhiteListedPackage.setTo(String8(args[1]));
             return status_t(NO_ERROR);
         } else if (args.size() == 1 && args[0] == String16("enable")) {
@@ -382,7 +378,7 @@ status_t SensorService::dump(int fd, const Vector<String16>& args) {
             if (mCurrentOperatingMode == RESTRICTED) {
                 mCurrentOperatingMode = NORMAL;
                 // enable sensors and recover all sensor direct report
-                enableAllSensorsLocked();
+                enableAllSensorsLocked(&connLock);
             }
             if (mCurrentOperatingMode == DATA_INJECTION) {
                resetToNormalModeLocked();
@@ -473,22 +469,18 @@ status_t SensorService::dump(int fd, const Vector<String16>& args) {
             result.appendFormat("Sensor Privacy: %s\n",
                     mSensorPrivacyPolicy->isSensorPrivacyEnabled() ? "enabled" : "disabled");
 
-            result.appendFormat("%zd active connections\n", mActiveConnections.size());
-            for (size_t i=0 ; i < mActiveConnections.size() ; i++) {
-                sp<SensorEventConnection> connection(mActiveConnections[i].promote());
-                if (connection != nullptr) {
-                    result.appendFormat("Connection Number: %zu \n", i);
-                    connection->dump(result);
-                }
+            const auto& activeConnections = connLock.getActiveConnections();
+            result.appendFormat("%zd active connections\n", activeConnections.size());
+            for (size_t i=0 ; i < activeConnections.size() ; i++) {
+                result.appendFormat("Connection Number: %zu \n", i);
+                activeConnections[i]->dump(result);
             }
 
-            result.appendFormat("%zd direct connections\n", mDirectConnections.size());
-            for (size_t i = 0 ; i < mDirectConnections.size() ; i++) {
-                sp<SensorDirectConnection> connection(mDirectConnections[i].promote());
-                if (connection != nullptr) {
-                    result.appendFormat("Direct connection %zu:\n", i);
-                    connection->dump(result);
-                }
+            const auto& directConnections = connLock.getDirectConnections();
+            result.appendFormat("%zd direct connections\n", directConnections.size());
+            for (size_t i = 0 ; i < directConnections.size() ; i++) {
+                result.appendFormat("Direct connection %zu:\n", i);
+                directConnections[i]->dump(result);
             }
 
             result.appendFormat("Previous Registrations:\n");
@@ -515,17 +507,14 @@ status_t SensorService::dump(int fd, const Vector<String16>& args) {
 }
 
 void SensorService::disableAllSensors() {
-    Mutex::Autolock _l(mLock);
-    disableAllSensorsLocked();
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
+    disableAllSensorsLocked(&connLock);
 }
 
-void SensorService::disableAllSensorsLocked() {
+void SensorService::disableAllSensorsLocked(ConnectionSafeAutolock* connLock) {
     SensorDevice& dev(SensorDevice::getInstance());
-    for (auto &i : mDirectConnections) {
-        sp<SensorDirectConnection> connection(i.promote());
-        if (connection != nullptr) {
-            connection->stopAll(true /* backupRecord */);
-        }
+    for (const sp<SensorDirectConnection>& connection : connLock->getDirectConnections()) {
+        connection->stopAll(true /* backupRecord */);
     }
     dev.disableAllSensors();
     // Clear all pending flush connections for all active sensors. If one of the active
@@ -537,11 +526,11 @@ void SensorService::disableAllSensorsLocked() {
 }
 
 void SensorService::enableAllSensors() {
-    Mutex::Autolock _l(mLock);
-    enableAllSensorsLocked();
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
+    enableAllSensorsLocked(&connLock);
 }
 
-void SensorService::enableAllSensorsLocked() {
+void SensorService::enableAllSensorsLocked(ConnectionSafeAutolock* connLock) {
     // sensors should only be enabled if the operating state is not restricted and sensor
     // privacy is not enabled.
     if (mCurrentOperatingMode == RESTRICTED || mSensorPrivacyPolicy->isSensorPrivacyEnabled()) {
@@ -552,14 +541,12 @@ void SensorService::enableAllSensorsLocked() {
     }
     SensorDevice& dev(SensorDevice::getInstance());
     dev.enableAllSensors();
-    for (auto &i : mDirectConnections) {
-        sp<SensorDirectConnection> connection(i.promote());
-        if (connection != nullptr) {
-            connection->recoverAll();
-        }
+    for (const sp<SensorDirectConnection>& connection : connLock->getDirectConnections()) {
+        connection->recoverAll();
     }
 }
 
+
 // NOTE: This is a remote API - make sure all args are validated
 status_t SensorService::shellCommand(int in, int out, int err, Vector<String16>& args) {
     if (!checkCallingPermission(sManageSensorsPermission, nullptr, nullptr)) {
@@ -734,17 +721,8 @@ bool SensorService::threadLoop() {
         for (int i = 0; i < count; i++) {
              mSensorEventBuffer[i].flags = 0;
         }
+        ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
 
-        // Make a copy of the connection vector as some connections may be removed during the course
-        // of this loop (especially when one-shot sensor events are present in the sensor_event
-        // buffer). Promote all connections to StrongPointers before the lock is acquired. If the
-        // destructor of the sp gets called when the lock is acquired, it may result in a deadlock
-        // as ~SensorEventConnection() needs to acquire mLock again for cleanup. So copy all the
-        // strongPointers to a vector before the lock is acquired.
-        SortedVector< sp<SensorEventConnection> > activeConnections;
-        populateActiveConnections(&activeConnections);
-
-        Mutex::Autolock _l(mLock);
         // Poll has returned. Hold a wakelock if one of the events is from a wake up sensor. The
         // rest of this loop is under a critical section protected by mLock. Acquiring a wakeLock,
         // sending events to clients (incrementing SensorEventConnection::mWakeLockRefCount) should
@@ -818,6 +796,10 @@ bool SensorService::threadLoop() {
             }
         }
 
+        // Cache the list of active connections, since we use it in multiple places below but won't
+        // modify it here
+        const std::vector<sp<SensorEventConnection>> activeConnections = connLock.getActiveConnections();
+
         for (int i = 0; i < count; ++i) {
             // Map flush_complete_events in the buffer to SensorEventConnections which called flush
             // on the hardware sensor. mapFlushEventsToConnections[i] will be the
@@ -869,11 +851,8 @@ bool SensorService::threadLoop() {
                         ALOGE("Dynamic sensor release error.");
                     }
 
-                    size_t numConnections = activeConnections.size();
-                    for (size_t i=0 ; i < numConnections; ++i) {
-                        if (activeConnections[i] != nullptr) {
-                            activeConnections[i]->removeSensor(handle);
-                        }
+                    for (const sp<SensorEventConnection>& connection : activeConnections) {
+                        connection->removeSensor(handle);
                     }
                 }
             }
@@ -882,18 +861,14 @@ bool SensorService::threadLoop() {
         // Send our events to clients. Check the state of wake lock for each client and release the
         // lock if none of the clients need it.
         bool needsWakeLock = false;
-        size_t numConnections = activeConnections.size();
-        for (size_t i=0 ; i < numConnections; ++i) {
-            if (activeConnections[i] != nullptr) {
-                activeConnections[i]->sendEvents(mSensorEventBuffer, count, mSensorEventScratch,
-                        mMapFlushEventsToConnections);
-                needsWakeLock |= activeConnections[i]->needsWakeLock();
-                // If the connection has one-shot sensors, it may be cleaned up after first trigger.
-                // Early check for one-shot sensors.
-                if (activeConnections[i]->hasOneShotSensors()) {
-                    cleanupAutoDisabledSensorLocked(activeConnections[i], mSensorEventBuffer,
-                            count);
-                }
+        for (const sp<SensorEventConnection>& connection : activeConnections) {
+            connection->sendEvents(mSensorEventBuffer, count, mSensorEventScratch,
+                    mMapFlushEventsToConnections);
+            needsWakeLock |= connection->needsWakeLock();
+            // If the connection has one-shot sensors, it may be cleaned up after first trigger.
+            // Early check for one-shot sensors.
+            if (connection->hasOneShotSensors()) {
+                cleanupAutoDisabledSensorLocked(connection, mSensorEventBuffer, count);
             }
         }
 
@@ -912,17 +887,11 @@ sp<Looper> SensorService::getLooper() const {
 }
 
 void SensorService::resetAllWakeLockRefCounts() {
-    SortedVector< sp<SensorEventConnection> > activeConnections;
-    populateActiveConnections(&activeConnections);
-    {
-        Mutex::Autolock _l(mLock);
-        for (size_t i=0 ; i < activeConnections.size(); ++i) {
-            if (activeConnections[i] != nullptr) {
-                activeConnections[i]->resetWakeLockRefCount();
-            }
-        }
-        setWakeLockAcquiredLocked(false);
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
+    for (const sp<SensorEventConnection>& connection : connLock.getActiveConnections()) {
+        connection->resetWakeLockRefCount();
     }
+    setWakeLockAcquiredLocked(false);
 }
 
 void SensorService::setWakeLockAcquiredLocked(bool acquire) {
@@ -1144,9 +1113,7 @@ sp<ISensorEventConnection> SensorService::createSensorEventConnection(const Stri
     sp<SensorEventConnection> result(new SensorEventConnection(this, uid, connPackageName,
             requestedMode == DATA_INJECTION, connOpPackageName, hasSensorAccess));
     if (requestedMode == DATA_INJECTION) {
-        if (mActiveConnections.indexOf(result) < 0) {
-            mActiveConnections.add(result);
-        }
+        mConnectionHolder.addEventConnectionIfNotPresent(result);
         // Add the associated file descriptor to the Looper for polling whenever there is data to
         // be injected.
         result->updateLooperRegistration(mLooper);
@@ -1162,7 +1129,7 @@ int SensorService::isDataInjectionEnabled() {
 sp<ISensorEventConnection> SensorService::createSensorDirectConnection(
         const String16& opPackageName, uint32_t size, int32_t type, int32_t format,
         const native_handle *resource) {
-    Mutex::Autolock _l(mLock);
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
 
     // No new direct connections are allowed when sensor privacy is enabled
     if (mSensorPrivacyPolicy->isSensorPrivacyEnabled()) {
@@ -1190,9 +1157,8 @@ sp<ISensorEventConnection> SensorService::createSensorDirectConnection(
     }
 
     // check for duplication
-    for (auto &i : mDirectConnections) {
-        sp<SensorDirectConnection> connection(i.promote());
-        if (connection != nullptr && connection->isEquivalent(&mem)) {
+    for (const sp<SensorDirectConnection>& connection : connLock.getDirectConnections()) {
+        if (connection->isEquivalent(&mem)) {
             ALOGE("Duplicate create channel request for the same share memory");
             return nullptr;
         }
@@ -1229,7 +1195,7 @@ sp<ISensorEventConnection> SensorService::createSensorDirectConnection(
         return nullptr;
     }
 
-    SensorDirectConnection* conn = nullptr;
+    sp<SensorDirectConnection> conn;
     SensorDevice& dev(SensorDevice::getInstance());
     int channelHandle = dev.registerDirectChannel(&mem);
 
@@ -1246,7 +1212,7 @@ sp<ISensorEventConnection> SensorService::createSensorDirectConnection(
     } else {
         // add to list of direct connections
         // sensor service should never hold pointer or sp of SensorDirectConnection object.
-        mDirectConnections.add(wp<SensorDirectConnection>(conn));
+        mConnectionHolder.addDirectConnection(conn);
     }
     return conn;
 }
@@ -1358,7 +1324,7 @@ status_t SensorService::resetToNormalModeLocked() {
 }
 
 void SensorService::cleanupConnection(SensorEventConnection* c) {
-    Mutex::Autolock _l(mLock);
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
     const wp<SensorEventConnection> connection(c);
     size_t size = mActiveSensors.size();
     ALOGD_IF(DEBUG_CONNECTIONS, "%zu active sensors", size);
@@ -1391,10 +1357,10 @@ void SensorService::cleanupConnection(SensorEventConnection* c) {
         }
     }
     c->updateLooperRegistration(mLooper);
-    mActiveConnections.remove(connection);
+    mConnectionHolder.removeEventConnection(connection);
     BatteryService::cleanup(c->getUid());
     if (c->needsWakeLock()) {
-        checkWakeLockStateLocked();
+        checkWakeLockStateLocked(&connLock);
     }
 
     {
@@ -1414,7 +1380,7 @@ void SensorService::cleanupConnection(SensorDirectConnection* c) {
 
     SensorDevice& dev(SensorDevice::getInstance());
     dev.unregisterDirectChannel(c->getHalChannelHandle());
-    mDirectConnections.remove(c);
+    mConnectionHolder.removeDirectConnection(c);
 }
 
 sp<SensorInterface> SensorService::getSensorInterfaceFromHandle(int handle) const {
@@ -1433,7 +1399,7 @@ status_t SensorService::enable(const sp<SensorEventConnection>& connection,
         return BAD_VALUE;
     }
 
-    Mutex::Autolock _l(mLock);
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
     if (mCurrentOperatingMode != NORMAL
            && !isWhiteListedPackage(connection->getPackageName())) {
         return INVALID_OPERATION;
@@ -1484,7 +1450,7 @@ status_t SensorService::enable(const sp<SensorEventConnection>& connection,
                             }
                             connection->sendEvents(&event, 1, nullptr);
                             if (!connection->needsWakeLock() && mWakeLockAcquired) {
-                                checkWakeLockStateLocked();
+                                checkWakeLockStateLocked(&connLock);
                             }
                         }
                     }
@@ -1497,9 +1463,7 @@ status_t SensorService::enable(const sp<SensorEventConnection>& connection,
         BatteryService::enableSensor(connection->getUid(), handle);
         // the sensor was added (which means it wasn't already there)
         // so, see if this connection becomes active
-        if (mActiveConnections.indexOf(connection) < 0) {
-            mActiveConnections.add(connection);
-        }
+        mConnectionHolder.addEventConnectionIfNotPresent(connection);
     } else {
         ALOGW("sensor %08x already enabled in connection %p (ignoring)",
             handle, connection.get());
@@ -1603,7 +1567,7 @@ status_t SensorService::cleanupWithoutDisableLocked(
         }
         if (connection->hasAnySensor() == false) {
             connection->updateLooperRegistration(mLooper);
-            mActiveConnections.remove(connection);
+            mConnectionHolder.removeEventConnection(connection);
         }
         // see if this sensor becomes inactive
         if (rec->removeConnection(connection)) {
@@ -1762,22 +1726,19 @@ int SensorService::getTargetSdkVersion(const String16& opPackageName) {
 }
 
 void SensorService::checkWakeLockState() {
-    Mutex::Autolock _l(mLock);
-    checkWakeLockStateLocked();
+    ConnectionSafeAutolock connLock = mConnectionHolder.lock(mLock);
+    checkWakeLockStateLocked(&connLock);
 }
 
-void SensorService::checkWakeLockStateLocked() {
+void SensorService::checkWakeLockStateLocked(ConnectionSafeAutolock* connLock) {
     if (!mWakeLockAcquired) {
         return;
     }
     bool releaseLock = true;
-    for (size_t i=0 ; i<mActiveConnections.size() ; i++) {
-        sp<SensorEventConnection> connection(mActiveConnections[i].promote());
-        if (connection != nullptr) {
-            if (connection->needsWakeLock()) {
-                releaseLock = false;
-                break;
-            }
+    for (const sp<SensorEventConnection>& connection : connLock->getActiveConnections()) {
+        if (connection->needsWakeLock()) {
+            releaseLock = false;
+            break;
         }
     }
     if (releaseLock) {
@@ -1793,17 +1754,6 @@ void SensorService::sendEventsFromCache(const sp<SensorEventConnection>& connect
     }
 }
 
-void SensorService::populateActiveConnections(
-        SortedVector< sp<SensorEventConnection> >* activeConnections) {
-    Mutex::Autolock _l(mLock);
-    for (size_t i=0 ; i < mActiveConnections.size(); ++i) {
-        sp<SensorEventConnection> connection(mActiveConnections[i].promote());
-        if (connection != nullptr) {
-            activeConnections->add(connection);
-        }
-    }
-}
-
 bool SensorService::isWhiteListedPackage(const String8& packageName) {
     return (packageName.contains(mWhiteListedPackage.string()));
 }
@@ -1938,4 +1888,62 @@ binder::Status SensorService::SensorPrivacyPolicy::onSensorPrivacyChanged(bool e
     }
     return binder::Status::ok();
 }
-}; // namespace android
+
+SensorService::ConnectionSafeAutolock::ConnectionSafeAutolock(
+        SensorService::SensorConnectionHolder& holder, Mutex& mutex)
+        : mConnectionHolder(holder), mAutolock(mutex) {}
+
+template<typename ConnectionType>
+const std::vector<sp<ConnectionType>>& SensorService::ConnectionSafeAutolock::getConnectionsHelper(
+        const SortedVector<wp<ConnectionType>>& connectionList,
+        std::vector<std::vector<sp<ConnectionType>>>* referenceHolder) {
+    referenceHolder->emplace_back();
+    std::vector<sp<ConnectionType>>& connections = referenceHolder->back();
+    for (const wp<ConnectionType>& weakConnection : connectionList){
+        sp<ConnectionType> connection = weakConnection.promote();
+        if (connection != nullptr) {
+            connections.push_back(std::move(connection));
+        }
+    }
+    return connections;
+}
+
+const std::vector<sp<SensorService::SensorEventConnection>>&
+        SensorService::ConnectionSafeAutolock::getActiveConnections() {
+    return getConnectionsHelper(mConnectionHolder.mActiveConnections,
+                                &mReferencedActiveConnections);
+}
+
+const std::vector<sp<SensorService::SensorDirectConnection>>&
+        SensorService::ConnectionSafeAutolock::getDirectConnections() {
+    return getConnectionsHelper(mConnectionHolder.mDirectConnections,
+                                &mReferencedDirectConnections);
+}
+
+void SensorService::SensorConnectionHolder::addEventConnectionIfNotPresent(
+        const sp<SensorService::SensorEventConnection>& connection) {
+    if (mActiveConnections.indexOf(connection) < 0) {
+        mActiveConnections.add(connection);
+    }
+}
+
+void SensorService::SensorConnectionHolder::removeEventConnection(
+        const wp<SensorService::SensorEventConnection>& connection) {
+    mActiveConnections.remove(connection);
+}
+
+void SensorService::SensorConnectionHolder::addDirectConnection(
+        const sp<SensorService::SensorDirectConnection>& connection) {
+    mDirectConnections.add(connection);
+}
+
+void SensorService::SensorConnectionHolder::removeDirectConnection(
+        const wp<SensorService::SensorDirectConnection>& connection) {
+    mDirectConnections.remove(connection);
+}
+
+SensorService::ConnectionSafeAutolock SensorService::SensorConnectionHolder::lock(Mutex& mutex) {
+    return ConnectionSafeAutolock(*this, mutex);
+}
+
+} // namespace android
diff --git a/services/sensorservice/SensorService.h b/services/sensorservice/SensorService.h
index e6ec96dd0a..060b5eba70 100644
--- a/services/sensorservice/SensorService.h
+++ b/services/sensorservice/SensorService.h
@@ -20,6 +20,7 @@
 #include "SensorList.h"
 #include "RecentEventLogger.h"
 
+#include <android-base/macros.h>
 #include <binder/AppOpsManager.h>
 #include <binder/BinderService.h>
 #include <binder/IUidObserver.h>
@@ -42,6 +43,7 @@
 #include <sys/types.h>
 #include <unordered_map>
 #include <unordered_set>
+#include <vector>
 
 #if __clang__
 // Clang warns about SensorEventConnection::dump hiding BBinder::dump. The cause isn't fixable
@@ -95,10 +97,67 @@ private:
     friend class BinderService<SensorService>;
 
     // nested class/struct for internal use
-    class SensorRecord;
+    class ConnectionSafeAutolock;
+    class SensorConnectionHolder;
     class SensorEventAckReceiver;
+    class SensorRecord;
     class SensorRegistrationInfo;
 
+    // Promoting a SensorEventConnection or SensorDirectConnection from wp to sp must be done with
+    // mLock held, but destroying that sp must be done unlocked to avoid a race condition that
+    // causes a deadlock (remote dies while we hold a local sp, then our decStrong() call invokes
+    // the dtor -> cleanupConnection() tries to re-lock the mutex). This class ensures safe usage
+    // by wrapping a Mutex::Autolock on SensorService's mLock, plus vectors that hold promoted sp<>
+    // references until the lock is released, when they are safely destroyed.
+    // All read accesses to the connection lists in mConnectionHolder must be done via this class.
+    class ConnectionSafeAutolock final {
+    public:
+        // Returns a list of non-null promoted connection references
+        const std::vector<sp<SensorEventConnection>>& getActiveConnections();
+        const std::vector<sp<SensorDirectConnection>>& getDirectConnections();
+
+    private:
+        // Constructed via SensorConnectionHolder::lock()
+        friend class SensorConnectionHolder;
+        explicit ConnectionSafeAutolock(SensorConnectionHolder& holder, Mutex& mutex);
+        DISALLOW_IMPLICIT_CONSTRUCTORS(ConnectionSafeAutolock);
+
+        // NOTE: Order of these members is important, as the destructor for non-static members
+        // get invoked in the reverse order of their declaration. Here we are relying on the
+        // Autolock to be destroyed *before* the vectors, so the sp<> objects are destroyed without
+        // the lock held, which avoids the deadlock.
+        SensorConnectionHolder& mConnectionHolder;
+        std::vector<std::vector<sp<SensorEventConnection>>> mReferencedActiveConnections;
+        std::vector<std::vector<sp<SensorDirectConnection>>> mReferencedDirectConnections;
+        Mutex::Autolock mAutolock;
+
+        template<typename ConnectionType>
+        const std::vector<sp<ConnectionType>>& getConnectionsHelper(
+                const SortedVector<wp<ConnectionType>>& connectionList,
+                std::vector<std::vector<sp<ConnectionType>>>* referenceHolder);
+    };
+
+    // Encapsulates the collection of active SensorEventConection and SensorDirectConnection
+    // references. Write access is done through this class with mLock held, but all read access
+    // must be routed through ConnectionSafeAutolock.
+    class SensorConnectionHolder {
+    public:
+        void addEventConnectionIfNotPresent(const sp<SensorEventConnection>& connection);
+        void removeEventConnection(const wp<SensorEventConnection>& connection);
+
+        void addDirectConnection(const sp<SensorDirectConnection>& connection);
+        void removeDirectConnection(const wp<SensorDirectConnection>& connection);
+
+        // Pass in the mutex that protects this connection holder; acquires the lock and returns an
+        // object that can be used to safely read the lists of connections
+        ConnectionSafeAutolock lock(Mutex& mutex);
+
+    private:
+        friend class ConnectionSafeAutolock;
+        SortedVector< wp<SensorEventConnection> > mActiveConnections;
+        SortedVector< wp<SensorDirectConnection> > mDirectConnections;
+    };
+
     // If accessing a sensor we need to make sure the UID has access to it. If
     // the app UID is idle then it cannot access sensors and gets no trigger
     // events, no on-change events, flush event behavior does not change, and
@@ -250,7 +309,7 @@ private:
     // method checks whether all the events from these wake up sensors have been delivered to the
     // corresponding applications, if yes the wakelock is released.
     void checkWakeLockState();
-    void checkWakeLockStateLocked();
+    void checkWakeLockStateLocked(ConnectionSafeAutolock* connLock);
     bool isWakeLockAcquired();
     bool isWakeUpSensorEvent(const sensors_event_t& event) const;
 
@@ -268,10 +327,6 @@ private:
     // Send events from the event cache for this particular connection.
     void sendEventsFromCache(const sp<SensorEventConnection>& connection);
 
-    // Promote all weak referecences in mActiveConnections vector to strong references and add them
-    // to the output vector.
-    void populateActiveConnections( SortedVector< sp<SensorEventConnection> >* activeConnections);
-
     // If SensorService is operating in RESTRICTED mode, only select whitelisted packages are
     // allowed to register for or call flush on sensors. Typically only cts test packages are
     // allowed.
@@ -306,10 +361,10 @@ private:
 
     // temporarily stops all active direct connections and disables all sensors
     void disableAllSensors();
-    void disableAllSensorsLocked();
+    void disableAllSensorsLocked(ConnectionSafeAutolock* connLock);
     // restarts the previously stopped direct connections and enables all sensors
     void enableAllSensors();
-    void enableAllSensorsLocked();
+    void enableAllSensorsLocked(ConnectionSafeAutolock* connLock);
 
     static uint8_t sHmacGlobalKey[128];
     static bool sHmacGlobalKeyIsValid;
@@ -327,12 +382,13 @@ private:
     mutable Mutex mLock;
     DefaultKeyedVector<int, SensorRecord*> mActiveSensors;
     std::unordered_set<int> mActiveVirtualSensors;
-    SortedVector< wp<SensorEventConnection> > mActiveConnections;
+    SensorConnectionHolder mConnectionHolder;
     bool mWakeLockAcquired;
     sensors_event_t *mSensorEventBuffer, *mSensorEventScratch;
+    // WARNING: these SensorEventConnection instances must not be promoted to sp, except via
+    // modification to add support for them in ConnectionSafeAutolock
     wp<const SensorEventConnection> * mMapFlushEventsToConnections;
     std::unordered_map<int, SensorServiceUtil::RecentEventLogger*> mRecentEvent;
-    SortedVector< wp<SensorDirectConnection> > mDirectConnections;
     Mode mCurrentOperatingMode;
 
     // This packagaName is set when SensorService is in RESTRICTED or DATA_INJECTION mode. Only