From 213049e66f5a1c6d88fba82d7fda415c2776a9ea Mon Sep 17 00:00:00 2001
From: Fabian Kozynski <kozynski@google.com>
Date: Wed, 6 Nov 2019 17:23:17 -0500
Subject: [PATCH] Ensure that maps are only accessed in BG thread

If the maps are accessed from main thread (as well as bg thread), they
could be in an inconsistent state. In particular they could have nulls
for non-nullable types.

This CL asserts that createFilter is only called in BG thread. It also
makes sure that only non-nulls are passed to IntentFilter#addAction.

Test: atest UserBroadcastDispatcherTest
Fixes: 144039089
Change-Id: I58e66bc9a861c70c830fcbd4476d6dfc991c4ad0
---
 .../broadcast/UserBroadcastDispatcher.kt      | 74 +++++++++++++------
 1 file changed, 51 insertions(+), 23 deletions(-)

diff --git a/packages/SystemUI/src/com/android/systemui/broadcast/UserBroadcastDispatcher.kt b/packages/SystemUI/src/com/android/systemui/broadcast/UserBroadcastDispatcher.kt
index 24357a740331e..d50666c1b3cf0 100644
--- a/packages/SystemUI/src/com/android/systemui/broadcast/UserBroadcastDispatcher.kt
+++ b/packages/SystemUI/src/com/android/systemui/broadcast/UserBroadcastDispatcher.kt
@@ -27,6 +27,8 @@ import android.os.UserHandle
 import android.util.ArrayMap
 import android.util.ArraySet
 import android.util.Log
+import androidx.annotation.MainThread
+import com.android.internal.util.Preconditions
 import com.android.systemui.Dumpable
 import java.io.FileDescriptor
 import java.io.PrintWriter
@@ -72,7 +74,14 @@ class UserBroadcastDispatcher(
 
     internal fun isRegistered() = registered.get()
 
-    private val registerReceiver = Runnable {
+    // Only modify in BG thread
+    private val actionsToReceivers = ArrayMap<String, MutableSet<ReceiverData>>()
+    private val receiverToReceiverData = ArrayMap<BroadcastReceiver, MutableSet<ReceiverData>>()
+
+    // Only call on BG thread as it reads from the maps
+    private fun createFilter(): IntentFilter {
+        Preconditions.checkState(bgHandler.looper.isCurrentThread,
+                "This method should only be called from BG thread")
         val categories = mutableSetOf<String>()
         receiverToReceiverData.values.flatten().forEach {
             it.filter.categoriesIterator()?.asSequence()?.let {
@@ -80,30 +89,13 @@ class UserBroadcastDispatcher(
             }
         }
         val intentFilter = IntentFilter().apply {
-            actionsToReceivers.keys.forEach { addAction(it) }
+            // The keys of the arrayMap are of type String! so null check is needed
+            actionsToReceivers.keys.forEach { if (it != null) addAction(it) else Unit }
             categories.forEach { addCategory(it) }
         }
-
-        if (registered.get()) {
-            context.unregisterReceiver(this)
-            registered.set(false)
-        }
-        // Short interval without receiver, this can be problematic
-        if (intentFilter.countActions() > 0 && !registered.get()) {
-            context.registerReceiverAsUser(
-                    this,
-                    UserHandle.of(userId),
-                    intentFilter,
-                    null,
-                    bgHandler)
-            registered.set(true)
-        }
+        return intentFilter
     }
 
-    // Only modify in BG thread
-    private val actionsToReceivers = ArrayMap<String, MutableSet<ReceiverData>>()
-    private val receiverToReceiverData = ArrayMap<BroadcastReceiver, MutableSet<ReceiverData>>()
-
     override fun onReceive(context: Context, intent: Intent) {
         val id = if (DEBUG) index.getAndIncrement() else 0
         if (DEBUG) Log.w(TAG, "[$id] Received $intent")
@@ -126,6 +118,8 @@ class UserBroadcastDispatcher(
     }
 
     private fun handleRegisterReceiver(receiverData: ReceiverData) {
+        Preconditions.checkState(bgHandler.looper.isCurrentThread,
+                "This method should only be called from BG thread")
         if (DEBUG) Log.w(TAG, "Register receiver: ${receiverData.receiver}")
         receiverToReceiverData.getOrPut(receiverData.receiver, { ArraySet() }).add(receiverData)
         var changed = false
@@ -138,11 +132,13 @@ class UserBroadcastDispatcher(
             }.add(receiverData)
         }
         if (changed) {
-            mainHandler.post(registerReceiver)
+            createFilterAndRegisterReceiverBG()
         }
     }
 
     private fun handleUnregisterReceiver(receiver: BroadcastReceiver) {
+        Preconditions.checkState(bgHandler.looper.isCurrentThread,
+                "This method should only be called from BG thread")
         if (DEBUG) Log.w(TAG, "Unregister receiver: $receiver")
         val actions = receiverToReceiverData.getOrElse(receiver) { return }
                 .flatMap { it.filter.actionsIterator().asSequence().asIterable() }.toSet()
@@ -156,10 +152,16 @@ class UserBroadcastDispatcher(
             }
         }
         if (changed) {
-            mainHandler.post(registerReceiver)
+            createFilterAndRegisterReceiverBG()
         }
     }
 
+    // Only call this from a BG thread
+    private fun createFilterAndRegisterReceiverBG() {
+        val intentFilter = createFilter()
+        mainHandler.post(RegisterReceiverRunnable(intentFilter))
+    }
+
     override fun dump(fd: FileDescriptor, pw: PrintWriter, args: Array<out String>) {
         pw.println("  Registered=${registered.get()}")
         actionsToReceivers.forEach { (action, list) ->
@@ -191,4 +193,30 @@ class UserBroadcastDispatcher(
                     }
         }
     }
+
+    private inner class RegisterReceiverRunnable(val intentFilter: IntentFilter) : Runnable {
+
+        /*
+         * Registers and unregisters the BroadcastReceiver
+         *
+         * Must be called from Main Thread
+         */
+        @MainThread
+        override fun run() {
+            if (registered.get()) {
+                context.unregisterReceiver(this@UserBroadcastDispatcher)
+                registered.set(false)
+            }
+            // Short interval without receiver, this can be problematic
+            if (intentFilter.countActions() > 0 && !registered.get()) {
+                context.registerReceiverAsUser(
+                        this@UserBroadcastDispatcher,
+                        UserHandle.of(userId),
+                        intentFilter,
+                        null,
+                        bgHandler)
+                registered.set(true)
+            }
+        }
+    }
 }