From 5a4727d78b3ee98601b43c7b17e49df7948491cc Mon Sep 17 00:00:00 2001
From: Tej Singh <singhtejinder@google.com>
Date: Tue, 9 Jun 2020 18:13:12 -0700
Subject: [PATCH] Remove invalid configs from memory

Right now if we receive an invalid config update, we keep the existing
config in memory. This makes us remove the existing config so that we
don't have any config for the invalid config key.

Test: atest statsd_test
Bug: 158617758
Change-Id: I9daecb1c96e3a63fea3a45b07d1295f3b4ba452a
---
 cmds/statsd/src/StatsLogProcessor.cpp        |  2 ++
 cmds/statsd/src/StatsLogProcessor.h          |  1 +
 cmds/statsd/src/guardrail/StatsdStats.h      |  2 ++
 cmds/statsd/tests/StatsLogProcessor_test.cpp | 35 ++++++++++++++++++++
 4 files changed, 40 insertions(+)

diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp
index 095dd1e9be592..e7b32c56551a0 100644
--- a/cmds/statsd/src/StatsLogProcessor.cpp
+++ b/cmds/statsd/src/StatsLogProcessor.cpp
@@ -529,7 +529,9 @@ void StatsLogProcessor::OnConfigUpdatedLocked(
         VLOG("StatsdConfig valid");
     } else {
         // If there is any error in the config, don't use it.
+        // Remove any existing config with the same key.
         ALOGE("StatsdConfig NOT valid");
+        mMetricsManagers.erase(key);
     }
 }
 
diff --git a/cmds/statsd/src/StatsLogProcessor.h b/cmds/statsd/src/StatsLogProcessor.h
index 7090bd46635dc..23f2584655b0d 100644
--- a/cmds/statsd/src/StatsLogProcessor.h
+++ b/cmds/statsd/src/StatsLogProcessor.h
@@ -284,6 +284,7 @@ private:
     FRIEND_TEST(StatsLogProcessorTest, TestRateLimitByteSize);
     FRIEND_TEST(StatsLogProcessorTest, TestRateLimitBroadcast);
     FRIEND_TEST(StatsLogProcessorTest, TestDropWhenByteSizeTooLarge);
+    FRIEND_TEST(StatsLogProcessorTest, InvalidConfigRemoved);
     FRIEND_TEST(StatsLogProcessorTest, TestActiveConfigMetricDiskWriteRead);
     FRIEND_TEST(StatsLogProcessorTest, TestActivationOnBoot);
     FRIEND_TEST(StatsLogProcessorTest, TestActivationOnBootMultipleActivations);
diff --git a/cmds/statsd/src/guardrail/StatsdStats.h b/cmds/statsd/src/guardrail/StatsdStats.h
index 8587e1452543e..7cac026c2421b 100644
--- a/cmds/statsd/src/guardrail/StatsdStats.h
+++ b/cmds/statsd/src/guardrail/StatsdStats.h
@@ -660,6 +660,8 @@ private:
     FRIEND_TEST(StatsdStatsTest, TestAtomMetricsStats);
     FRIEND_TEST(StatsdStatsTest, TestActivationBroadcastGuardrailHit);
     FRIEND_TEST(StatsdStatsTest, TestAtomErrorStats);
+
+    FRIEND_TEST(StatsLogProcessorTest, InvalidConfigRemoved);
 };
 
 }  // namespace statsd
diff --git a/cmds/statsd/tests/StatsLogProcessor_test.cpp b/cmds/statsd/tests/StatsLogProcessor_test.cpp
index 076f32752223e..9a9702c345625 100644
--- a/cmds/statsd/tests/StatsLogProcessor_test.cpp
+++ b/cmds/statsd/tests/StatsLogProcessor_test.cpp
@@ -324,6 +324,41 @@ TEST(StatsLogProcessorTest, TestPullUidProviderSetOnConfigUpdate) {
     EXPECT_EQ(pullerManager->mPullUidProviders.find(key), pullerManager->mPullUidProviders.end());
 }
 
+TEST(StatsLogProcessorTest, InvalidConfigRemoved) {
+    // Setup simple config key corresponding to empty config.
+    StatsdStats::getInstance().reset();
+    sp<UidMap> m = new UidMap();
+    sp<StatsPullerManager> pullerManager = new StatsPullerManager();
+    m->updateMap(1, {1, 2}, {1, 2}, {String16("v1"), String16("v2")},
+                 {String16("p1"), String16("p2")}, {String16(""), String16("")});
+    sp<AlarmMonitor> anomalyAlarmMonitor;
+    sp<AlarmMonitor> subscriberAlarmMonitor;
+    StatsLogProcessor p(m, pullerManager, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
+                        [](const ConfigKey& key) { return true; },
+                        [](const int&, const vector<int64_t>&) {return true;});
+    ConfigKey key(3, 4);
+    StatsdConfig config = MakeConfig(true);
+    p.OnConfigUpdated(0, key, config);
+    EXPECT_EQ(1, p.mMetricsManagers.size());
+    EXPECT_NE(p.mMetricsManagers.find(key), p.mMetricsManagers.end());
+    // Cannot assert the size of mConfigStats since it is static and does not get cleared on reset.
+    EXPECT_NE(StatsdStats::getInstance().mConfigStats.end(),
+              StatsdStats::getInstance().mConfigStats.find(key));
+    EXPECT_EQ(0, StatsdStats::getInstance().mIceBox.size());
+
+    StatsdConfig invalidConfig = MakeConfig(true);
+    invalidConfig.clear_allowed_log_source();
+    p.OnConfigUpdated(0, key, invalidConfig);
+    EXPECT_EQ(0, p.mMetricsManagers.size());
+    // The current configs should not contain the invalid config.
+    EXPECT_EQ(StatsdStats::getInstance().mConfigStats.end(),
+              StatsdStats::getInstance().mConfigStats.find(key));
+    // Both "config" and "invalidConfig" should be in the icebox.
+    EXPECT_EQ(2, StatsdStats::getInstance().mIceBox.size());
+
+}
+
+
 TEST(StatsLogProcessorTest, TestActiveConfigMetricDiskWriteRead) {
     int uid = 1111;