diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java index 941b61686b..1fd8a620fb 100644 --- a/service/src/com/android/server/connectivity/KeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/KeepaliveTracker.java @@ -106,7 +106,12 @@ public class KeepaliveTracker { private final int mAllowedUnprivilegedSlotsForUid; public KeepaliveTracker(Context context, Handler handler) { - mTcpController = new TcpKeepaliveController(handler); + this(context, handler, new TcpKeepaliveController(handler)); + } + + @VisibleForTesting + KeepaliveTracker(Context context, Handler handler, TcpKeepaliveController tcpController) { + mTcpController = tcpController; mContext = context; mSupportedKeepalives = KeepaliveResourceUtil.getSupportedKeepalives(context); @@ -375,7 +380,7 @@ public class KeepaliveTracker { break; case TYPE_TCP: try { - mTcpController.startSocketMonitor(mFd, this, mSlot); + mTcpController.startSocketMonitor(mFd, mCallback, mSlot); } catch (InvalidSocketException e) { handleStopKeepalive(mNai, mSlot, ERROR_INVALID_SOCKET); return ERROR_INVALID_SOCKET; @@ -455,12 +460,6 @@ public class KeepaliveTracker { } } - // TODO: This does not clean up the autoKi in AutomaticOnOffKeepaliveTracker and it is not - // possible without a big refactor. - void onFileDescriptorInitiatedStop(final int socketKeepaliveReason) { - handleStopKeepalive(mNai, mSlot, socketKeepaliveReason); - } - /** * Construct a new KeepaliveInfo from existing KeepaliveInfo with a new fd. */ diff --git a/service/src/com/android/server/connectivity/TcpKeepaliveController.java b/service/src/com/android/server/connectivity/TcpKeepaliveController.java index a9cb2fa91d..0fd8604363 100644 --- a/service/src/com/android/server/connectivity/TcpKeepaliveController.java +++ b/service/src/com/android/server/connectivity/TcpKeepaliveController.java @@ -15,6 +15,7 @@ */ package com.android.server.connectivity; +import static android.net.NetworkAgent.CMD_STOP_SOCKET_KEEPALIVE; import static android.net.SocketKeepalive.DATA_RECEIVED; import static android.net.SocketKeepalive.ERROR_INVALID_IP_ADDRESS; import static android.net.SocketKeepalive.ERROR_INVALID_SOCKET; @@ -33,6 +34,7 @@ import static android.system.OsConstants.TIOCOUTQ; import static com.android.net.module.util.NetworkStackConstants.IPV4_HEADER_MIN_LEN; import android.annotation.NonNull; +import android.net.ISocketKeepaliveCallback; import android.net.InvalidPacketException; import android.net.NetworkUtils; import android.net.SocketKeepalive.InvalidSocketException; @@ -50,7 +52,6 @@ import android.util.SparseArray; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.net.module.util.IpUtils; -import com.android.server.connectivity.KeepaliveTracker.KeepaliveInfo; import java.io.FileDescriptor; import java.net.InetAddress; @@ -88,6 +89,8 @@ public class TcpKeepaliveController { private final MessageQueue mFdHandlerQueue; + private final Handler mConnectivityServiceHandler; + private static final int FD_EVENTS = EVENT_INPUT | EVENT_ERROR; private static final int TCP_HEADER_LENGTH = 20; @@ -115,6 +118,7 @@ public class TcpKeepaliveController { public TcpKeepaliveController(final Handler connectivityServiceHandler) { mFdHandlerQueue = connectivityServiceHandler.getLooper().getQueue(); + mConnectivityServiceHandler = connectivityServiceHandler; } /** Build tcp keepalive packet. */ @@ -324,12 +328,13 @@ public class TcpKeepaliveController { * Start monitoring incoming packets. * * @param fd socket fd to monitor. - * @param ki a {@link KeepaliveInfo} that tracks information about a socket keepalive. + * @param callback a {@link ISocketKeepaliveCallback} that tracks information about a socket + * keepalive. * @param slot keepalive slot. */ - public void startSocketMonitor(@NonNull final FileDescriptor fd, - @NonNull final KeepaliveInfo ki, final int slot) - throws IllegalArgumentException, InvalidSocketException { + public void startSocketMonitor( + @NonNull final FileDescriptor fd, @NonNull final ISocketKeepaliveCallback callback, + final int slot) throws IllegalArgumentException, InvalidSocketException { synchronized (mListeners) { if (null != mListeners.get(slot)) { throw new IllegalArgumentException("This slot is already taken"); @@ -350,7 +355,8 @@ public class TcpKeepaliveController { } else { reason = DATA_RECEIVED; } - ki.onFileDescriptorInitiatedStop(reason); + mConnectivityServiceHandler.obtainMessage(CMD_STOP_SOCKET_KEEPALIVE, + 0 /* unused */, reason, callback.asBinder()).sendToTarget(); // The listener returns the new set of events to listen to. Because 0 means no // event, the listener gets unregistered. return 0; diff --git a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java index 608e6d8647..db65c2b997 100644 --- a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java +++ b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java @@ -18,6 +18,7 @@ package com.android.server.connectivity; import static android.content.pm.PackageManager.PERMISSION_GRANTED; import static android.net.ConnectivityManager.TYPE_MOBILE; +import static android.net.NetworkAgent.CMD_STOP_SOCKET_KEEPALIVE; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static com.android.testutils.HandlerUtils.visibleOnHandlerThread; @@ -28,6 +29,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; @@ -47,6 +49,7 @@ import android.content.Context; import android.content.res.Resources; import android.net.INetd; import android.net.ISocketKeepaliveCallback; +import android.net.KeepalivePacketData; import android.net.LinkAddress; import android.net.LinkProperties; import android.net.MarkMaskParcel; @@ -55,6 +58,7 @@ import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkInfo; import android.net.SocketKeepalive; +import android.net.TcpKeepalivePacketData; import android.os.Binder; import android.os.Build; import android.os.Handler; @@ -120,6 +124,7 @@ public class AutomaticOnOffKeepaliveTrackerTest { TestKeepaliveTracker mKeepaliveTracker; AOOTestHandler mTestHandler; + TestTcpKeepaliveController mTcpController; // Hexadecimal representation of a SOCK_DIAG response with tcp info. private static final String SOCK_DIAG_TCP_INET_HEX = @@ -233,9 +238,9 @@ public class AutomaticOnOffKeepaliveTrackerTest { public final FileDescriptor fd; public final ISocketKeepaliveCallback socketKeepaliveCallback; public final Network underpinnedNetwork; - public final NattKeepalivePacketData kpd; + public final KeepalivePacketData kpd; - TestKeepaliveInfo(NattKeepalivePacketData kpd) throws Exception { + TestKeepaliveInfo(KeepalivePacketData kpd) throws Exception { this.kpd = kpd; socket = new Socket(); socket.bind(null); @@ -252,8 +257,9 @@ public class AutomaticOnOffKeepaliveTrackerTest { private class TestKeepaliveTracker extends KeepaliveTracker { private KeepaliveInfo mKi; - TestKeepaliveTracker(@NonNull final Context context, @NonNull final Handler handler) { - super(context, handler); + TestKeepaliveTracker(@NonNull final Context context, @NonNull final Handler handler, + @NonNull final TcpKeepaliveController tcpController) { + super(context, handler, tcpController); } public void setReturnedKeepaliveInfo(@NonNull final KeepaliveInfo ki) { @@ -272,6 +278,24 @@ public class AutomaticOnOffKeepaliveTrackerTest { } return mKi; } + + @NonNull + @Override + public KeepaliveInfo makeTcpKeepaliveInfo(@Nullable final NetworkAgentInfo nai, + @Nullable final FileDescriptor fd, final int intervalSeconds, + @NonNull final ISocketKeepaliveCallback cb) { + if (null == mKi) { + throw new IllegalStateException("Please call `setReturnedKeepaliveInfo`" + + " before makeTcpKeepaliveInfo is called"); + } + return mKi; + } + } + + private static class TestTcpKeepaliveController extends TcpKeepaliveController { + TestTcpKeepaliveController(final Handler connectivityServiceHandler) { + super(connectivityServiceHandler); + } } @Before @@ -303,7 +327,8 @@ public class AutomaticOnOffKeepaliveTrackerTest { mHandlerThread = new HandlerThread("KeepaliveTrackerTest"); mHandlerThread.start(); mTestHandler = new AOOTestHandler(mHandlerThread.getLooper()); - mKeepaliveTracker = new TestKeepaliveTracker(mCtx, mTestHandler); + mTcpController = new TestTcpKeepaliveController(mTestHandler); + mKeepaliveTracker = new TestKeepaliveTracker(mCtx, mTestHandler, mTcpController); doReturn(mKeepaliveTracker).when(mDependencies).newKeepaliveTracker(mCtx, mTestHandler); doReturn(true).when(mDependencies).isFeatureEnabled(any(), anyBoolean()); mAOOKeepaliveTracker = @@ -333,6 +358,14 @@ public class AutomaticOnOffKeepaliveTrackerTest { Log.d(TAG, "Test handler received CMD_MONITOR_AUTOMATIC_KEEPALIVE : " + msg); mLastAutoKi = mAOOKeepaliveTracker.getKeepaliveForBinder((IBinder) msg.obj); break; + case CMD_STOP_SOCKET_KEEPALIVE: + Log.d(TAG, "Test handler received CMD_STOP_SOCKET_KEEPALIVE : " + msg); + mLastAutoKi = mAOOKeepaliveTracker.getKeepaliveForBinder((IBinder) msg.obj); + if (mLastAutoKi == null) { + fail("Attempt to stop an already stopped keepalive"); + } + mAOOKeepaliveTracker.handleStopKeepalive(mLastAutoKi, msg.arg2); + break; } } } @@ -481,14 +514,15 @@ public class AutomaticOnOffKeepaliveTrackerTest { mTestHandler, () -> mAOOKeepaliveTracker.getKeepaliveForBinder(binder)); } - private void checkAndProcessKeepaliveStart(final NattKeepalivePacketData kpd) throws Exception { + private void checkAndProcessKeepaliveStart(final KeepalivePacketData kpd) throws Exception { checkAndProcessKeepaliveStart(TEST_SLOT, kpd); } private void checkAndProcessKeepaliveStart( - int slot, final NattKeepalivePacketData kpd) throws Exception { - verify(mNai).onStartNattSocketKeepalive(slot, TEST_KEEPALIVE_INTERVAL_SEC, kpd); - verify(mNai).onAddNattKeepalivePacketFilter(slot, kpd); + int slot, final KeepalivePacketData kpd) throws Exception { + verify(mNai).onStartNattSocketKeepalive( + slot, TEST_KEEPALIVE_INTERVAL_SEC, (NattKeepalivePacketData) kpd); + verify(mNai).onAddNattKeepalivePacketFilter(slot, (NattKeepalivePacketData) kpd); triggerEventKeepalive(slot, SocketKeepalive.SUCCESS); } @@ -531,9 +565,10 @@ public class AutomaticOnOffKeepaliveTrackerTest { public void testHandleEventSocketKeepalive_startingFailureHardwareError() throws Exception { final TestKeepaliveInfo testInfo = doStartNattKeepalive(); - verify(mNai) - .onStartNattSocketKeepalive(TEST_SLOT, TEST_KEEPALIVE_INTERVAL_SEC, testInfo.kpd); - verify(mNai).onAddNattKeepalivePacketFilter(TEST_SLOT, testInfo.kpd); + verify(mNai).onStartNattSocketKeepalive( + TEST_SLOT, TEST_KEEPALIVE_INTERVAL_SEC, (NattKeepalivePacketData) testInfo.kpd); + verify(mNai).onAddNattKeepalivePacketFilter( + TEST_SLOT, (NattKeepalivePacketData) testInfo.kpd); // Network agent returns an error, fails to start the keepalive. triggerEventKeepalive(TEST_SLOT, SocketKeepalive.ERROR_HARDWARE_ERROR); @@ -674,9 +709,10 @@ public class AutomaticOnOffKeepaliveTrackerTest { clearInvocations(mNai); doResumeKeepalive(getAutoKiForBinder(testInfo.binder)); - verify(mNai) - .onStartNattSocketKeepalive(TEST_SLOT, TEST_KEEPALIVE_INTERVAL_SEC, testInfo.kpd); - verify(mNai).onAddNattKeepalivePacketFilter(TEST_SLOT, testInfo.kpd); + verify(mNai).onStartNattSocketKeepalive( + TEST_SLOT, TEST_KEEPALIVE_INTERVAL_SEC, (NattKeepalivePacketData) testInfo.kpd); + verify(mNai).onAddNattKeepalivePacketFilter( + TEST_SLOT, (NattKeepalivePacketData) testInfo.kpd); // Network agent returns error on starting the keepalive. triggerEventKeepalive(TEST_SLOT, SocketKeepalive.ERROR_HARDWARE_ERROR); @@ -772,4 +808,38 @@ public class AutomaticOnOffKeepaliveTrackerTest { verifyNoMoreInteractions(ignoreStubs(testInfo1.socketKeepaliveCallback)); verifyNoMoreInteractions(ignoreStubs(testInfo2.socketKeepaliveCallback)); } + + @Test + public void testStartTcpKeepalive_fdInitiatedStop() throws Exception { + final InetAddress srcAddress = InetAddress.getByAddress( + new byte[] { (byte) 192, 0, 0, (byte) 129 }); + mNai.linkProperties.addLinkAddress(new LinkAddress(srcAddress, 24)); + + final KeepalivePacketData kpd = new TcpKeepalivePacketData( + InetAddress.getByAddress(new byte[] { (byte) 192, 0, 0, (byte) 129 }) /* srcAddr */, + 12345 /* srcPort */, + InetAddress.getByAddress(new byte[] { 8, 8, 8, 8}) /* dstAddr */, + 12345 /* dstPort */, new byte[] {1}, 111 /* tcpSeq */, + 222 /* tcpAck */, 800 /* tcpWindow */, 2 /* tcpWindowScale */, + 4 /* ipTos */, 64 /* ipTtl */); + final TestKeepaliveInfo testInfo = new TestKeepaliveInfo(kpd); + + final KeepaliveInfo ki = mKeepaliveTracker.new KeepaliveInfo( + testInfo.socketKeepaliveCallback, mNai, kpd, + TEST_KEEPALIVE_INTERVAL_SEC, KeepaliveInfo.TYPE_TCP, testInfo.fd); + mKeepaliveTracker.setReturnedKeepaliveInfo(ki); + + // Setup TCP keepalive. + mAOOKeepaliveTracker.startTcpKeepalive(mNai, testInfo.fd, TEST_KEEPALIVE_INTERVAL_SEC, + testInfo.socketKeepaliveCallback); + HandlerUtils.waitForIdle(mTestHandler, TIMEOUT_MS); + + // A closed socket will result in EVENT_HANGUP and trigger error to + // FileDescriptorEventListener. + testInfo.socket.close(); + HandlerUtils.waitForIdle(mTestHandler, TIMEOUT_MS); + + // The keepalive should be removed in AutomaticOnOffKeepaliveTracker. + getAutoKiForBinder(testInfo.binder); + } }