From 98732dbd6df1d436ea47831e4f6d83e8e758ca8e Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 3 Feb 2023 21:26:59 +0900 Subject: [PATCH] Use the binder token to reference AutoKI in alarm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch uses the binder token represented by the callback to communicate the identity of the AutoOnOffKeepalive through the alarm mechanism. This is a lot more robust and easy to understand. Addressing this, on top of the robustness and readability advantages, corrects three bugs. • The |obj| in the message to CMD_MONITOR_AUTOMATIC_KEEPALIVE is now the binder token both on the sender side and the receiver side, while the previous code had a Network in the sender while the receiver expected AutoOnOffKeepalive, crashing the system server with a wrong cast. • The intent sent in the alarm would have a yet-uninitialized value for the slot, so it would not be possible to find the auto keepalive when the alarm fires. • When the slot is reassigned, the alarm continued to fire with the same slot value, so the auto keepalive would no longer be found after a pause/resume cycle that would change the slot. Test: FrameworksNetTests Change-Id: Ibdbfcd884e0d3559206cbaae7b6b7a524972c3ca --- framework/src/android/net/NetworkAgent.java | 2 +- .../android/server/ConnectivityService.java | 4 +- .../AutomaticOnOffKeepaliveTracker.java | 46 +++++++++++++------ .../server/connectivity/KeepaliveTracker.java | 3 +- 4 files changed, 38 insertions(+), 17 deletions(-) diff --git a/framework/src/android/net/NetworkAgent.java b/framework/src/android/net/NetworkAgent.java index 62e4fe1406..3ec00d9f03 100644 --- a/framework/src/android/net/NetworkAgent.java +++ b/framework/src/android/net/NetworkAgent.java @@ -491,7 +491,7 @@ public abstract class NetworkAgent { * TCP sockets are open over a VPN. The system will check periodically for presence of * such open sockets, and this message is what triggers the re-evaluation. * - * obj = AutomaticOnOffKeepaliveObject. + * obj = A Binder object associated with the keepalive. * @hide */ public static final int CMD_MONITOR_AUTOMATIC_KEEPALIVE = BASE + 30; diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 330a1da7dd..f1c68cbf77 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -5547,7 +5547,9 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } case NetworkAgent.CMD_MONITOR_AUTOMATIC_KEEPALIVE: { - final AutomaticOnOffKeepalive ki = (AutomaticOnOffKeepalive) msg.obj; + final AutomaticOnOffKeepalive ki = + mKeepaliveTracker.getKeepaliveForBinder((IBinder) msg.obj); + if (null == ki) return; // The callback was unregistered before the alarm fired final Network network = ki.getNetwork(); boolean networkFound = false; diff --git a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java index 46fff6cdfd..18e2dd8e7b 100644 --- a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java @@ -45,6 +45,7 @@ import android.net.MarkMaskParcel; import android.net.Network; import android.net.NetworkAgent; import android.net.SocketKeepalive.InvalidSocketException; +import android.os.Bundle; import android.os.FileUtils; import android.os.Handler; import android.os.IBinder; @@ -60,6 +61,7 @@ import android.util.SparseArray; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.IndentingPrintWriter; import com.android.modules.utils.build.SdkLevel; +import com.android.net.module.util.BinderUtils; import com.android.net.module.util.CollectionUtils; import com.android.net.module.util.DeviceConfigUtils; import com.android.net.module.util.HexDump; @@ -92,8 +94,7 @@ public class AutomaticOnOffKeepaliveTracker { private static final int[] ADDRESS_FAMILIES = new int[] {AF_INET6, AF_INET}; private static final String ACTION_TCP_POLLING_ALARM = "com.android.server.connectivity.KeepaliveTracker.TCP_POLLING_ALARM"; - private static final String EXTRA_NETWORK = "network_id"; - private static final String EXTRA_SLOT = "slot"; + private static final String EXTRA_BINDER_TOKEN = "token"; private static final long DEFAULT_TCP_POLLING_INTERVAL_MS = 120_000L; private static final String AUTOMATIC_ON_OFF_KEEPALIVE_VERSION = "automatic_on_off_keepalive_version"; @@ -159,11 +160,10 @@ public class AutomaticOnOffKeepaliveTracker { public void onReceive(Context context, Intent intent) { if (ACTION_TCP_POLLING_ALARM.equals(intent.getAction())) { Log.d(TAG, "Received TCP polling intent"); - final Network network = intent.getParcelableExtra(EXTRA_NETWORK); - final int slot = intent.getIntExtra(EXTRA_SLOT, -1); + final IBinder token = intent.getBundleExtra(EXTRA_BINDER_TOKEN).getBinder( + EXTRA_BINDER_TOKEN); mConnectivityServiceHandler.obtainMessage( - NetworkAgent.CMD_MONITOR_AUTOMATIC_KEEPALIVE, - slot, 0 , network).sendToTarget(); + NetworkAgent.CMD_MONITOR_AUTOMATIC_KEEPALIVE, token).sendToTarget(); } } }; @@ -183,6 +183,8 @@ public class AutomaticOnOffKeepaliveTracker { public class AutomaticOnOffKeepalive { @NonNull private final KeepaliveTracker.KeepaliveInfo mKi; + @NonNull + private final ISocketKeepaliveCallback mCallback; @Nullable private final FileDescriptor mFd; @Nullable @@ -193,6 +195,7 @@ public class AutomaticOnOffKeepaliveTracker { AutomaticOnOffKeepalive(@NonNull final KeepaliveTracker.KeepaliveInfo ki, final boolean autoOnOff, @NonNull Context context) throws InvalidSocketException { this.mKi = Objects.requireNonNull(ki); + mCallback = ki.mCallback; if (autoOnOff && mDependencies.isFeatureEnabled(AUTOMATIC_ON_OFF_KEEPALIVE_VERSION)) { mAutomaticOnOffState = STATE_ENABLED; if (null == ki.mFd) { @@ -205,8 +208,7 @@ public class AutomaticOnOffKeepaliveTracker { Log.e(TAG, "Cannot dup fd: ", e); throw new InvalidSocketException(ERROR_INVALID_SOCKET, e); } - mTcpPollingAlarm = createTcpPollingAlarmIntent( - context, ki.getNai().network(), ki.getSlot()); + mTcpPollingAlarm = createTcpPollingAlarmIntent(context, mCallback.asBinder()); } else { mAutomaticOnOffState = STATE_ALWAYS_ON; // A null fd is acceptable in KeepaliveInfo for backward compatibility of @@ -226,12 +228,14 @@ public class AutomaticOnOffKeepaliveTracker { } private PendingIntent createTcpPollingAlarmIntent(@NonNull Context context, - @NonNull Network network, int slot) { + @NonNull IBinder token) { final Intent intent = new Intent(ACTION_TCP_POLLING_ALARM); - intent.putExtra(EXTRA_NETWORK, network); - intent.putExtra(EXTRA_SLOT, slot); - return PendingIntent.getBroadcast( - context, 0 /* requestCode */, intent, PendingIntent.FLAG_IMMUTABLE); + // Intent doesn't expose methods to put extra Binders, but Bundle does. + final Bundle b = new Bundle(); + b.putBinder(EXTRA_BINDER_TOKEN, token); + intent.putExtra(EXTRA_BINDER_TOKEN, b); + return BinderUtils.withCleanCallingIdentity(() -> PendingIntent.getBroadcast( + context, 0 /* requestCode */, intent, PendingIntent.FLAG_IMMUTABLE)); } } @@ -318,13 +322,14 @@ public class AutomaticOnOffKeepaliveTracker { newKi = autoKi.mKi.withFd(autoKi.mFd); } catch (InvalidSocketException | IllegalArgumentException | SecurityException e) { Log.e(TAG, "Fail to construct keepalive", e); - mKeepaliveTracker.notifyErrorCallback(autoKi.mKi.mCallback, ERROR_INVALID_SOCKET); + mKeepaliveTracker.notifyErrorCallback(autoKi.mCallback, ERROR_INVALID_SOCKET); return; } autoKi.mAutomaticOnOffState = STATE_ENABLED; handleResumeKeepalive(newKi); } + // TODO : this method should be removed ; the keepalives should always be indexed by callback private int findAutomaticOnOffKeepaliveIndex(@NonNull Network network, int slot) { ensureRunningOnHandlerThread(); @@ -338,6 +343,7 @@ public class AutomaticOnOffKeepaliveTracker { return -1; } + // TODO : this method should be removed ; the keepalives should always be indexed by callback @Nullable private AutomaticOnOffKeepalive findAutomaticOnOffKeepalive(@NonNull Network network, int slot) { @@ -347,6 +353,18 @@ public class AutomaticOnOffKeepaliveTracker { return (index >= 0) ? mAutomaticOnOffKeepalives.get(index) : null; } + /** + * Find the AutomaticOnOffKeepalive associated with a given callback. + * @return the keepalive associated with this callback, or null if none + */ + @Nullable + public AutomaticOnOffKeepalive getKeepaliveForBinder(@NonNull final IBinder token) { + ensureRunningOnHandlerThread(); + + return CollectionUtils.findFirst(mAutomaticOnOffKeepalives, + it -> it.mCallback.asBinder().equals(token)); + } + /** * Handle keepalive events from lower layer. * diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java index 63b76c79dc..7cb613b551 100644 --- a/service/src/com/android/server/connectivity/KeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/KeepaliveTracker.java @@ -125,8 +125,9 @@ public class KeepaliveTracker { * which is only returned when the hardware has successfully started the keepalive. */ class KeepaliveInfo implements IBinder.DeathRecipient { - // Bookkeeping data. + // TODO : remove this member. Only AutoOnOffKeepalive should have a reference to this. public final ISocketKeepaliveCallback mCallback; + // Bookkeeping data. private final int mUid; private final int mPid; private final boolean mPrivileged;