commit d4b1ec0b4215ea4362072f2d9dbd5aaad70c7064
author Connor O'Brien <connoro@google.com> Tue Apr 26 18:50:26 2022 -0700
committer Connor O'Brien <connoro@google.com> Tue Apr 26 19:01:46 2022 -0700
tree eb318f11195416a430a98a829c2a2e9d1fae5c88
parent 171a64a823952b0ab210688a1d342e9882078400

libtimeinstate: fix bug when cpu count exceeds CPUS_PER_ENTRY

In getUidCpuFreqTimes() and getUidConcurrentTimes(), in the case where
findMapEntry fails but getFirstMapKey succeeds we overwrite key and
then continue to the next loop iteration. This can cause problems when
gNCpus > CPUS_PER_ENTRY causing us to use multiple buckets: in both
functions, subsequent loop iterations could use the wrong UID, and in
getUidConcurrentTimes() we can enter an infinite loop if
getFirstMapKey() overwrites key.bucket with a lower value.

Instead, use a separate time_key_t variable for the call to
getFirstMapKey(). To prevent similar bugs, make key a const variable
scoped to a single loop iteration.

Bug: 229788490
Test: libtimeinstate_test
Signed-off-by: Connor O'Brien <connoro@google.com>
Change-Id: I491aa3bf4c5bce9218bdc44a5eee6ea426b151ae

libs/cputimeinstate/cputimeinstate.cpp

diff --git a/libs/cputimeinstate/cputimeinstate.cpp b/libs/cputimeinstate/cputimeinstate.cpp
index 7e9bb7d..d169043 100644
--- a/libs/cputimeinstate/cputimeinstate.cpp
+++ b/libs/cputimeinstate/cputimeinstate.cpp
@@ -300,11 +300,11 @@
     }
 
     std::vector<tis_val_t> vals(gNCpus);
-    time_key_t key = {.uid = uid};
     for (uint32_t i = 0; i <= (maxFreqCount - 1) / FREQS_PER_ENTRY; ++i) {
-        key.bucket = i;
+        const time_key_t key = {.uid = uid, .bucket = i};
         if (findMapEntry(gTisMapFd, &key, vals.data())) {
-            if (errno != ENOENT || getFirstMapKey(gTisMapFd, &key)) return {};
+            time_key_t tmpKey;
+            if (errno != ENOENT || getFirstMapKey(gTisMapFd, &tmpKey)) return {};
             continue;
         }
 
@@ -412,10 +412,11 @@
     concurrent_time_t ret = {.active = std::vector<uint64_t>(gNCpus, 0)};
     for (const auto &cpuList : gPolicyCpus) ret.policy.emplace_back(cpuList.size(), 0);
     std::vector<concurrent_val_t> vals(gNCpus);
-    time_key_t key = {.uid = uid};
-    for (key.bucket = 0; key.bucket <= (gNCpus - 1) / CPUS_PER_ENTRY; ++key.bucket) {
+    for (uint32_t i = 0; i <= (gNCpus - 1) / CPUS_PER_ENTRY; ++i) {
+        const time_key_t key = {.uid = uid, .bucket = i};
         if (findMapEntry(gConcurrentMapFd, &key, vals.data())) {
-            if (errno != ENOENT || getFirstMapKey(gConcurrentMapFd, &key)) return {};
+            time_key_t tmpKey;
+            if (errno != ENOENT || getFirstMapKey(gConcurrentMapFd, &tmpKey)) return {};
             continue;
         }
         auto offset = key.bucket * CPUS_PER_ENTRY;