Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Do not use Map.Entry outside of lock

Browse Source 
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
This commit is contained in:
Kevin Chyn
2023-06-12 20:58:16 +00:00
parent b3ea50636a
commit 2d447169a3
2 changed files with 44 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
services/core/java/com/android/server/wm/DeviceStateController.java
View File

@@ -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<Map.Entry<Consumer<DeviceState>, Executor>> entries = new ArrayList<>();
synchronized (mWmLock) {
for (Map.Entry<Consumer<DeviceState>, Executor> entry
: mDeviceStateCallbacks.entrySet()) {
entries.add(entry);
}
}
// ConcurrentModificationException. Note that cannot use a List<Map.Entry> 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<Pair<Consumer<DeviceState>, Executor>> entries = copyDeviceStateCallbacks();
for (int i = 0; i < entries.size(); i++) {
Map.Entry<Consumer<DeviceState>, Executor> entry = entries.get(i);
entry.getValue().execute(() -> entry.getKey().accept(mCurrentDeviceState));
final Pair<Consumer<DeviceState>, Executor> entry = entries.get(i);
entry.second.execute(() -> entry.first.accept(deviceState));
}
}
}
@VisibleForTesting
@NonNull
List<Pair<Consumer<DeviceState>, Executor>> copyDeviceStateCallbacks() {
final List<Pair<Consumer<DeviceState>, Executor>> entries = new ArrayList<>();
synchronized (mWmLock) {
mDeviceStateCallbacks.forEach((deviceStateConsumer, executor) -> {
entries.add(new Pair<>(deviceStateConsumer, executor));
});
}
return entries;
}
}

28
services/tests/wmtests/src/com/android/server/wm/DeviceStateControllerTests.java
View File

@@ -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<DeviceStateController.DeviceState> 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<Pair<Consumer<DeviceStateController.DeviceState>, Executor>> entries =
mTarget.copyDeviceStateCallbacks();
mTarget.unregisterDeviceStateCallback(mDelegate);
// In contrast to List<Map.Entry> where the entries are tied to changes in the backing map,
// List<Pair> 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);
}
}
}