From bdb8282604556f1a4e4665227d253d5cde2f46b9 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 19 Jan 2023 23:14:02 +0900 Subject: [PATCH] [DK3] Send onPause/onResume keepalive callbacks Test: CTS in the patch immediately on top of this, [DK4] Change-Id: I208ceceb37c7977452479361f70f046fabafb37a --- .../src/android/net/ConnectivityManager.java | 29 ++++++++ .../android/net/ISocketKeepaliveCallback.aidl | 4 ++ .../src/android/net/SocketKeepalive.java | 40 +++++++++++ .../AutomaticOnOffKeepaliveTracker.java | 38 ++++++++--- .../server/connectivity/KeepaliveTracker.java | 66 +++++++++++++++++-- 5 files changed, 163 insertions(+), 14 deletions(-) diff --git a/framework/src/android/net/ConnectivityManager.java b/framework/src/android/net/ConnectivityManager.java index 17389b4529..497c105e66 100644 --- a/framework/src/android/net/ConnectivityManager.java +++ b/framework/src/android/net/ConnectivityManager.java @@ -2213,9 +2213,13 @@ public class ConnectivityManager { /** The requested keepalive was successfully started. */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public void onStarted() {} + /** The keepalive was resumed after being paused by the system. */ + public void onResumed() {} /** The keepalive was successfully stopped. */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public void onStopped() {} + /** The keepalive was paused automatically by the system. */ + public void onPaused() {} /** An error occurred. */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public void onError(int error) {} @@ -2313,6 +2317,18 @@ public class ConnectivityManager { } } + @Override + public void onResumed() { + final long token = Binder.clearCallingIdentity(); + try { + mExecutor.execute(() -> { + callback.onResumed(); + }); + } finally { + Binder.restoreCallingIdentity(token); + } + } + @Override public void onStopped() { final long token = Binder.clearCallingIdentity(); @@ -2326,6 +2342,19 @@ public class ConnectivityManager { mExecutor.shutdown(); } + @Override + public void onPaused() { + final long token = Binder.clearCallingIdentity(); + try { + mExecutor.execute(() -> { + callback.onPaused(); + }); + } finally { + Binder.restoreCallingIdentity(token); + } + mExecutor.shutdown(); + } + @Override public void onError(int error) { final long token = Binder.clearCallingIdentity(); diff --git a/framework/src/android/net/ISocketKeepaliveCallback.aidl b/framework/src/android/net/ISocketKeepaliveCallback.aidl index 1240e37b94..0a1de6c13f 100644 --- a/framework/src/android/net/ISocketKeepaliveCallback.aidl +++ b/framework/src/android/net/ISocketKeepaliveCallback.aidl @@ -31,4 +31,8 @@ oneway interface ISocketKeepaliveCallback void onError(int error); /** The keepalive on a TCP socket was stopped because the socket received data. */ void onDataReceived(); + /** The keepalive was paused by the system because it's not necessary right now. */ + void onPaused(); + /** The keepalive was resumed by the system after being suspended. */ + void onResumed(); } diff --git a/framework/src/android/net/SocketKeepalive.java b/framework/src/android/net/SocketKeepalive.java index 2911ce7afb..00104f6432 100644 --- a/framework/src/android/net/SocketKeepalive.java +++ b/framework/src/android/net/SocketKeepalive.java @@ -63,6 +63,12 @@ public abstract class SocketKeepalive implements AutoCloseable { @SystemApi public static final int SUCCESS = 0; + /** + * Success when trying to suspend. + * @hide + */ + public static final int SUCCESS_PAUSED = 1; + /** * No keepalive. This should only be internally as it indicates There is no keepalive. * It should not propagate to applications. @@ -270,6 +276,18 @@ public abstract class SocketKeepalive implements AutoCloseable { } } + @Override + public void onResumed() { + final long token = Binder.clearCallingIdentity(); + try { + mExecutor.execute(() -> { + callback.onResumed(); + }); + } finally { + Binder.restoreCallingIdentity(token); + } + } + @Override public void onStopped() { final long token = Binder.clearCallingIdentity(); @@ -282,6 +300,18 @@ public abstract class SocketKeepalive implements AutoCloseable { } } + @Override + public void onPaused() { + final long token = Binder.clearCallingIdentity(); + try { + executor.execute(() -> { + callback.onPaused(); + }); + } finally { + Binder.restoreCallingIdentity(token); + } + } + @Override public void onError(int error) { final long token = Binder.clearCallingIdentity(); @@ -387,8 +417,18 @@ public abstract class SocketKeepalive implements AutoCloseable { public static class Callback { /** The requested keepalive was successfully started. */ public void onStarted() {} + /** + * The keepalive was resumed by the system after being suspended. + * @hide + **/ + public void onResumed() {} /** The keepalive was successfully stopped. */ public void onStopped() {} + /** + * The keepalive was paused by the system because it's not necessary right now. + * @hide + **/ + public void onPaused() {} /** An error occurred. */ public void onError(@ErrorCode int error) {} /** The keepalive on a TCP socket was stopped because the socket received data. This is diff --git a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java index 9dcbac8133..937c93b0ef 100644 --- a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java @@ -18,7 +18,7 @@ package com.android.server.connectivity; import static android.net.NetworkAgent.CMD_START_SOCKET_KEEPALIVE; import static android.net.SocketKeepalive.ERROR_INVALID_SOCKET; -import static android.net.SocketKeepalive.SUCCESS; +import static android.net.SocketKeepalive.SUCCESS_PAUSED; import static android.provider.DeviceConfig.NAMESPACE_CONNECTIVITY; import static android.system.OsConstants.AF_INET; import static android.system.OsConstants.AF_INET6; @@ -180,7 +180,7 @@ public class AutomaticOnOffKeepaliveTracker { * resumed if a TCP socket is open on the VPN. * See the documentation for the states for more detail. */ - public class AutomaticOnOffKeepalive { + public class AutomaticOnOffKeepalive implements IBinder.DeathRecipient { @NonNull private final KeepaliveTracker.KeepaliveInfo mKi; @NonNull @@ -237,6 +237,11 @@ public class AutomaticOnOffKeepaliveTracker { return BinderUtils.withCleanCallingIdentity(() -> PendingIntent.getBroadcast( context, 0 /* requestCode */, intent, PendingIntent.FLAG_IMMUTABLE)); } + + @Override + public void binderDied() { + mConnectivityServiceHandler.post(() -> cleanupAutoOnOffKeepalive(this)); + } } public AutomaticOnOffKeepaliveTracker(@NonNull Context context, @NonNull Handler handler) { @@ -295,7 +300,7 @@ public class AutomaticOnOffKeepaliveTracker { // SUSPENDED. if (ki.mAutomaticOnOffState == STATE_ENABLED) { ki.mAutomaticOnOffState = STATE_SUSPENDED; - handleSuspendKeepalive(ki.mKi); + handlePauseKeepalive(ki.mKi); } } else { handleMaybeResumeKeepalive(ki); @@ -372,6 +377,13 @@ public class AutomaticOnOffKeepaliveTracker { mKeepaliveTracker.handleStartKeepalive(autoKi.mKi); // Add automatic on/off request into list to track its life cycle. + try { + autoKi.mKi.mCallback.asBinder().linkToDeath(autoKi, 0); + } catch (RemoteException e) { + // The underlying keepalive performs its own cleanup + autoKi.binderDied(); + return; + } mAutomaticOnOffKeepalives.add(autoKi); if (STATE_ALWAYS_ON != autoKi.mAutomaticOnOffState) { startTcpPollingAlarm(autoKi.mTcpPollingAlarm); @@ -382,10 +394,9 @@ public class AutomaticOnOffKeepaliveTracker { mKeepaliveTracker.handleStartKeepalive(ki); } - private void handleSuspendKeepalive(@NonNull final KeepaliveTracker.KeepaliveInfo ki) { + private void handlePauseKeepalive(@NonNull final KeepaliveTracker.KeepaliveInfo ki) { // TODO : mKT.handleStopKeepalive should take a KeepaliveInfo instead - // TODO : create a separate success code for suspend - mKeepaliveTracker.handleStopKeepalive(ki.getNai(), ki.getSlot(), SUCCESS); + mKeepaliveTracker.handleStopKeepalive(ki.getNai(), ki.getSlot(), SUCCESS_PAUSED); } /** @@ -397,6 +408,8 @@ public class AutomaticOnOffKeepaliveTracker { if (autoKi.mAutomaticOnOffState != STATE_SUSPENDED) { final KeepaliveTracker.KeepaliveInfo ki = autoKi.mKi; mKeepaliveTracker.handleStopKeepalive(ki.getNai(), ki.getSlot(), reason); + } else { + mKeepaliveTracker.finalizePausedKeepalive(autoKi.mKi); } cleanupAutoOnOffKeepalive(autoKi); @@ -404,10 +417,17 @@ public class AutomaticOnOffKeepaliveTracker { private void cleanupAutoOnOffKeepalive(@NonNull final AutomaticOnOffKeepalive autoKi) { ensureRunningOnHandlerThread(); - if (null != autoKi.mTcpPollingAlarm) mAlarmManager.cancel(autoKi.mTcpPollingAlarm); - // Close the duplicated fd that maintains the lifecycle of socket. + // Close the duplicated fd that maintains the lifecycle of socket. If this fd was + // not duplicated this is a no-op. FileUtils.closeQuietly(autoKi.mFd); - mAutomaticOnOffKeepalives.remove(autoKi); + if (null != autoKi.mTcpPollingAlarm) mAlarmManager.cancel(autoKi.mTcpPollingAlarm); + + // If the KI is not in the array, it's because it was already removed, or it was never + // added ; the only ways this can happen is if the keepalive is stopped by the app and the + // app dies immediately, or if the app died before the link to death could be registered. + if (!mAutomaticOnOffKeepalives.remove(autoKi)) return; + + autoKi.mKi.mCallback.asBinder().unlinkToDeath(autoKi, 0); } /** diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java index a512b7cc79..5572361d0f 100644 --- a/service/src/com/android/server/connectivity/KeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/KeepaliveTracker.java @@ -32,6 +32,7 @@ import static android.net.SocketKeepalive.MAX_INTERVAL_SEC; import static android.net.SocketKeepalive.MIN_INTERVAL_SEC; import static android.net.SocketKeepalive.NO_KEEPALIVE; import static android.net.SocketKeepalive.SUCCESS; +import static android.net.SocketKeepalive.SUCCESS_PAUSED; import android.annotation.NonNull; import android.annotation.Nullable; @@ -134,6 +135,9 @@ public class KeepaliveTracker { public final NetworkAgentInfo mNai; private final int mType; public final FileDescriptor mFd; + // True if this was resumed from a previously turned off keepalive, otherwise false. + // This is necessary to send the correct callbacks. + public final boolean mResumed; public static final int TYPE_NATT = 1; public static final int TYPE_TCP = 2; @@ -160,6 +164,16 @@ public class KeepaliveTracker { int interval, int type, @Nullable FileDescriptor fd) throws InvalidSocketException { + this(callback, nai, packet, interval, type, fd, false /* resumed */); + } + + KeepaliveInfo(@NonNull ISocketKeepaliveCallback callback, + @NonNull NetworkAgentInfo nai, + @NonNull KeepalivePacketData packet, + int interval, + int type, + @Nullable FileDescriptor fd, + boolean resumed) throws InvalidSocketException { mCallback = callback; mPid = Binder.getCallingPid(); mUid = Binder.getCallingUid(); @@ -169,6 +183,7 @@ public class KeepaliveTracker { mPacket = packet; mInterval = interval; mType = type; + mResumed = resumed; // For SocketKeepalive, a dup of fd is kept in mFd so the source port from which the // keepalives are sent cannot be reused by another app even if the fd gets closed by @@ -232,6 +247,8 @@ public class KeepaliveTracker { /** Called when the application process is killed. */ public void binderDied() { + // TODO b/267106526 : this is not called on the handler thread but stop() happily + // assumes it is, which means this is a pretty dangerous race condition. stop(BINDER_DIED); } @@ -327,6 +344,10 @@ public class KeepaliveTracker { } void start(int slot) { + // BINDER_DIED can happen if the binder died before the KeepaliveInfo was created and + // the constructor set the state to BINDER_DIED. If that's the case, the KI is already + // cleaned up. + if (BINDER_DIED == mStartedState) return; mSlot = slot; int error = isValid(); if (error == SUCCESS) { @@ -371,7 +392,10 @@ public class KeepaliveTracker { // To prevent races from re-entrance of stop(), return if the state is already stopping. // This might happen if multiple event sources stop keepalive in a short time. Such as // network disconnect after user calls stop(), or tear down socket after binder died. - if (mStartedState == STOPPING) return; + // Note that it's always possible this method is called by the auto keepalive timer + // or any other way after the binder died, hence the check for BINDER_DIED. If the + // binder has died, then the KI has already been cleaned up. + if (mStartedState == STOPPING || mStartedState == BINDER_DIED) return; // Store the reason of stopping, and report it after the keepalive is fully stopped. if (mStopReason != ERROR_STOP_REASON_UNINITIALIZED) { @@ -382,9 +406,10 @@ public class KeepaliveTracker { + ": " + reason); switch (mStartedState) { case NOT_STARTED: - // Remove the reference of the keepalive that meet error before starting, + // Remove the reference to this keepalive that had an error before starting, // e.g. invalid parameter. cleanupStoppedKeepalive(mNai, mSlot); + if (BINDER_DIED == reason) mStartedState = BINDER_DIED; break; default: mStartedState = STOPPING; @@ -422,7 +447,8 @@ public class KeepaliveTracker { * Construct a new KeepaliveInfo from existing KeepaliveInfo with a new fd. */ public KeepaliveInfo withFd(@NonNull FileDescriptor fd) throws InvalidSocketException { - return new KeepaliveInfo(mCallback, mNai, mPacket, mInterval, mType, fd); + return new KeepaliveInfo(mCallback, mNai, mPacket, mInterval, mType, fd, + true /* resumed */); } } @@ -523,6 +549,12 @@ public class KeepaliveTracker { } catch (RemoteException e) { Log.w(TAG, "Discarded onStop callback: " + reason); } + } else if (reason == SUCCESS_PAUSED) { + try { + ki.mCallback.onPaused(); + } catch (RemoteException e) { + Log.w(TAG, "Discarded onPaused callback: " + reason); + } } else if (reason == DATA_RECEIVED) { try { ki.mCallback.onDataReceived(); @@ -540,6 +572,25 @@ public class KeepaliveTracker { ki.unlinkDeathRecipient(); } + /** + * Finalize a paused keepalive. + * + * This will simply send the onStopped() callback after checking that this keepalive is + * indeed paused. + * + * @param ki the keepalive to finalize + */ + public void finalizePausedKeepalive(@NonNull final KeepaliveInfo ki) { + if (SUCCESS_PAUSED != ki.mStopReason) { + throw new IllegalStateException("Keepalive is not paused"); + } + try { + ki.mCallback.onStopped(); + } catch (RemoteException e) { + Log.w(TAG, "Discarded onStopped callback while finalizing paused keepalive"); + } + } + public void handleCheckKeepalivesStillValid(NetworkAgentInfo nai) { HashMap networkKeepalives = mKeepalives.get(nai); if (networkKeepalives != null) { @@ -589,9 +640,14 @@ public class KeepaliveTracker { Log.d(TAG, "Started keepalive " + slot + " on " + nai.toShortString()); ki.mStartedState = KeepaliveInfo.STARTED; try { - ki.mCallback.onStarted(); + if (ki.mResumed) { + ki.mCallback.onResumed(); + } else { + ki.mCallback.onStarted(); + } } catch (RemoteException e) { - Log.w(TAG, "Discarded onStarted callback"); + Log.w(TAG, "Discarded " + (ki.mResumed ? "onResumed" : "onStarted") + + " callback for slot " + slot); } } else { Log.d(TAG, "Failed to start keepalive " + slot + " on " + nai.toShortString()