wifi_scan_cache: Correctly calculate ageMs
Prior code could produce very large ageMs values due to improper early
truncation of the 64-bit nanosecond timestamp. The updated version still
avoids 64-bit integer division, but produces correct results and also
guards against providing unreasonably large values even if the provided
time is invalid.
Bug: 382566782
Test: atest chre_unit_tests before & after patch
Change-Id: I8811255906c2bc97dab11fc75d3cecae54e44aad
diff --git a/pal/util/tests/wifi_scan_cache_test.cc b/pal/util/tests/wifi_scan_cache_test.cc
index 5c0dcec..ab55305 100644
--- a/pal/util/tests/wifi_scan_cache_test.cc
+++ b/pal/util/tests/wifi_scan_cache_test.cc
@@ -20,15 +20,23 @@
#include <cinttypes>
#include <cstring>
+#include "chre/platform/linux/system_time.h"
#include "chre/platform/log.h"
#include "chre/platform/shared/pal_system_api.h"
#include "chre/util/fixed_size_vector.h"
#include "chre/util/macros.h"
#include "chre/util/nanoapp/wifi.h"
#include "chre/util/optional.h"
+#include "chre/util/time.h"
#include "chre_api/chre/common.h"
#include "gtest/gtest.h"
+using chre::Milliseconds;
+using chre::Nanoseconds;
+using chre::Seconds;
+using chre::platform_linux::clearMonotonicTimeOverride;
+using chre::platform_linux::overrideMonotonicTime;
+
namespace {
/************************************************
@@ -72,6 +80,7 @@
void TearDown() override {
chreWifiScanCacheDeinit();
+ clearMonotonicTimeOverride();
}
void clearTestState() {
@@ -489,3 +498,56 @@
EXPECT_EQ(gWifiScanResponse->errorCode, CHRE_ERROR_NONE);
EXPECT_EQ(gWifiScanResultList.size(), 2);
}
+
+TEST_F(WifiScanCacheTests, AgeCalculatedCorrectly) {
+ constexpr auto kStartTime = Seconds(4);
+ overrideMonotonicTime(kStartTime);
+ beginDefaultWifiCache(nullptr /* scannedFreqList */,
+ 0 /* scannedFreqListLen */);
+
+ overrideMonotonicTime(kStartTime + Milliseconds(100));
+ chreWifiScanResult result = {};
+ chreWifiScanCacheScanEventAdd(&result);
+
+ overrideMonotonicTime(kStartTime + Milliseconds(500));
+ chreWifiScanCacheScanEventEnd(CHRE_ERROR_NONE);
+
+ ASSERT_EQ(gWifiScanResultList.size(), 1);
+ EXPECT_EQ(gWifiScanResultList[0].ageMs, 500 - 100);
+}
+
+TEST_F(WifiScanCacheTests, AgeLongUptime) {
+ constexpr auto kStartTime = Seconds(60 * 60 * 24 * 50); // 50 days
+ overrideMonotonicTime(kStartTime);
+ beginDefaultWifiCache(nullptr /* scannedFreqList */,
+ 0 /* scannedFreqListLen */);
+
+ overrideMonotonicTime(kStartTime + Milliseconds(500));
+ chreWifiScanResult result = {};
+ chreWifiScanCacheScanEventAdd(&result);
+
+ overrideMonotonicTime(kStartTime + Milliseconds(4000));
+ chreWifiScanCacheScanEventEnd(CHRE_ERROR_NONE);
+
+ ASSERT_EQ(gWifiScanResultList.size(), 1);
+ EXPECT_EQ(gWifiScanResultList[0].ageMs, 4000 - 500);
+}
+
+TEST_F(WifiScanCacheTests, AgeAvoidsUnderflow) {
+ constexpr auto kStartTime = Seconds(30);
+ constexpr auto kEndTime = kStartTime + Seconds(5);
+ overrideMonotonicTime(kStartTime);
+ beginDefaultWifiCache(nullptr /* scannedFreqList */,
+ 0 /* scannedFreqListLen */);
+
+ overrideMonotonicTime(Nanoseconds(0));
+ chreWifiScanResult result = {};
+ chreWifiScanCacheScanEventAdd(&result);
+
+ overrideMonotonicTime(kEndTime);
+ chreWifiScanCacheScanEventEnd(CHRE_ERROR_NONE);
+
+ ASSERT_EQ(gWifiScanResultList.size(), 1);
+ EXPECT_LT(gWifiScanResultList[0].ageMs,
+ Milliseconds(kEndTime - kStartTime).getMilliseconds());
+}
diff --git a/pal/util/wifi_scan_cache.c b/pal/util/wifi_scan_cache.c
index a927bd2..4a44071 100644
--- a/pal/util/wifi_scan_cache.c
+++ b/pal/util/wifi_scan_cache.c
@@ -21,6 +21,17 @@
#include "chre/util/macros.h"
/************************************************
+ * Constants
+ ***********************************************/
+
+// Constants used in chreWifiScanCacheInitialAgeMsValue() and
+// chreWifiScanCacheFinalizeAgeMs()
+// These values are selected because msec = nsec / 1000000 and
+// 1000000 = 64 * 15625 = (1 << 6) * 15625
+#define CHRE_WIFI_SCAN_CACHE_AGE_MS_SHIFT (6)
+#define CHRE_WIFI_SCAN_CACHE_AGE_MS_DIVISOR (15625)
+
+/************************************************
* Prototypes
***********************************************/
@@ -46,6 +57,8 @@
bool scanMonitoringEnabled;
uint32_t scannedFreqList[CHRE_WIFI_FREQUENCY_LIST_MAX_LEN];
+
+ uint64_t scanStartTimeNs;
};
/************************************************
@@ -187,6 +200,59 @@
return foundWeakerResult;
}
+static uint32_t chreWifiScanCacheInitialAgeMsValue() {
+ // ageMs will be finalized via chreWifiScanCacheFinalizeAgeMs() once the scan
+ // finishes, because it is relative to the scan end time that we can't know
+ // yet. Before the end of the scan, populate ageMs with the time since the
+ // start of the scan.
+ //
+ // We avoid 64-bit integer division by:
+ // - Only considering the delta between this result and the start of the
+ // scan, which constrains the range of expected values to what should be
+ // only a few seconds
+ // - Instead of directly dividing by 1000000, we first divide by 64 (right
+ // shift by 6), then truncate to 32 bits, then later we'll do integer
+ // division by 15625 to get milliseconds
+ // - This works because x/1000000 = x/(64 * 15625) = (x/64)/15625
+ // - The largest delta we can fit here is 2^32/15625 ms = 274877 ms or
+ // about 4.5 minutes
+ uint64_t timeSinceScanStartNs =
+ (gSystemApi->getCurrentTime() - gWifiCacheState.scanStartTimeNs);
+ return (timeSinceScanStartNs >> CHRE_WIFI_SCAN_CACHE_AGE_MS_SHIFT);
+}
+
+static void chreWifiScanCacheFinalizeAgeMs() {
+ // Convert ageMs from the chreWifiScanCacheInitialAgeMsValue() to its final,
+ // correct value using the formula derived from these steps:
+ // ageMs = (referenceTimeNs - absoluteScanResultTimeNs) / 1000000
+ // = (referenceTimeNs - (scanStartTimeNs + scanOffsetNs)) / 1000000
+ // = ((referenceTimeNs - scanStartTimeNs) - scanOffsetNs) / 1000000
+ // = (scanDuration / 64 - scanOffsetNs / 64) / 15625
+ // ageMs = (scanDurationShifted - currentAgeMsValue) / 15625
+ uint64_t referenceTimeNs = gWifiCacheState.event.referenceTime;
+ uint64_t scanStartTimeNs = gWifiCacheState.scanStartTimeNs;
+ uint32_t scanDurationShifted =
+ (referenceTimeNs - scanStartTimeNs) >> CHRE_WIFI_SCAN_CACHE_AGE_MS_SHIFT;
+ if (referenceTimeNs < scanStartTimeNs) {
+ gSystemApi->log(CHRE_LOG_ERROR, "Invalid scan timestamp, clamping");
+ // Use a smaller number to avoid very large ageMs values
+ scanDurationShifted = 78125000; // 5 seconds --> 5*10e9 / 64
+ }
+
+ for (uint16_t i = 0; i < gWifiCacheState.event.resultTotal; i++) {
+ if (scanDurationShifted < gWifiCacheState.resultList[i].ageMs) {
+ gSystemApi->log(CHRE_LOG_ERROR,
+ "Invalid result timestamp %" PRIu32 " vs. %" PRIu32,
+ gWifiCacheState.resultList[i].ageMs, scanDurationShifted);
+ gWifiCacheState.resultList[i].ageMs = 0;
+ } else {
+ gWifiCacheState.resultList[i].ageMs =
+ (scanDurationShifted - gWifiCacheState.resultList[i].ageMs) /
+ CHRE_WIFI_SCAN_CACHE_AGE_MS_DIVISOR;
+ }
+ }
+}
+
/************************************************
* Public functions
***********************************************/
@@ -242,6 +308,7 @@
gWifiCacheState.scanRequestedByChre = scanRequestedByChre;
gWifiCacheState.started = true;
+ gWifiCacheState.scanStartTimeNs = gSystemApi->getCurrentTime();
}
if (scanRequestedByChre && !success) {
@@ -277,10 +344,8 @@
memcpy(&gWifiCacheState.resultList[index], result,
sizeof(const struct chreWifiScanResult));
- // ageMs will be properly populated in chreWifiScanCacheScanEventEnd
gWifiCacheState.resultList[index].ageMs =
- (uint32_t)gSystemApi->getCurrentTime() /
- (uint32_t)kOneMillisecondInNanoseconds;
+ chreWifiScanCacheInitialAgeMsValue();
}
void chreWifiScanCacheScanEventEnd(enum chreError errorCode) {
@@ -299,14 +364,7 @@
(gWifiCacheState.scanRequestedByChre || gScanMonitoringEnabled)) {
gWifiCacheState.event.referenceTime = gSystemApi->getCurrentTime();
gWifiCacheState.event.scannedFreqList = gWifiCacheState.scannedFreqList;
-
- uint32_t referenceTimeMs = (uint32_t)gWifiCacheState.event.referenceTime /
- (uint32_t)kOneMillisecondInNanoseconds;
- for (uint16_t i = 0; i < gWifiCacheState.event.resultTotal; i++) {
- gWifiCacheState.resultList[i].ageMs =
- referenceTimeMs - gWifiCacheState.resultList[i].ageMs;
- }
-
+ chreWifiScanCacheFinalizeAgeMs();
chreWifiScanCacheDispatchAll();
}
