From 2fb5653b9761dba5fa29d9abd84e938e59932075 Mon Sep 17 00:00:00 2001
From: Bookatz <bookatz@google.com>
Date: Thu, 8 Mar 2018 11:16:48 -0800
Subject: [PATCH] Fix statsd crash due to bad bucket index

Statsd crashes because predictAnomalyTimestamp requests past buckets
that occurred before time began. That's fine, but statsd needs to know
that the data before time began was 0 (instead of reading before the
beginning of the array and crashing).

AnomalyTracker's use of bucketNumbers is in general risky, and should be
auditted. But this cl will fix the current crashing.

Bug: 73825954
Test: Definitely necessary. Will write during audit.
Change-Id: I990ff134153f290d3089bfe3440d838f47996b63
---
 cmds/statsd/src/anomaly/AnomalyTracker.cpp | 12 ++++++++----
 cmds/statsd/src/anomaly/AnomalyTracker.h   |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/cmds/statsd/src/anomaly/AnomalyTracker.cpp b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
index 6ee51f38dde87..133f33b15405e 100644
--- a/cmds/statsd/src/anomaly/AnomalyTracker.cpp
+++ b/cmds/statsd/src/anomaly/AnomalyTracker.cpp
@@ -59,6 +59,12 @@ void AnomalyTracker::resetStorage() {
 }
 
 size_t AnomalyTracker::index(int64_t bucketNum) const {
+    if (bucketNum < 0) {
+        // To support this use-case, we can easily modify index to wrap around. But currently
+        // AnomalyTracker should never need this, so if it happens, it's a bug we should log.
+        // TODO: Audit this.
+        ALOGE("index() was passed a negative bucket number (%lld)!", (long long)bucketNum);
+    }
     return bucketNum % mNumOfPastBuckets;
 }
 
@@ -72,9 +78,7 @@ void AnomalyTracker::flushPastBuckets(const int64_t& latestPastBucketNum) {
     // The past packets are ancient. Empty out old mPastBuckets[i] values and reset
     // mSumOverPastBuckets.
     if (latestPastBucketNum - mMostRecentBucketNum >= mNumOfPastBuckets) {
-        mPastBuckets.clear();
-        mPastBuckets.resize(mNumOfPastBuckets);
-        mSumOverPastBuckets.clear();
+        resetStorage();
     } else {
         for (int64_t i = std::max(0LL, (long long)(mMostRecentBucketNum - mNumOfPastBuckets + 1));
              i <= latestPastBucketNum - mNumOfPastBuckets; i++) {
@@ -150,7 +154,7 @@ void AnomalyTracker::addBucketToSum(const shared_ptr<DimToValMap>& bucket) {
 
 int64_t AnomalyTracker::getPastBucketValue(const MetricDimensionKey& key,
                                            const int64_t& bucketNum) const {
-    if (mNumOfPastBuckets == 0) {
+    if (mNumOfPastBuckets == 0 || bucketNum < 0) {
         return 0;
     }
 
diff --git a/cmds/statsd/src/anomaly/AnomalyTracker.h b/cmds/statsd/src/anomaly/AnomalyTracker.h
index e3f493cfe7cf0..d27dee843b22a 100644
--- a/cmds/statsd/src/anomaly/AnomalyTracker.h
+++ b/cmds/statsd/src/anomaly/AnomalyTracker.h
@@ -110,7 +110,7 @@ protected:
 
     // Number of past buckets. One less than the total number of buckets needed
     // for the anomaly detection (since the current bucket is not in the past).
-    int mNumOfPastBuckets;
+    const int mNumOfPastBuckets;
 
     // The existing bucket list.
     std::vector<shared_ptr<DimToValMap>> mPastBuckets;