Include AID_ROOT in system uid omission from UidMap

Bug: 343303596
Test: statsd_test
Change-Id: Ia2fff96983be37e3e6459f7c1ecf4b388c48f6e1

5 files changed, 80 insertions, 62 deletions

diff --git a/statsd/src/packages/UidMap.cpp b/statsd/src/packages/UidMap.cpp
index f6985258..d3bf71ab 100644
--- a/statsd/src/packages/UidMap.cpp
+++ b/statsd/src/packages/UidMap.cpp
@@ -77,7 +77,7 @@ const int FIELD_ID_CHANGE_PREV_VERSION_STRING_HASH = 11;
 inline bool omitUid(int32_t uid, bool omitSystemUids) {
     // If omitSystemUids is true, uids for which (uid % AID_USER_OFFSET) is in [0, AID_APP_START)
     // should be excluded.
-    return omitSystemUids && uid > 0 && uid % AID_USER_OFFSET < AID_APP_START;
+    return omitSystemUids && uid >= 0 && uid % AID_USER_OFFSET < AID_APP_START;
 }
 
 }  // namespace
@@ -327,14 +327,14 @@ size_t UidMap::getBytesUsed() const {
 
 void UidMap::writeUidMapSnapshot(int64_t timestamp, bool includeVersionStrings,
                                  bool includeInstaller, const uint8_t truncatedCertificateHashSize,
-                                 const std::set<int32_t>& interestingUids,
+                                 bool omitSystemUids, const std::set<int32_t>& interestingUids,
                                  map<string, int>* installerIndices, std::set<string>* str_set,
                                  ProtoOutputStream* proto) const {
     lock_guard<mutex> lock(mMutex);
 
     writeUidMapSnapshotLocked(timestamp, includeVersionStrings, includeInstaller,
-                              truncatedCertificateHashSize, false /* omitSystemUids */,
-                              interestingUids, installerIndices, str_set, proto);
+                              truncatedCertificateHashSize, omitSystemUids, interestingUids,
+                              installerIndices, str_set, proto);
 }
 
 void UidMap::writeUidMapSnapshotLocked(const int64_t timestamp, const bool includeVersionStrings,
diff --git a/statsd/src/packages/UidMap.h b/statsd/src/packages/UidMap.h
index 68fba8ec..4d5c6772 100644
--- a/statsd/src/packages/UidMap.h
+++ b/statsd/src/packages/UidMap.h
@@ -159,7 +159,7 @@ public:
     // str_set: if not null, add new string to the set and write str_hash to proto
     //          if null, write string to proto.
     void writeUidMapSnapshot(int64_t timestamp, bool includeVersionStrings, bool includeInstaller,
-                             const uint8_t truncatedCertificateHashSize,
+                             const uint8_t truncatedCertificateHashSize, bool omitSystemUids,
                              const std::set<int32_t>& interestingUids,
                              std::map<string, int>* installerIndices, std::set<string>* str_set,
                              ProtoOutputStream* proto) const;
diff --git a/statsd/src/subscriber/IncidentdReporter.cpp b/statsd/src/subscriber/IncidentdReporter.cpp
index 1e4e5515..979b32b6 100644
--- a/statsd/src/subscriber/IncidentdReporter.cpp
+++ b/statsd/src/subscriber/IncidentdReporter.cpp
@@ -102,10 +102,10 @@ void getProtoData(const int64_t& rule_id, int64_t metricId, const MetricDimensio
 
     if (!uids.empty()) {
         uint64_t token = headerProto.start(FIELD_TYPE_MESSAGE | FIELD_ID_PACKAGE_INFO);
-        UidMap::getInstance()->writeUidMapSnapshot(getElapsedRealtimeNs(), true, true,
-                                                   /*trucnatedCertificateHashSize*/ 0, uids,
-                                                   nullptr /*installerIndices*/,
-                                                   nullptr /*string set*/, &headerProto);
+        UidMap::getInstance()->writeUidMapSnapshot(
+                getElapsedRealtimeNs(), true, true,
+                /*trucnatedCertificateHashSize*/ 0, /*omitSystemUids*/ false, uids,
+                nullptr /*installerIndices*/, nullptr /*string set*/, &headerProto);
         headerProto.end(token);
     }
 
diff --git a/statsd/tests/UidMap_test.cpp b/statsd/tests/UidMap_test.cpp
index 3a8ae74b..ac18b562 100644
--- a/statsd/tests/UidMap_test.cpp
+++ b/statsd/tests/UidMap_test.cpp
@@ -117,49 +117,6 @@ protected:
     }
 };
 
-class UidMapTestAppendUidMapSystemUids : public UidMapTestAppendUidMapBase {
-protected:
-    static const uint64_t bucketStartTimeNs = 10000000000;  // 0:10
-    uint64_t bucketSizeNs = TimeUnitToBucketSizeInMillis(TEN_MINUTES) * 1000000LL;
-    StatsdConfig config;
-
-    void SetUp() override {
-        UidData uidData = createUidData(
-                {AID_LMKD, AID_APP_START + 1, AID_USER_OFFSET + AID_UWB,
-                 AID_USER_OFFSET + AID_APP_START + 2} /* uids */,
-                {1, 2, 3, 4} /* versions */, {"v1", "v2", "v3", "v4"} /* versionStrings */,
-                {"LMKD", "app1", "UWB", "app2"} /* apps */,
-                {"installer", "installer", "installer", "installer"} /* installers */,
-                {{}, {}, {}, {}, {}} /* certificateHashes */);
-
-        uidMap->updateMap(/* timestamp */ 1, uidData);
-
-        uidMap->updateApp(/* timestamp */ 5, "LMKD", AID_LMKD, /* versionCode */ 10, "v10",
-                          /* installer */ "", /* certificateHash */ {});
-        uidMap->updateApp(/* timestamp */ 6, "UWB", AID_USER_OFFSET + AID_UWB, /* versionCode */ 20,
-                          "v20", /* installer */ "", /* certificateHash */ {});
-
-        *config.add_atom_matcher() =
-                CreateSimpleAtomMatcher("TestAtomMatcher", util::TEST_ATOM_REPORTED);
-        *config.add_event_metric() =
-                createEventMetric("TestAtomReported", config.atom_matcher(0).id(), nullopt);
-    }
-
-    inline sp<StatsLogProcessor> createStatsLogProcessor(const StatsdConfig& config) const {
-        return CreateStatsLogProcessor(bucketStartTimeNs, bucketStartTimeNs, config, cfgKey,
-                                       /* puller */ nullptr, /* puller atomTag */ 0, uidMap);
-    }
-
-    UidMapping getUidMapping(const sp<StatsLogProcessor>& processor) const {
-        vector<uint8_t> buffer;
-        processor->onDumpReport(cfgKey, bucketStartTimeNs + bucketSizeNs + 1, false, true, ADB_DUMP,
-                                FAST, &buffer);
-        ConfigMetricsReportList reports;
-        reports.ParseFromArray(&buffer[0], buffer.size());
-        return reports.reports(0).uid_map();
-    }
-};
-
 }  // anonymous namespace
 
 TEST(UidMapTest, TestIsolatedUID) {
@@ -592,6 +549,7 @@ TEST(UidMapTest, TestMemoryGuardrail) {
     ASSERT_EQ(1U, m.mChanges.size());
 }
 
+namespace {
 class UidMapTestAppendUidMap : public UidMapTestAppendUidMapBase {
 protected:
     const shared_ptr<StatsService> service;
@@ -765,18 +723,76 @@ TEST_P(UidMapTestTruncateCertificateHash, TestCertificateHashesTruncated) {
                 UnorderedPointwise(EqPackageInfo(), expectedPackageInfos));
 }
 
+class UidMapTestAppendUidMapSystemUids : public UidMapTestAppendUidMapBase {
+protected:
+    static const uint64_t bucketStartTimeNs = 10000000000;  // 0:10
+    uint64_t bucketSizeNs = TimeUnitToBucketSizeInMillis(TEN_MINUTES) * 1000000LL;
+    StatsdConfig config;
+
+    void SetUp() override {
+        UidData uidData =
+                createUidData({AID_LMKD, AID_APP_START + 1, AID_USER_OFFSET + AID_UWB,
+                               AID_USER_OFFSET + AID_APP_START + 2, AID_ROOT, AID_APP_START - 1,
+                               AID_APP_START} /* uids */,
+                              {1, 2, 3, 4, 5, 6, 7} /* versions */,
+                              {"v1", "v2", "v3", "v4", "v5", "v6", "v7"} /* versionStrings */,
+                              {"LMKD", "app1", "UWB", "app2", "root", "app3", "app4"} /* apps */,
+                              vector(7, string("installer")) /* installers */,
+                              vector(7, vector<uint8_t>{}) /* certificateHashes */);
+
+        uidMap->updateMap(/* timestamp */ 1, uidData);
+
+        uidMap->updateApp(/* timestamp */ 5, "LMKD", AID_LMKD, /* versionCode */ 10, "v10",
+                          /* installer */ "", /* certificateHash */ {});
+        uidMap->updateApp(/* timestamp */ 6, "UWB", AID_USER_OFFSET + AID_UWB, /* versionCode */ 20,
+                          "v20", /* installer */ "", /* certificateHash */ {});
+        uidMap->updateApp(/* timestamp */ 7, "root", AID_ROOT, /* versionCode */ 50, "v50",
+                          /* installer */ "", /* certificateHash */ {});
+        uidMap->updateApp(/* timestamp */ 8, "app3", AID_APP_START - 1, /* versionCode */ 60, "v60",
+                          /* installer */ "", /* certificateHash */ {});
+        uidMap->updateApp(/* timestamp */ 9, "app4", AID_APP_START, /* versionCode */ 70, "v70",
+                          /* installer */ "", /* certificateHash */ {});
+
+        *config.add_atom_matcher() =
+                CreateSimpleAtomMatcher("TestAtomMatcher", util::TEST_ATOM_REPORTED);
+        *config.add_event_metric() =
+                createEventMetric("TestAtomReported", config.atom_matcher(0).id(), nullopt);
+    }
+
+    inline sp<StatsLogProcessor> createStatsLogProcessor(const StatsdConfig& config) const {
+        return CreateStatsLogProcessor(bucketStartTimeNs, bucketStartTimeNs, config, cfgKey,
+                                       /* puller */ nullptr, /* puller atomTag */ 0, uidMap);
+    }
+
+    UidMapping getUidMapping(const sp<StatsLogProcessor>& processor) const {
+        vector<uint8_t> buffer;
+        processor->onDumpReport(cfgKey, bucketStartTimeNs + bucketSizeNs + 1, false, true, ADB_DUMP,
+                                FAST, &buffer);
+        ConfigMetricsReportList reports;
+        reports.ParseFromArray(&buffer[0], buffer.size());
+        return reports.reports(0).uid_map();
+    }
+};
+
 TEST_F(UidMapTestAppendUidMapSystemUids, testHasSystemUids) {
     sp<StatsLogProcessor> processor = createStatsLogProcessor(config);
     UidMapping results = getUidMapping(processor);
 
     ASSERT_EQ(results.snapshots_size(), 1);
     EXPECT_THAT(results.snapshots(0).package_info(),
-                Contains(Property(&PackageInfo::uid, AID_LMKD)));
-    EXPECT_THAT(results.snapshots(0).package_info(),
-                Contains(Property(&PackageInfo::uid, AID_USER_OFFSET + AID_UWB)));
-
-    EXPECT_THAT(results.changes(), Contains(Property(&Change::uid, AID_LMKD)));
-    EXPECT_THAT(results.changes(), Contains(Property(&Change::uid, AID_USER_OFFSET + AID_UWB)));
+                IsSupersetOf({
+                        Property(&PackageInfo::uid, AID_LMKD),
+                        Property(&PackageInfo::uid, AID_USER_OFFSET + AID_UWB),
+                        Property(&PackageInfo::uid, AID_ROOT),
+                        Property(&PackageInfo::uid, AID_APP_START - 1),
+                }));
+
+    EXPECT_THAT(results.changes(), IsSupersetOf({
+                                           Property(&Change::uid, AID_LMKD),
+                                           Property(&Change::uid, AID_USER_OFFSET + AID_UWB),
+                                           Property(&Change::uid, AID_ROOT),
+                                           Property(&Change::uid, AID_APP_START - 1),
+                                   }));
 }
 
 TEST_F(UidMapTestAppendUidMapSystemUids, testHasNoSystemUids) {
@@ -786,13 +802,14 @@ TEST_F(UidMapTestAppendUidMapSystemUids, testHasNoSystemUids) {
 
     ASSERT_EQ(results.snapshots_size(), 1);
     EXPECT_THAT(results.snapshots(0).package_info(),
-                Contains(Property(&PackageInfo::uid, AID_LMKD)).Times(0));
-    EXPECT_THAT(results.snapshots(0).package_info(),
-                Contains(Property(&PackageInfo::uid, AID_USER_OFFSET + AID_UWB)).Times(0));
+                Each(Property(&PackageInfo::uid,
+                              AllOf(Not(Eq(AID_LMKD)), Not(Eq(AID_USER_OFFSET + AID_UWB)),
+                                    Not(Eq(AID_ROOT)), Not(Eq(AID_APP_START - 1))))));
 
-    EXPECT_THAT(results.changes(), IsEmpty());
+    EXPECT_THAT(results.changes(), ElementsAre(Property(&Change::uid, AID_APP_START)));
 }
 
+}  // anonymous namespace
 #else
 GTEST_LOG_(INFO) << "This test does nothing.\n";
 #endif
diff --git a/statsd/tests/statsd_test_util.cpp b/statsd/tests/statsd_test_util.cpp
index 71f5cd1e..54e803c5 100644
--- a/statsd/tests/statsd_test_util.cpp
+++ b/statsd/tests/statsd_test_util.cpp
@@ -2128,6 +2128,7 @@ PackageInfoSnapshot getPackageInfoSnapshot(const sp<UidMap> uidMap) {
     ProtoOutputStream protoOutputStream;
     uidMap->writeUidMapSnapshot(/* timestamp */ 1, /* includeVersionStrings */ true,
                                 /* includeInstaller */ true, /* certificateHashSize */ UINT8_MAX,
+                                /* omitSystemUids */ false,
                                 /* interestingUids */ {},
                                 /* installerIndices */ nullptr, /* str_set */ nullptr,
                                 &protoOutputStream);