From 98a28501fe8ab53a490ec353c8a1f74f2e329cc5 Mon Sep 17 00:00:00 2001 From: yro Date: Thu, 18 Jan 2018 17:00:14 -0800 Subject: [PATCH] Adding guardrails on writing to disk from statsd - Limit total number of files to 1000 - Limit total size of files to 5MB - Remove idle files to be deleted after 30 days Bug: 69854160 Test: manual testing, statsd, statsd_test Change-Id: I33148a3b7ca11d413ec2495d5c0659f1ba4485c3 --- cmds/statsd/Android.mk | 2 +- cmds/statsd/src/StatsLogProcessor.cpp | 6 +- cmds/statsd/src/config/ConfigManager.cpp | 4 +- cmds/statsd/src/guardrail/StatsdStats.h | 9 + cmds/statsd/src/storage/StorageManager.cpp | 183 ++++++++++++++------- cmds/statsd/src/storage/StorageManager.h | 8 +- 6 files changed, 145 insertions(+), 67 deletions(-) diff --git a/cmds/statsd/Android.mk b/cmds/statsd/Android.mk index 5eff54887be12..9bddbcbe405b0 100644 --- a/cmds/statsd/Android.mk +++ b/cmds/statsd/Android.mk @@ -134,7 +134,7 @@ LOCAL_SHARED_LIBRARIES := $(statsd_common_shared_libraries) \ LOCAL_MODULE_CLASS := EXECUTABLES -#LOCAL_INIT_RC := statsd.rc +LOCAL_INIT_RC := statsd.rc include $(BUILD_EXECUTABLE) diff --git a/cmds/statsd/src/StatsLogProcessor.cpp b/cmds/statsd/src/StatsLogProcessor.cpp index 9d6d8a1f2b732..e0187b72456d8 100644 --- a/cmds/statsd/src/StatsLogProcessor.cpp +++ b/cmds/statsd/src/StatsLogProcessor.cpp @@ -236,7 +236,7 @@ void StatsLogProcessor::onDumpReport(const ConfigKey& key, vector* outD // Then, check stats-data directory to see there's any file containing // ConfigMetricsReport from previous shutdowns to concatenate to reports. - StorageManager::appendConfigMetricsReport(STATS_DATA_DIR, proto); + StorageManager::appendConfigMetricsReport(proto); if (outData != nullptr) { outData->clear(); @@ -307,8 +307,8 @@ void StatsLogProcessor::WriteDataToDisk() { vector data; onDumpReport(key, &data); // TODO: Add a guardrail to prevent accumulation of file on disk. - string file_name = StringPrintf("%s/%d-%lld-%ld", STATS_DATA_DIR, key.GetUid(), - (long long)key.GetId(), time(nullptr)); + string file_name = StringPrintf("%s/%ld_%d_%lld", STATS_DATA_DIR, time(nullptr), + key.GetUid(), (long long)key.GetId()); StorageManager::writeFile(file_name.c_str(), &data[0], data.size()); } } diff --git a/cmds/statsd/src/config/ConfigManager.cpp b/cmds/statsd/src/config/ConfigManager.cpp index 42994b558208b..15af7c5d54305 100644 --- a/cmds/statsd/src/config/ConfigManager.cpp +++ b/cmds/statsd/src/config/ConfigManager.cpp @@ -185,8 +185,8 @@ void ConfigManager::update_saved_configs(const ConfigKey& key, const StatsdConfi remove_saved_configs(key); // Then we save the latest config. - string file_name = StringPrintf("%s/%d-%lld-%ld", STATS_SERVICE_DIR, key.GetUid(), - (long long)key.GetId(), time(nullptr)); + string file_name = StringPrintf("%s/%ld_%d_%lld", STATS_SERVICE_DIR, time(nullptr), + key.GetUid(), (long long)key.GetId()); const int numBytes = config.ByteSize(); vector buffer(numBytes); config.SerializeToArray(&buffer[0], numBytes); diff --git a/cmds/statsd/src/guardrail/StatsdStats.h b/cmds/statsd/src/guardrail/StatsdStats.h index 9178daa4fe12a..7cb48ead55d8c 100644 --- a/cmds/statsd/src/guardrail/StatsdStats.h +++ b/cmds/statsd/src/guardrail/StatsdStats.h @@ -74,6 +74,15 @@ public: // Default cooldown time for a puller static const long kDefaultPullerCooldown = 1; + // Maximum age (30 days) that files on disk can exist in seconds. + static const int kMaxAgeSecond = 60 * 60 * 24 * 30; + + // Maximum number of files (1000) that can be in stats directory on disk. + static const int kMaxFileNumber = 1000; + + // Maximum size of all files that can be written to stats directory on disk. + static const int kMaxFileSize = 50 * 1024 * 1024; + /** * Report a new config has been received and report the static stats about the config. * diff --git a/cmds/statsd/src/storage/StorageManager.cpp b/cmds/statsd/src/storage/StorageManager.cpp index 1d75e20d7eaa5..f5c79cb266972 100644 --- a/cmds/statsd/src/storage/StorageManager.cpp +++ b/cmds/statsd/src/storage/StorageManager.cpp @@ -17,11 +17,15 @@ #define DEBUG true // STOPSHIP if true #include "Log.h" -#include "storage/StorageManager.h" #include "android-base/stringprintf.h" +#include "guardrail/StatsdStats.h" +#include "storage/StorageManager.h" #include #include +#include +#include +#include namespace android { namespace os { @@ -31,6 +35,7 @@ using android::util::FIELD_COUNT_REPEATED; using android::util::FIELD_TYPE_MESSAGE; using std::map; +#define STATS_DATA_DIR "/data/misc/stats-data" #define STATS_SERVICE_DIR "/data/misc/stats-service" // for ConfigMetricsReportList @@ -39,12 +44,37 @@ const int FIELD_ID_REPORTS = 2; using android::base::StringPrintf; using std::unique_ptr; +// Returns array of int64_t which contains timestamp in seconds, uid, and +// configID. +static void parseFileName(char* name, int64_t* result) { + int index = 0; + char* substr = strtok(name, "_"); + while (substr != nullptr && index < 3) { + result[index] = StrToInt64(substr); + index++; + substr = strtok(nullptr, "_"); + } + // When index ends before hitting 3, file name is corrupted. We + // intentionally put -1 at index 0 to indicate the error to caller. + // TODO: consider removing files with unexpected name format. + if (index < 3) { + result[0] = -1; + } +} + +static string getFilePath(const char* path, int64_t timestamp, int64_t uid, int64_t configID) { + return StringPrintf("%s/%lld-%d-%lld", path, (long long)timestamp, (int)uid, + (long long)configID); +} + void StorageManager::writeFile(const char* file, const void* buffer, int numBytes) { int fd = open(file, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); if (fd == -1) { VLOG("Attempt to access %s but failed", file); return; } + trimToFit(STATS_SERVICE_DIR); + trimToFit(STATS_DATA_DIR); int result = write(fd, buffer, numBytes); if (result == numBytes) { @@ -52,6 +82,12 @@ void StorageManager::writeFile(const char* file, const void* buffer, int numByte } else { VLOG("Failed to write %s", file); } + + result = fchown(fd, AID_STATSD, AID_STATSD); + if (result) { + VLOG("Failed to chown %s to statsd", file); + } + close(fd); } @@ -109,30 +145,20 @@ void StorageManager::sendBroadcast(const char* path, if (name[0] == '.') continue; VLOG("file %s", name); - int index = 0; - int uid = 0; - int64_t configID = 0; - char* substr = strtok(name, "-"); - // Timestamp lives at index 2 but we skip parsing it as it's not needed. - while (substr != nullptr && index < 2) { - if (index == 0) { - uid = atoi(substr); - } else if (index == 1) { - configID = StrToInt64(substr); - } - index++; - substr = strtok(nullptr, "-"); - } - if (index < 2) continue; + int64_t result[3]; + parseFileName(name, result); + if (result[0] == -1) continue; + int64_t uid = result[1]; + int64_t configID = result[2]; - sendBroadcast(ConfigKey(uid, configID)); + sendBroadcast(ConfigKey((int)uid, configID)); } } -void StorageManager::appendConfigMetricsReport(const char* path, ProtoOutputStream& proto) { - unique_ptr dir(opendir(path), closedir); +void StorageManager::appendConfigMetricsReport(ProtoOutputStream& proto) { + unique_ptr dir(opendir(STATS_DATA_DIR), closedir); if (dir == NULL) { - VLOG("Path %s does not exist", path); + VLOG("Path %s does not exist", STATS_DATA_DIR); return; } @@ -142,25 +168,13 @@ void StorageManager::appendConfigMetricsReport(const char* path, ProtoOutputStre if (name[0] == '.') continue; VLOG("file %s", name); - int index = 0; - int uid = 0; - int64_t configID = 0; - int64_t timestamp = 0; - char* substr = strtok(name, "-"); - while (substr != nullptr && index < 3) { - if (index == 0) { - uid = atoi(substr); - } else if (index == 1) { - configID = StrToInt64(substr); - } else if (index == 2) { - timestamp = atoi(substr); - } - index++; - substr = strtok(nullptr, "-"); - } - if (index < 3) continue; - string file_name = StringPrintf("%s/%d-%lld-%lld", STATS_SERVICE_DIR, uid, - (long long)configID, (long long)timestamp); + int64_t result[3]; + parseFileName(name, result); + if (result[0] == -1) continue; + int64_t timestamp = result[0]; + int64_t uid = result[1]; + int64_t configID = result[2]; + string file_name = getFilePath(STATS_DATA_DIR, timestamp, uid, configID); int fd = open(file_name.c_str(), O_RDONLY | O_CLOEXEC); if (fd != -1) { string content; @@ -182,6 +196,7 @@ void StorageManager::readConfigFromDisk(map& configsMap VLOG("no default config on disk"); return; } + trimToFit(STATS_SERVICE_DIR); dirent* de; while ((de = readdir(dir.get()))) { @@ -189,26 +204,13 @@ void StorageManager::readConfigFromDisk(map& configsMap if (name[0] == '.') continue; VLOG("file %s", name); - int index = 0; - int uid = 0; - int64_t configID = 0; - int64_t timestamp = 0; - char* substr = strtok(name, "-"); - while (substr != nullptr && index < 3) { - if (index == 0) { - uid = atoi(substr); - } else if (index == 1) { - configID = StrToInt64(substr); - } else if (index == 2) { - timestamp = atoi(substr); - } - index++; - substr = strtok(nullptr, "-"); - } - if (index < 3) continue; - - string file_name = StringPrintf("%s/%d-%lld-%lld", STATS_SERVICE_DIR, uid, - (long long)configID, (long long)timestamp); + int64_t result[3]; + parseFileName(name, result); + if (result[0] == -1) continue; + int64_t timestamp = result[0]; + int64_t uid = result[1]; + int64_t configID = result[2]; + string file_name = getFilePath(STATS_SERVICE_DIR, timestamp, uid, configID); int fd = open(file_name.c_str(), O_RDONLY | O_CLOEXEC); if (fd != -1) { string content; @@ -216,7 +218,7 @@ void StorageManager::readConfigFromDisk(map& configsMap StatsdConfig config; if (config.ParseFromString(content)) { configsMap[ConfigKey(uid, configID)] = config; - VLOG("map key uid=%d|configID=%lld", uid, (long long)configID); + VLOG("map key uid=%lld|configID=%lld", (long long)uid, (long long)configID); } } close(fd); @@ -224,6 +226,67 @@ void StorageManager::readConfigFromDisk(map& configsMap } } +void StorageManager::trimToFit(const char* path) { + unique_ptr dir(opendir(path), closedir); + if (dir == NULL) { + VLOG("Path %s does not exist", path); + return; + } + dirent* de; + int totalFileSize = 0; + vector fileNames; + while ((de = readdir(dir.get()))) { + char* name = de->d_name; + if (name[0] == '.') continue; + + int64_t result[3]; + parseFileName(name, result); + if (result[0] == -1) continue; + int64_t timestamp = result[0]; + int64_t uid = result[1]; + int64_t configID = result[2]; + string file_name = getFilePath(path, timestamp, uid, configID); + + // Check for timestamp and delete if it's too old. + long fileAge = time(nullptr) - timestamp; + if (fileAge > StatsdStats::kMaxAgeSecond) { + deleteFile(file_name.c_str()); + } + + fileNames.push_back(file_name); + ifstream file(file_name.c_str(), ifstream::in | ifstream::binary); + if (file.is_open()) { + file.seekg(0, ios::end); + int fileSize = file.tellg(); + file.close(); + totalFileSize += fileSize; + } + } + + if (fileNames.size() > StatsdStats::kMaxFileNumber || + totalFileSize > StatsdStats::kMaxFileSize) { + // Reverse sort to effectively remove from the back (oldest entries). + // This will sort files in reverse-chronological order. + sort(fileNames.begin(), fileNames.end(), std::greater()); + } + + // Start removing files from oldest to be under the limit. + while (fileNames.size() > 0 && (fileNames.size() > StatsdStats::kMaxFileNumber || + totalFileSize > StatsdStats::kMaxFileSize)) { + string file_name = fileNames.at(fileNames.size() - 1); + ifstream file(file_name.c_str(), ifstream::in | ifstream::binary); + if (file.is_open()) { + file.seekg(0, ios::end); + int fileSize = file.tellg(); + file.close(); + totalFileSize -= fileSize; + } + + deleteFile(file_name.c_str()); + fileNames.pop_back(); + } +} + } // namespace statsd } // namespace os } // namespace android diff --git a/cmds/statsd/src/storage/StorageManager.h b/cmds/statsd/src/storage/StorageManager.h index caf5b8b42887d..b05cbc64968e6 100644 --- a/cmds/statsd/src/storage/StorageManager.h +++ b/cmds/statsd/src/storage/StorageManager.h @@ -61,12 +61,18 @@ public: * Appends ConfigMetricsReport found on disk to the specific proto and * delete it. */ - static void appendConfigMetricsReport(const char* path, ProtoOutputStream& proto); + static void appendConfigMetricsReport(ProtoOutputStream& proto); /** * Call to load the saved configs from disk. */ static void readConfigFromDisk(std::map& configsMap); + + /** + * Trims files in the provided directory to limit the total size, number of + * files, accumulation of outdated files. + */ + static void trimToFit(const char* dir); }; } // namespace statsd