From ede7ae7dc67ba45302d1bb4f3f86df2d8df25d38 Mon Sep 17 00:00:00 2001
From: Ytai Ben-Tsvi <ytai@google.com>
Date: Mon, 27 Apr 2020 11:09:41 -0700
Subject: [PATCH] Fix deadlock between APS and sound trigger middleware

SoundTriggerMiddlewareValidation.setCaptureState() acquired a lock,
which is also acquired by load{Phrase,}Model() and unloadModel(),
which in turn call into audio policy service for the sake of
acquiring/releasing sound trigger capture session handles.

Since audio policy manager is single threaded, a deadlock may result
if setCaptureState() occurs concurrently with one of the model loading
operations:

- System server thread is in the process of loading, acquired the lock
  in SoundTriggerMiddlewareValidation and calls into APS, blocked
  waiting to acquire the mutex of APM.
- Audio server thread is in the process of changing the capture state,
  acquired the APM mutex and is calling into sound trigger middleware,
  blocked waiting to acquire the lock in
  SoundTriggerMiddlewareValidation.

The fix is simple: no need to lock SoundTriggerMiddlewareValidation if
using an atomic for the capture state. Verified by careful reading of
the code that this is the only contention point.

Bug: 154383165
Test: Hard to verify a rare deadlock, ran a quick verification of
      basic sound trigger functionality.
Change-Id: I1350316521d56d7de74f20ad980f92f0df0035f8
---
 .../SoundTriggerMiddlewareValidation.java           | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java
index bae244179346f..9868c3928dbc9 100644
--- a/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java
+++ b/services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java
@@ -47,6 +47,7 @@ import java.util.HashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * This is a decorator of an {@link ISoundTriggerMiddlewareService}, which enforces permissions and
@@ -123,7 +124,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
         }
     }
 
-    private Boolean mCaptureState;
+    private AtomicReference<Boolean> mCaptureState = new AtomicReference<>();
 
     private final @NonNull ISoundTriggerMiddlewareInternal mDelegate;
     private final @NonNull Context mContext;
@@ -230,10 +231,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
         } catch (Exception e) {
             throw handleException(e);
         } finally {
-            // It is safe to lock here - local operation.
-            synchronized (this) {
-                mCaptureState = active;
-            }
+            mCaptureState.set(active);
         }
     }
 
@@ -287,8 +285,9 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware
     @Override
     public void dump(PrintWriter pw) {
         synchronized (this) {
-            pw.printf("Capture state is %s\n\n", mCaptureState == null ? "uninitialized"
-                    : (mCaptureState ? "active" : "inactive"));
+            Boolean captureState = mCaptureState.get();
+            pw.printf("Capture state is %s\n\n", captureState == null ? "uninitialized"
+                    : (captureState ? "active" : "inactive"));
             if (mModules != null) {
                 for (int handle : mModules.keySet()) {
                     final ModuleState module = mModules.get(handle);