Fix uidmap in statsd.
Previously tried an optimization that results in corrupted proto
output. This changes to a safer approach of storing the snapshot data
in memory and only converting to proto output when the
ProtoOutputStream is provided.
Also fixes a security issue when trying to invoke triggerUidSnapshot
since we forgot to use SCS' permissions.
Test: Added a unit-test to verify output of StatsLogProcessor.
Bug: 76231867
Change-Id: Id410ce3505fda9d71caa71942ef3068b55872c66
diff --git a/cmds/statsd/src/packages/UidMap.cpp b/cmds/statsd/src/packages/UidMap.cpp
index 1cb20bc..8c8152d 100644
--- a/cmds/statsd/src/packages/UidMap.cpp
+++ b/cmds/statsd/src/packages/UidMap.cpp
@@ -116,32 +116,16 @@
lock_guard<mutex> lock(mMutex); // Exclusively lock for updates.
mMap.clear();
- ProtoOutputStream proto;
- uint64_t token = proto.start(FIELD_TYPE_MESSAGE | FIELD_COUNT_REPEATED |
- FIELD_ID_SNAPSHOT_PACKAGE_INFO);
+ vector<const SnapshotPackageInfo> infos;
for (size_t j = 0; j < uid.size(); j++) {
string package = string(String8(packageName[j]).string());
mMap.insert(make_pair(uid[j], AppData(package, versionCode[j])));
- proto.write(FIELD_TYPE_STRING | FIELD_ID_SNAPSHOT_PACKAGE_NAME, package);
- proto.write(FIELD_TYPE_INT32 | FIELD_ID_SNAPSHOT_PACKAGE_VERSION, (int)versionCode[j]);
- proto.write(FIELD_TYPE_INT32 | FIELD_ID_SNAPSHOT_PACKAGE_UID, (int)uid[j]);
+ infos.emplace_back(package, versionCode[j], uid[j]);
}
- proto.end(token);
- // Copy ProtoOutputStream output to
- auto iter = proto.data();
- size_t pos = 0;
- vector<char> outData(proto.size());
- while (iter.readBuffer() != NULL) {
- size_t toRead = iter.currentToRead();
- std::memcpy(&(outData[pos]), iter.readBuffer(), toRead);
- pos += toRead;
- iter.rp()->move(toRead);
- }
- SnapshotRecord record(timestamp, outData);
- mSnapshots.push_back(record);
+ mSnapshots.emplace_back(timestamp, infos);
- mBytesUsed += proto.size() + kBytesTimestampField;
+ mBytesUsed += mSnapshots.back().bytes;
ensureBytesUsedBelowLimit();
StatsdStats::getInstance().setCurrentUidMapMemory(mBytesUsed);
StatsdStats::getInstance().setUidMapSnapshots(mSnapshots.size());
@@ -212,7 +196,7 @@
while (mBytesUsed > limit) {
ALOGI("Bytes used %zu is above limit %zu, need to delete something", mBytesUsed, limit);
if (mSnapshots.size() > 0) {
- mBytesUsed -= mSnapshots.front().bytes.size() + kBytesTimestampField;
+ mBytesUsed -= mSnapshots.front().bytes;
mSnapshots.pop_front();
StatsdStats::getInstance().noteUidMapDropped(1, 0);
} else if (mChanges.size() > 0) {
@@ -365,8 +349,14 @@
count++;
proto->write(FIELD_TYPE_INT64 | FIELD_ID_SNAPSHOT_TIMESTAMP,
(long long)record.timestampNs);
- proto->write(FIELD_TYPE_MESSAGE | FIELD_ID_SNAPSHOT_PACKAGE_INFO, record.bytes.data(),
- record.bytes.size());
+ for (const SnapshotPackageInfo& info : record.infos) {
+ uint64_t token = proto->start(FIELD_TYPE_MESSAGE | FIELD_COUNT_REPEATED |
+ FIELD_ID_SNAPSHOT_PACKAGE_INFO);
+ proto->write(FIELD_TYPE_STRING | FIELD_ID_SNAPSHOT_PACKAGE_NAME, info.package);
+ proto->write(FIELD_TYPE_INT32 | FIELD_ID_SNAPSHOT_PACKAGE_VERSION, info.version);
+ proto->write(FIELD_TYPE_INT32 | FIELD_ID_SNAPSHOT_PACKAGE_UID, info.uid);
+ proto->end(token);
+ }
proto->end(snapshotsToken);
}
}
@@ -380,7 +370,7 @@
int64_t cutoff_nanos = newMin;
for (auto it_snapshots = mSnapshots.begin(); it_snapshots != mSnapshots.end();) {
if (it_snapshots->timestampNs < cutoff_nanos) {
- mBytesUsed -= it_snapshots->bytes.size() + kBytesTimestampField;
+ mBytesUsed -= it_snapshots->bytes;
it_snapshots = mSnapshots.erase(it_snapshots);
} else {
++it_snapshots;
@@ -399,31 +389,13 @@
// Produce another snapshot. This results in extra data being uploaded but
// helps ensure we can re-construct the UID->app name, versionCode mapping
// in server.
- ProtoOutputStream snapshotProto;
- uint64_t token = snapshotProto.start(FIELD_TYPE_MESSAGE | FIELD_COUNT_REPEATED |
- FIELD_ID_SNAPSHOT_PACKAGE_INFO);
+ vector<const SnapshotPackageInfo> infos;
for (const auto& it : mMap) {
- snapshotProto.write(FIELD_TYPE_STRING | FIELD_ID_SNAPSHOT_PACKAGE_NAME,
- it.second.packageName);
- snapshotProto.write(FIELD_TYPE_INT32 | FIELD_ID_SNAPSHOT_PACKAGE_VERSION,
- (int)it.second.versionCode);
- snapshotProto.write(FIELD_TYPE_INT32 | FIELD_ID_SNAPSHOT_PACKAGE_UID,
- (int)it.first);
+ infos.emplace_back(it.second.packageName, it.second.versionCode, it.first);
}
- snapshotProto.end(token);
- // Copy ProtoOutputStream output to
- auto iter = snapshotProto.data();
- vector<char> snapshotData(snapshotProto.size());
- size_t pos = 0;
- while (iter.readBuffer() != NULL) {
- size_t toRead = iter.currentToRead();
- std::memcpy(&(snapshotData[pos]), iter.readBuffer(), toRead);
- pos += toRead;
- iter.rp()->move(toRead);
- }
- mSnapshots.emplace_back(timestamp, snapshotData);
- mBytesUsed += kBytesTimestampField + snapshotData.size();
+ mSnapshots.emplace_back(timestamp, infos);
+ mBytesUsed += mSnapshots.back().bytes;
}
}
StatsdStats::getInstance().setCurrentUidMapMemory(mBytesUsed);
diff --git a/cmds/statsd/src/packages/UidMap.h b/cmds/statsd/src/packages/UidMap.h
index 9dc73f4..a3632d2 100644
--- a/cmds/statsd/src/packages/UidMap.h
+++ b/cmds/statsd/src/packages/UidMap.h
@@ -74,17 +74,35 @@
// less because of varint encoding).
const unsigned int kBytesTimestampField = 10;
+struct SnapshotPackageInfo {
+ const string package;
+ const int32_t version;
+ const int32_t uid;
+ SnapshotPackageInfo(const string& package, const int32_t version, const int32_t uid)
+ : package(package), version(version), uid(uid) {
+ }
+};
+
+const unsigned int kBytesSnapshotInfo = sizeof(struct SnapshotPackageInfo);
+
// When calling appendUidMap, we retrieve all the snapshots since the last
// timestamp we called appendUidMap for this configuration key.
struct SnapshotRecord {
const int64_t timestampNs;
- // For performance reasons, we convert the package_info field (which is a
- // repeated field of PackageInfo messages).
- vector<char> bytes;
+ // All the package info known.
+ vector<const SnapshotPackageInfo> infos;
- SnapshotRecord(const int64_t timestampNs, vector<char> bytes)
- : timestampNs(timestampNs), bytes(bytes) {
+ // Tracks the number of bytes this snapshot consumes.
+ uint32_t bytes;
+
+ SnapshotRecord(const int64_t timestampNs, vector<const SnapshotPackageInfo>& infos)
+ : timestampNs(timestampNs), infos(infos) {
+ bytes = 0;
+ for (auto info : infos) {
+ bytes += info.package.size() + kBytesSnapshotInfo;
+ }
+ bytes += kBytesTimestampField;
}
};
@@ -210,6 +228,7 @@
// Allows unit-test to access private methods.
FRIEND_TEST(UidMapTest, TestClearingOutput);
+ FRIEND_TEST(UidMapTest, TestOutputIncludesAtLeastOneSnapshot);
FRIEND_TEST(UidMapTest, TestMemoryComputed);
FRIEND_TEST(UidMapTest, TestMemoryGuardrail);
};
diff --git a/cmds/statsd/tests/StatsLogProcessor_test.cpp b/cmds/statsd/tests/StatsLogProcessor_test.cpp
index efed42e..4b9a87d 100644
--- a/cmds/statsd/tests/StatsLogProcessor_test.cpp
+++ b/cmds/statsd/tests/StatsLogProcessor_test.cpp
@@ -14,6 +14,7 @@
#include "StatsLogProcessor.h"
#include "config/ConfigKey.h"
+#include "frameworks/base/cmds/statsd/src/stats_log.pb.h"
#include "frameworks/base/cmds/statsd/src/statsd_config.pb.h"
#include "guardrail/StatsdStats.h"
#include "logd/LogEvent.h"
@@ -122,6 +123,32 @@
EXPECT_EQ(0, broadcastCount);
}
+TEST(StatsLogProcessorTest, TestUidMapHasSnapshot) {
+ // Setup simple config key corresponding to empty config.
+ sp<UidMap> m = new UidMap();
+ m->updateMap({1, 2}, {1, 2}, {String16("p1"), String16("p2")});
+ sp<AlarmMonitor> anomalyAlarmMonitor;
+ sp<AlarmMonitor> subscriberAlarmMonitor;
+ int broadcastCount = 0;
+ StatsLogProcessor p(m, anomalyAlarmMonitor, subscriberAlarmMonitor, 0,
+ [&broadcastCount](const ConfigKey& key) { broadcastCount++; });
+ ConfigKey key(3, 4);
+ StatsdConfig config;
+ config.add_allowed_log_source("AID_ROOT");
+ p.OnConfigUpdated(key, config);
+
+ // Expect to get no metrics, but snapshot specified above in uidmap.
+ vector<uint8_t> bytes;
+ p.onDumpReport(key, 1, &bytes);
+
+ ConfigMetricsReportList output;
+ output.ParseFromArray(bytes.data(), bytes.size());
+ EXPECT_TRUE(output.reports_size() > 0);
+ auto uidmap = output.reports(0).uid_map();
+ EXPECT_TRUE(uidmap.snapshots_size() > 0);
+ EXPECT_EQ(2, uidmap.snapshots(0).package_info_size());
+}
+
#else
GTEST_LOG_(INFO) << "This test does nothing.\n";
#endif
diff --git a/cmds/statsd/tests/UidMap_test.cpp b/cmds/statsd/tests/UidMap_test.cpp
index c9492eb..a9b67e0 100644
--- a/cmds/statsd/tests/UidMap_test.cpp
+++ b/cmds/statsd/tests/UidMap_test.cpp
@@ -173,6 +173,33 @@
results->ParseFromArray(bytes.data(), bytes.size());
}
+// Test that uid map returns at least one snapshot even if we already obtained
+// this snapshot from a previous call to getData.
+TEST(UidMapTest, TestOutputIncludesAtLeastOneSnapshot) {
+ UidMap m;
+ // Initialize single config key.
+ ConfigKey config1(1, StringToId("config1"));
+ m.OnConfigUpdated(config1);
+ vector<int32_t> uids;
+ vector<int64_t> versions;
+ vector<String16> apps;
+ uids.push_back(1000);
+ apps.push_back(String16(kApp2.c_str()));
+ versions.push_back(5);
+ m.updateMap(1, uids, versions, apps);
+
+ // Set the last timestamp for this config key to be newer.
+ m.mLastUpdatePerConfigKey[config1] = 2;
+
+ ProtoOutputStream proto;
+ m.appendUidMap(3, config1, &proto);
+
+ // Check there's still a uidmap attached this one.
+ UidMapping results;
+ protoOutputStreamToUidMapping(&proto, &results);
+ EXPECT_EQ(1, results.snapshots_size());
+}
+
TEST(UidMapTest, TestClearingOutput) {
UidMap m;
@@ -199,7 +226,7 @@
protoOutputStreamToUidMapping(&proto, &results);
EXPECT_EQ(1, results.snapshots_size());
- // It should be cleared now
+ // We have to keep at least one snapshot in memory at all times.
EXPECT_EQ(1U, m.mSnapshots.size());
proto.clear();
m.appendUidMap(2, config1, &proto);
diff --git a/services/core/java/com/android/server/stats/StatsCompanionService.java b/services/core/java/com/android/server/stats/StatsCompanionService.java
index 74c5ee9..b3d28fc 100644
--- a/services/core/java/com/android/server/stats/StatsCompanionService.java
+++ b/services/core/java/com/android/server/stats/StatsCompanionService.java
@@ -972,10 +972,13 @@
public void triggerUidSnapshot() {
enforceCallingPermission();
synchronized (sStatsdLock) {
+ final long token = Binder.clearCallingIdentity();
try {
informAllUidsLocked(mContext);
} catch (RemoteException e) {
Slog.e(TAG, "Failed to trigger uid snapshot.", e);
+ } finally {
+ restoreCallingIdentity(token);
}
}
}