From 7192e0a8e0f68ee2dd7f0df923a07f91d158569b Mon Sep 17 00:00:00 2001
From: tsaichristine <tsaichristine@google.com>
Date: Fri, 6 Dec 2019 02:20:00 -0800
Subject: [PATCH] Remove locks from StateManager to avoid race condition

A race condition is introduced in ValueMetric due to StateManager's
locks. When StateManager processes a log event, it notifies all
listeners (metric producers) if a state has changed. In ValueMetric, a
state change eventually requires querying through a Statemanager
function that is held by the same lock as StateManager::onLogEvent.

To fix this, we can remove the locks in StateManager and use the locks
in StatsLogProcessor to protect StateManager objects. However, we have
to ensure that the path to StateManager function calls all originate
from StatsLogProcessor.

Test: bit statsd_test:*
Bug: 145838132
Change-Id: Ie5b747961ffa4cd5c78e86fa5f78472e29a972dd
---
 cmds/statsd/src/state/StateManager.cpp |  9 ++-------
 cmds/statsd/src/state/StateManager.h   | 13 +++++++------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/cmds/statsd/src/state/StateManager.cpp b/cmds/statsd/src/state/StateManager.cpp
index 2fa28c9846fd2..80d398339ebb9 100644
--- a/cmds/statsd/src/state/StateManager.cpp
+++ b/cmds/statsd/src/state/StateManager.cpp
@@ -29,18 +29,15 @@ StateManager& StateManager::getInstance() {
 }
 
 void StateManager::onLogEvent(const LogEvent& event) {
-    std::lock_guard<std::mutex> lock(mMutex);
     if (mStateTrackers.find(event.GetTagId()) != mStateTrackers.end()) {
         mStateTrackers[event.GetTagId()]->onLogEvent(event);
     }
 }
 
 bool StateManager::registerListener(int32_t atomId, wp<StateListener> listener) {
-    std::lock_guard<std::mutex> lock(mMutex);
-
-    // Check if state tracker already exists
+    // Check if state tracker already exists.
     if (mStateTrackers.find(atomId) == mStateTrackers.end()) {
-        // Create a new state tracker iff atom is a state atom
+        // Create a new state tracker iff atom is a state atom.
         auto it = android::util::AtomsInfo::kStateAtomsFieldOptions.find(atomId);
         if (it != android::util::AtomsInfo::kStateAtomsFieldOptions.end()) {
             mStateTrackers[atomId] = new StateTracker(atomId, it->second);
@@ -79,8 +76,6 @@ void StateManager::unregisterListener(int32_t atomId, wp<StateListener> listener
 
 bool StateManager::getStateValue(int32_t atomId, const HashableDimensionKey& key,
                                  FieldValue* output) const {
-    std::lock_guard<std::mutex> lock(mMutex);
-
     auto it = mStateTrackers.find(atomId);
     if (it != mStateTrackers.end()) {
         return it->second->getStateValue(key, output);
diff --git a/cmds/statsd/src/state/StateManager.h b/cmds/statsd/src/state/StateManager.h
index 272724ceeb08f..a6053e6f317e6 100644
--- a/cmds/statsd/src/state/StateManager.h
+++ b/cmds/statsd/src/state/StateManager.h
@@ -27,6 +27,10 @@ namespace android {
 namespace os {
 namespace statsd {
 
+/**
+ * This class is NOT thread safe.
+ * It should only be used while StatsLogProcessor's lock is held.
+ */
 class StateManager : public virtual RefBase {
 public:
     StateManager(){};
@@ -56,13 +60,10 @@ public:
                        FieldValue* output) const;
 
     inline int getStateTrackersCount() const {
-        std::lock_guard<std::mutex> lock(mMutex);
         return mStateTrackers.size();
     }
 
     inline int getListenersCount(int32_t atomId) const {
-        std::lock_guard<std::mutex> lock(mMutex);
-
         auto it = mStateTrackers.find(atomId);
         if (it != mStateTrackers.end()) {
             return it->second->getListenersCount();
@@ -71,10 +72,10 @@ public:
     }
 
 private:
-  mutable std::mutex mMutex;
+    mutable std::mutex mMutex;
 
-  // Maps state atom ids to StateTrackers
-  std::unordered_map<int32_t, sp<StateTracker>> mStateTrackers;
+    // Maps state atom ids to StateTrackers
+    std::unordered_map<int32_t, sp<StateTracker>> mStateTrackers;
 };
 
 }  // namespace statsd