MtpServer: Fix concurrent access to mStorages

 * getStorage is called both internally with mMutex held and externally
   without mMutex held.  Create getStorageLocked for internal use and
   make getStorage a wrapper.

 * hasStorage is only called internally with mMutex held.  Make it a
   private method.

Change-Id: I8f73310ad6cca14cd88b8e29f20cc181b3a4fac3

2 files changed, 25 insertions, 17 deletions

diff --git a/media/mtp/MtpServer.cpp b/media/mtp/MtpServer.cpp
index 96ce81b..d5f8ff4 100644
--- a/media/mtp/MtpServer.cpp
+++ b/media/mtp/MtpServer.cpp
@@ -137,20 +137,9 @@ void MtpServer::removeStorage(MtpStorage* storage) {
 }
 
 MtpStorage* MtpServer::getStorage(MtpStorageID id) {
-    if (id == 0)
-        return mStorages.empty() ? NULL : mStorages[0];
-    for (size_t i = 0; i < mStorages.size(); i++) {
-        MtpStorage* storage = mStorages[i];
-        if (storage->getStorageID() == id)
-            return storage;
-    }
-    return NULL;
-}
+    Mutex::Autolock autoLock(mMutex);
 
-bool MtpServer::hasStorage(MtpStorageID id) {
-    if (id == 0 || id == 0xFFFFFFFF)
-        return mStorages.size() > 0;
-    return (getStorage(id) != NULL);
+    return getStorageLocked(id);
 }
 
 void MtpServer::run() {
@@ -260,6 +249,23 @@ void MtpServer::sendObjectUpdated(MtpObjectHandle handle) {
     sendEvent(MTP_EVENT_OBJECT_PROP_CHANGED, handle);
 }
 
+MtpStorage* MtpServer::getStorageLocked(MtpStorageID id) {
+    if (id == 0)
+        return mStorages.empty() ? NULL : mStorages[0];
+    for (size_t i = 0; i < mStorages.size(); i++) {
+        MtpStorage* storage = mStorages[i];
+        if (storage->getStorageID() == id)
+            return storage;
+    }
+    return NULL;
+}
+
+bool MtpServer::hasStorage(MtpStorageID id) {
+    if (id == 0 || id == 0xFFFFFFFF)
+        return mStorages.size() > 0;
+    return (getStorageLocked(id) != NULL);
+}
+
 void MtpServer::sendStoreAdded(MtpStorageID id) {
     ALOGV("sendStoreAdded %08X\n", id);
     sendEvent(MTP_EVENT_STORE_ADDED, id);
@@ -543,7 +549,7 @@ MtpResponseCode MtpServer::doGetStorageInfo() {
         return MTP_RESPONSE_INVALID_PARAMETER;
 
     MtpStorageID id = mRequest.getParameter(1);
-    MtpStorage* storage = getStorage(id);
+    MtpStorage* storage = getStorageLocked(id);
     if (!storage)
         return MTP_RESPONSE_INVALID_STORAGE_ID;
 
@@ -894,7 +900,7 @@ MtpResponseCode MtpServer::doSendObjectInfo() {
     if (mRequest.getParameterCount() < 2)
         return MTP_RESPONSE_INVALID_PARAMETER;
     MtpStorageID storageID = mRequest.getParameter(1);
-    MtpStorage* storage = getStorage(storageID);
+    MtpStorage* storage = getStorageLocked(storageID);
     MtpObjectHandle parent = mRequest.getParameter(2);
     if (!storage)
         return MTP_RESPONSE_INVALID_STORAGE_ID;
diff --git a/media/mtp/MtpServer.h b/media/mtp/MtpServer.h
index 8bd0472..a79e199 100644
--- a/media/mtp/MtpServer.h
+++ b/media/mtp/MtpServer.h
@@ -95,8 +95,6 @@ public:
     virtual             ~MtpServer();
 
     MtpStorage*         getStorage(MtpStorageID id);
-    inline bool         hasStorage() { return mStorages.size() > 0; }
-    bool                hasStorage(MtpStorageID id);
     void                addStorage(MtpStorage* storage);
     void                removeStorage(MtpStorage* storage);
 
@@ -108,6 +106,10 @@ public:
     void                sendObjectUpdated(MtpObjectHandle handle);
 
 private:
+    MtpStorage*         getStorageLocked(MtpStorageID id);
+    inline bool         hasStorage() { return mStorages.size() > 0; }
+    bool                hasStorage(MtpStorageID id);
+
     void                sendStoreAdded(MtpStorageID id);
     void                sendStoreRemoved(MtpStorageID id);
     void                sendEvent(MtpEventCode code, uint32_t param1);