From ef64770162f19acc6e6d61e6dd57d43a5dfd4818 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 30 May 2019 03:50:19 -0700 Subject: [PATCH] Prevent a 4-way binder interlock leading to a leak. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit System server | NetworkStack | NetworkMonitorCallbacks ←----|--- NetworkMonitorCallbacks$Stub$Proxy ↓ | ↑ NetworkAgentInfo | NetworkMonitor ↓ | ↑ NetworkMonitor$Stub$Proxy ----|---→ NetworkMonitorImpl Bug: b/133174607 Test: Manual. The simplest artifact is observed by watching the output of adb shell dumpsys meminfo -d com.android.networkstack | grep 'Proxy Binders' while connecting and disconnecting multiple times to any network. This will display the number of binder proxies. Before this, the binder proxy count increases by 1 with each connection and never goes down (there is some noise, as proxy objects are sometimes created for other reasons, and get GC'd eventually). After this, the binder proxy count is always eventually stable at 27 + connected network count. See the bug for the complete analysis. Merged-In: Ide2428dab3fcd6d7cd00aa2a9fd99d6c99b815a4 Change-Id: I6b74cf12bdbc72c0593d2a4d6f39c895d1ef3534 (cherry picked from commit eb43884fee35102a7fc886750d6a7e891a82ce33) --- .../android/server/ConnectivityService.java | 19 +++++---- .../connectivity/AutodestructReference.java | 42 +++++++++++++++++++ 2 files changed, 52 insertions(+), 9 deletions(-) create mode 100644 services/core/java/com/android/server/connectivity/AutodestructReference.java diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 45f7360a25..863389b85d 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -169,6 +169,7 @@ import com.android.internal.util.MessageUtils; import com.android.internal.util.WakeupMessage; import com.android.internal.util.XmlUtils; import com.android.server.am.BatteryStatsService; +import com.android.server.connectivity.AutodestructReference; import com.android.server.connectivity.DataConnectionStats; import com.android.server.connectivity.DnsManager; import com.android.server.connectivity.DnsManager.PrivateDnsValidationUpdate; @@ -2763,29 +2764,31 @@ public class ConnectivityService extends IConnectivityManager.Stub } private class NetworkMonitorCallbacks extends INetworkMonitorCallbacks.Stub { - private final NetworkAgentInfo mNai; + private final int mNetId; + private final AutodestructReference mNai; private NetworkMonitorCallbacks(NetworkAgentInfo nai) { - mNai = nai; + mNetId = nai.network.netId; + mNai = new AutodestructReference(nai); } @Override public void onNetworkMonitorCreated(INetworkMonitor networkMonitor) { mHandler.sendMessage(mHandler.obtainMessage(EVENT_REGISTER_NETWORK_AGENT, - new Pair<>(mNai, networkMonitor))); + new Pair<>(mNai.getAndDestroy(), networkMonitor))); } @Override public void notifyNetworkTested(int testResult, @Nullable String redirectUrl) { mTrackerHandler.sendMessage(mTrackerHandler.obtainMessage(EVENT_NETWORK_TESTED, - testResult, mNai.network.netId, redirectUrl)); + testResult, mNetId, redirectUrl)); } @Override public void notifyPrivateDnsConfigResolved(PrivateDnsConfigParcel config) { mTrackerHandler.sendMessage(mTrackerHandler.obtainMessage( EVENT_PRIVATE_DNS_CONFIG_RESOLVED, - 0, mNai.network.netId, PrivateDnsConfig.fromParcel(config))); + 0, mNetId, PrivateDnsConfig.fromParcel(config))); } @Override @@ -2803,15 +2806,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } mTrackerHandler.sendMessage(mTrackerHandler.obtainMessage( EVENT_PROVISIONING_NOTIFICATION, PROVISIONING_NOTIFICATION_SHOW, - mNai.network.netId, - pendingIntent)); + mNetId, pendingIntent)); } @Override public void hideProvisioningNotification() { mTrackerHandler.sendMessage(mTrackerHandler.obtainMessage( - EVENT_PROVISIONING_NOTIFICATION, PROVISIONING_NOTIFICATION_HIDE, - mNai.network.netId)); + EVENT_PROVISIONING_NOTIFICATION, PROVISIONING_NOTIFICATION_HIDE, mNetId)); } @Override diff --git a/services/core/java/com/android/server/connectivity/AutodestructReference.java b/services/core/java/com/android/server/connectivity/AutodestructReference.java new file mode 100644 index 0000000000..009a43e582 --- /dev/null +++ b/services/core/java/com/android/server/connectivity/AutodestructReference.java @@ -0,0 +1,42 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.connectivity; + +import android.annotation.NonNull; + +import java.util.concurrent.atomic.AtomicReference; + +/** + * A ref that autodestructs at the first usage of it. + * @param The type of the held object + * @hide + */ +public class AutodestructReference { + private final AtomicReference mHeld; + public AutodestructReference(@NonNull T obj) { + if (null == obj) throw new NullPointerException("Autodestruct reference to null"); + mHeld = new AtomicReference<>(obj); + } + + /** Get the ref and destruct it. NPE if already destructed. */ + @NonNull + public T getAndDestroy() { + final T obj = mHeld.getAndSet(null); + if (null == obj) throw new NullPointerException("Already autodestructed"); + return obj; + } +}