From c635cfddc776e44aaae62893dbbfcae6c35fe40a Mon Sep 17 00:00:00 2001
From: Dmitri Plotnikov <dplotnikov@google.com>
Date: Thu, 16 Dec 2021 11:36:51 -0800
Subject: [PATCH] Fix concurrency issue with BatteryUsageStats

BatteryUsageStats is created under a BatteryStatsImpl lock.  One of
the elements of BatteryUsageStats is the battery history buffer Parcel.
Once the BatteryUsageStats object is created, the BatteryStatsImpl lock
is released and the history buffer parcel continues to be appended
by BatteryStatsImpl.  The Parcel may even be reset altogether if the
battery stats session is reset.  The BatteryUsageStats object is parceled
during the getBatteryUsageStats binder call. Any modification of the
history buffer concurrent with parceling causes a crash.

Bug: 194256984
Test: atest FrameworksCoreTests:BatteryUsageStatsTest FrameworksCoreTests:BatteryUsageStatsProviderTest
Change-Id: Ifb03a32275dfbea172cd28309a42349d6dd4bcd5
---
 .../internal/os/BatteryUsageStatsProvider.java    | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/core/java/com/android/internal/os/BatteryUsageStatsProvider.java b/core/java/com/android/internal/os/BatteryUsageStatsProvider.java
index 88425be843130..c1111ec259c9d 100644
--- a/core/java/com/android/internal/os/BatteryUsageStatsProvider.java
+++ b/core/java/com/android/internal/os/BatteryUsageStatsProvider.java
@@ -21,11 +21,13 @@ import android.hardware.SensorManager;
 import android.os.BatteryStats;
 import android.os.BatteryUsageStats;
 import android.os.BatteryUsageStatsQuery;
+import android.os.Parcel;
 import android.os.SystemClock;
 import android.os.UidBatteryConsumer;
 import android.util.Log;
 import android.util.SparseArray;
 
+import com.android.internal.annotations.GuardedBy;
 import com.android.internal.annotations.VisibleForTesting;
 
 import java.util.ArrayList;
@@ -133,9 +135,12 @@ public class BatteryUsageStatsProvider {
      */
     @VisibleForTesting
     public BatteryUsageStats getBatteryUsageStats(BatteryUsageStatsQuery query) {
-        return getBatteryUsageStats(query, currentTimeMillis());
+        synchronized (mStats) {
+            return getBatteryUsageStats(query, currentTimeMillis());
+        }
     }
 
+    @GuardedBy("mStats")
     private BatteryUsageStats getBatteryUsageStats(BatteryUsageStatsQuery query,
             long currentTimeMs) {
         if (query.getToTimestamp() == 0) {
@@ -145,6 +150,7 @@ public class BatteryUsageStatsProvider {
         }
     }
 
+    @GuardedBy("mStats")
     private BatteryUsageStats getCurrentBatteryUsageStats(BatteryUsageStatsQuery query,
             long currentTimeMs) {
         final long realtimeUs = elapsedRealtime() * 1000;
@@ -189,7 +195,12 @@ public class BatteryUsageStatsProvider {
             }
 
             BatteryStatsImpl batteryStatsImpl = (BatteryStatsImpl) mStats;
-            batteryUsageStatsBuilder.setBatteryHistory(batteryStatsImpl.mHistoryBuffer);
+
+            // Make a copy of battery history to avoid concurrent modification.
+            Parcel historyBuffer = Parcel.obtain();
+            historyBuffer.appendFrom(batteryStatsImpl.mHistoryBuffer, 0,
+                    batteryStatsImpl.mHistoryBuffer.dataSize());
+            batteryUsageStatsBuilder.setBatteryHistory(historyBuffer);
         }
 
         return batteryUsageStatsBuilder.build();