From 2d447169a339dbddb8934672625fc4fbfca7e47c Mon Sep 17 00:00:00 2001 From: Kevin Chyn Date: Mon, 12 Jun 2023 20:58:16 +0000 Subject: [PATCH] Do not use Map.Entry outside of lock Map.Entry returned from Map#entrySet are tied to the backing map. A change to the map is reflected to the Map.Entry. This causes undefined behavior (including NPE) if we try to use them outside of the lock. If a client removes their callback after we exit the lock but before we invoke the client callbacks, we run into NPE. Instead, lets copy the callback and executor into a separate data structure. Fixes: 286512746 Test: atest DeviceStateControllerTests Change-Id: I8ed29cf6e27f5c8020f120c3968896f627573a35 --- .../server/wm/DeviceStateController.java | 30 ++++++++++++------- .../server/wm/DeviceStateControllerTests.java | 28 ++++++++++++++--- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/wm/DeviceStateController.java b/services/core/java/com/android/server/wm/DeviceStateController.java index db4762e5f8773..857e03d71f3f1 100644 --- a/services/core/java/com/android/server/wm/DeviceStateController.java +++ b/services/core/java/com/android/server/wm/DeviceStateController.java @@ -20,6 +20,7 @@ import android.annotation.CallbackExecutor; import android.annotation.NonNull; import android.content.Context; import android.util.ArrayMap; +import android.util.Pair; import com.android.internal.R; import com.android.internal.annotations.GuardedBy; @@ -165,19 +166,28 @@ final class DeviceStateController { // Make a copy here because it's possible that the consumer tries to remove a callback // while we're still iterating through the list, which would end up in a - // ConcurrentModificationException. - final List, Executor>> entries = new ArrayList<>(); - synchronized (mWmLock) { - for (Map.Entry, Executor> entry - : mDeviceStateCallbacks.entrySet()) { - entries.add(entry); - } - } + // ConcurrentModificationException. Note that cannot use a List because the + // entries are tied to the backing map. So, if a client removes a callback while + // we are notifying clients, we will get a NPE. + final List, Executor>> entries = copyDeviceStateCallbacks(); for (int i = 0; i < entries.size(); i++) { - Map.Entry, Executor> entry = entries.get(i); - entry.getValue().execute(() -> entry.getKey().accept(mCurrentDeviceState)); + final Pair, Executor> entry = entries.get(i); + entry.second.execute(() -> entry.first.accept(deviceState)); } } } + + @VisibleForTesting + @NonNull + List, Executor>> copyDeviceStateCallbacks() { + final List, Executor>> entries = new ArrayList<>(); + + synchronized (mWmLock) { + mDeviceStateCallbacks.forEach((deviceStateConsumer, executor) -> { + entries.add(new Pair<>(deviceStateConsumer, executor)); + }); + } + return entries; + } } diff --git a/services/tests/wmtests/src/com/android/server/wm/DeviceStateControllerTests.java b/services/tests/wmtests/src/com/android/server/wm/DeviceStateControllerTests.java index 07d8b8a0c8dff..51255948ed4a7 100644 --- a/services/tests/wmtests/src/com/android/server/wm/DeviceStateControllerTests.java +++ b/services/tests/wmtests/src/com/android/server/wm/DeviceStateControllerTests.java @@ -17,17 +17,16 @@ package com.android.server.wm; import static com.android.dx.mockito.inline.extended.ExtendedMockito.mock; -import static com.android.dx.mockito.inline.extended.ExtendedMockito.verify; import static com.android.dx.mockito.inline.extended.ExtendedMockito.when; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; import android.content.Context; import android.content.res.Resources; import android.hardware.devicestate.DeviceStateManager; import android.platform.test.annotations.Presubmit; +import android.util.Pair; import androidx.test.filters.SmallTest; @@ -38,6 +37,8 @@ import com.google.common.util.concurrent.MoreExecutors; import org.junit.Before; import org.junit.Test; +import java.util.List; +import java.util.concurrent.Executor; import java.util.function.Consumer; /** @@ -58,6 +59,7 @@ public class DeviceStateControllerTests { private DeviceStateController.DeviceState mCurrentState = DeviceStateController.DeviceState.UNKNOWN; private Consumer mDelegate; + private Executor mExecutor = MoreExecutors.directExecutor(); @Before public void setUp() { @@ -124,13 +126,31 @@ public class DeviceStateControllerTests { mTarget.onDeviceStateReceivedByDisplayManager(mFoldedStates[0]); assertEquals(DeviceStateController.DeviceState.FOLDED, mCurrentState); - // The callback should not receive state change when the it is unregistered. + // The callback should not receive state change when it is unregistered. mTarget.unregisterDeviceStateCallback(mDelegate); assertTrue(mTarget.mDeviceStateCallbacks.isEmpty()); mTarget.onDeviceStateReceivedByDisplayManager(mOpenDeviceStates[0]); assertEquals(DeviceStateController.DeviceState.FOLDED /* unchanged */, mCurrentState); } + @Test + public void testCopyDeviceStateCallbacks() { + initialize(true /* supportFold */, true /* supportHalfFolded */); + assertEquals(1, mTarget.mDeviceStateCallbacks.size()); + assertTrue(mTarget.mDeviceStateCallbacks.containsKey(mDelegate)); + + List, Executor>> entries = + mTarget.copyDeviceStateCallbacks(); + mTarget.unregisterDeviceStateCallback(mDelegate); + + // In contrast to List where the entries are tied to changes in the backing map, + // List should still contain non-null callbacks and executors even though they were + // removed from the backing map via the unregister method above. + assertEquals(1, entries.size()); + assertEquals(mDelegate, entries.get(0).first); + assertEquals(mExecutor, entries.get(0).second); + } + private final int[] mFoldedStates = {0}; private final int[] mOpenDeviceStates = {1}; private final int[] mHalfFoldedStates = {2}; @@ -194,7 +214,7 @@ public class DeviceStateControllerTests { when(mMockContext.getResources()).thenReturn((mockRes)); mockFold(mSupportFold, mSupportHalfFold); mTarget = new DeviceStateController(mMockContext, new WindowManagerGlobalLock()); - mTarget.registerDeviceStateCallback(mDelegate, MoreExecutors.directExecutor()); + mTarget.registerDeviceStateCallback(mDelegate, mExecutor); } } }