commit 35a397c495f86f8a59248ff11ce6d71a9a749fcb
author Tomislav Novak <tnovak@meta.com> Thu Oct 19 09:49:31 2023 -0700
committer Tomislav Novak <tnovak@meta.com> Thu Oct 19 09:56:52 2023 -0700
tree 58ac3f339ae292e088e284fb667fecb58efc8257
parent 6b30dd666d4b215ee49af09ff37c7428f1a05e16

StrictMode: fix race condition in onVmPolicyViolation

There's a TOCTOU race condition in onVmPolicyViolation() that can cause
a NullPointerException if multiple threads trigger a violation and a
penalty listener is set. For example:

1. Thread 1 passes the mCallbackExecutor null check and calls execute()
2. T2 passes the same check and then gets preempted
3. Runnable queued by T1 temporarily replaces sVmPolicy with LAX (which
   has a null executor) by calling allowVmViolations()
4. T2 calls execute() on sVmPolicy.mCallbackExecutor, which is now null

Fix it by using the same VmPolicy object throughout onVmPolicyViolation.

Test: atest StrictModeTest
Change-Id: Ifa20253ea936b8d3d8c3719c3278bfaccbdf8275

core/java/android/os/StrictMode.java

diff --git a/core/java/android/os/StrictMode.java b/core/java/android/os/StrictMode.java
index 180735b..9c051ac 100644
--- a/core/java/android/os/StrictMode.java
+++ b/core/java/android/os/StrictMode.java
@@ -2436,11 +2436,12 @@
 
     /** @hide */
     public static void onVmPolicyViolation(Violation violation, boolean forceDeath) {
-        final boolean penaltyDropbox = (sVmPolicy.mask & PENALTY_DROPBOX) != 0;
-        final boolean penaltyDeath = ((sVmPolicy.mask & PENALTY_DEATH) != 0) || forceDeath;
-        final boolean penaltyLog = (sVmPolicy.mask & PENALTY_LOG) != 0;
+        final VmPolicy vmPolicy = getVmPolicy();
+        final boolean penaltyDropbox = (vmPolicy.mask & PENALTY_DROPBOX) != 0;
+        final boolean penaltyDeath = ((vmPolicy.mask & PENALTY_DEATH) != 0) || forceDeath;
+        final boolean penaltyLog = (vmPolicy.mask & PENALTY_LOG) != 0;
 
-        final int penaltyMask = (sVmPolicy.mask & PENALTY_ALL);
+        final int penaltyMask = (vmPolicy.mask & PENALTY_ALL);
         final ViolationInfo info = new ViolationInfo(violation, penaltyMask);
 
         // Erase stuff not relevant for process-wide violations
@@ -2493,10 +2494,10 @@
 
         // If penaltyDeath, we can't guarantee this callback finishes before the process dies for
         // all executors. penaltyDeath supersedes penaltyCallback.
-        if (sVmPolicy.mListener != null && sVmPolicy.mCallbackExecutor != null) {
-            final OnVmViolationListener listener = sVmPolicy.mListener;
+        if (vmPolicy.mListener != null && vmPolicy.mCallbackExecutor != null) {
+            final OnVmViolationListener listener = vmPolicy.mListener;
             try {
-                sVmPolicy.mCallbackExecutor.execute(
+                vmPolicy.mCallbackExecutor.execute(
                         () -> {
                             // Lift violated policy to prevent infinite recursion.
                             VmPolicy oldPolicy = allowVmViolations();