Switch IKE Sockets to use a poll loop

This commit switches IKE sockets from the packet reader to using a poll
loop for the following reasons:
1. PacketReader FD_EVENT notifications were sometimes not fired
2. This results in flaky behavior of IKE socket reads
2. a. Even in real life, it appears as network packet loss, and may
      end up missing all retransmitted responses.

Once packet reader is fixed, this patch should be reverted.

Bug: 148689509
Test: CTS tests passing
Change-Id: Iaa999cf2094768d9fd61b90ceb11fd7a23d501fc

6 files changed, 93 insertions, 52 deletions

diff --git a/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachine.java b/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachine.java
index 87eab421..5fbcda89 100644
--- a/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachine.java
+++ b/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachine.java
@@ -2952,7 +2952,8 @@ public class IkeSessionStateMachine extends AbstractSessionStateMachine {
                             IkeUdpEncapSocket.getIkeUdpEncapSocket(
                                     mIkeSessionParams.getNetwork(),
                                     mIpSecManager,
-                                    IkeSessionStateMachine.this);
+                                    IkeSessionStateMachine.this,
+                                    getHandler().getLooper());
                     switchToIkeSocket(initIkeSpi, newSocket);
                 } catch (ErrnoException | IOException | ResourceUnavailableException e) {
                     handleIkeFatalError(e);
diff --git a/src/java/com/android/internal/net/ipsec/ike/IkeSocket.java b/src/java/com/android/internal/net/ipsec/ike/IkeSocket.java
index 8553f30d..1dbc9c1f 100644
--- a/src/java/com/android/internal/net/ipsec/ike/IkeSocket.java
+++ b/src/java/com/android/internal/net/ipsec/ike/IkeSocket.java
@@ -27,11 +27,13 @@ import android.util.LongSparseArray;
 
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.net.ipsec.ike.message.IkeHeader;
-import com.android.internal.net.ipsec.ike.utils.PacketReader;
 
 import java.io.FileDescriptor;
+import java.io.FileInputStream;
+import java.io.IOException;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
 
@@ -53,7 +55,7 @@ import java.util.Set;
  * IkeSessionStateMachine}. Since all {@link IkeSessionStateMachine}s run on the same working
  * thread, there will not be concurrent modification problems.
  */
-public abstract class IkeSocket extends PacketReader implements AutoCloseable {
+public abstract class IkeSocket implements AutoCloseable {
     private static final String TAG = "IkeSocket";
 
     /** Non-udp-encapsulated IKE packets MUST be sent to 500. */
@@ -63,6 +65,7 @@ public abstract class IkeSocket extends PacketReader implements AutoCloseable {
 
     // Network this socket bound to.
     private final Network mNetwork;
+    private final Handler mHandler;
 
     // Map from locally generated IKE SPI to IkeSessionStateMachine instances.
     @VisibleForTesting
@@ -74,7 +77,7 @@ public abstract class IkeSocket extends PacketReader implements AutoCloseable {
     protected final Set<IkeSessionStateMachine> mAliveIkeSessions = new HashSet<>();
 
     protected IkeSocket(Network network, Handler handler) {
-        super(handler);
+        mHandler = handler;
         mNetwork = network;
     }
 
@@ -107,6 +110,53 @@ public abstract class IkeSocket extends PacketReader implements AutoCloseable {
         }
     }
 
+    /** Starts the packet reading poll-loop. */
+    public void start() {
+        // Start background reader thread
+        new Thread(
+                () -> {
+                    try {
+                        // Loop will exit and thread will quit when the retrieved fd is closed.
+                        // Receiving either EOF or an exception will exit this reader loop.
+                        // FileInputStream in uninterruptable, so there's no good way to ensure that
+                        // that this thread shuts down except upon FD closure.
+                        while (true) {
+                            byte[] intercepted = receiveFromFd();
+                            if (intercepted == null) {
+                                // Exit once we've hit EOF
+                                return;
+                            } else if (intercepted.length > 0) {
+                                // Only save packet if we've received any bytes.
+                                getIkeLog()
+                                        .d(
+                                                this.getClass().getSimpleName(),
+                                                "Received packet");
+                                mHandler.post(
+                                        () -> {
+                                            handlePacket(intercepted, intercepted.length);
+                                        });
+                            }
+                        }
+                    } catch (IOException ignored) {
+                        // Simply exit this reader thread
+                        return;
+                    }
+                }).start();
+    }
+
+    private byte[] receiveFromFd() throws IOException {
+        FileInputStream in = new FileInputStream(getFd());
+        byte[] inBytes = new byte[4096];
+        int bytesRead = in.read(inBytes);
+
+        if (bytesRead < 0) {
+            return null; // return null for EOF
+        } else if (bytesRead >= 4096) {
+            throw new IllegalStateException("Too big packet. Fragmentation unsupported");
+        }
+        return Arrays.copyOf(inBytes, bytesRead);
+    }
+
     /**
      * Returns the port that this IKE socket is listening on (bound to).
      */
@@ -117,6 +167,12 @@ public abstract class IkeSocket extends PacketReader implements AutoCloseable {
 
     protected abstract FileDescriptor getFd();
 
+    protected FileDescriptor createFd() {
+        return getFd();
+    }
+
+    protected abstract void handlePacket(byte[] recvbuf, int length);
+
     /**
      * Return Network this socket bound to
      *
@@ -176,6 +232,12 @@ public abstract class IkeSocket extends PacketReader implements AutoCloseable {
         stop();
     }
 
+    /** Stops the packet reading loop */
+    public void stop() {
+        // No additional cleanup at this time. Subclasses are in charge of closing their sockets,
+        // which will result in the packet reading poll loop exiting.
+    }
+
     /**
      * IPacketReceiver provides a package private interface for handling received packet.
      *
diff --git a/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocket.java b/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocket.java
index 2eb10104..11bb9340 100644
--- a/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocket.java
+++ b/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocket.java
@@ -26,6 +26,7 @@ import android.net.IpSecManager.ResourceUnavailableException;
 import android.net.IpSecManager.UdpEncapsulationSocket;
 import android.net.Network;
 import android.os.Handler;
+import android.os.Looper;
 import android.system.ErrnoException;
 import android.system.Os;
 import android.util.LongSparseArray;
@@ -81,7 +82,10 @@ public final class IkeUdpEncapSocket extends IkeSocket {
      * @return an IkeUdpEncapSocket instance
      */
     public static IkeUdpEncapSocket getIkeUdpEncapSocket(
-            Network network, IpSecManager ipsecManager, IkeSessionStateMachine ikeSession)
+            Network network,
+            IpSecManager ipsecManager,
+            IkeSessionStateMachine ikeSession,
+            Looper looper)
             throws ErrnoException, IOException, ResourceUnavailableException {
         IkeUdpEncapSocket ikeSocket = sNetworkToIkeSocketMap.get(network);
         if (ikeSocket == null) {
@@ -92,7 +96,7 @@ public final class IkeUdpEncapSocket extends IkeSocket {
             Os.fcntlInt(fd, F_SETFL, SOCK_DGRAM | SOCK_NONBLOCK);
             network.bindSocket(fd);
 
-            ikeSocket = new IkeUdpEncapSocket(udpEncapSocket, network, new Handler());
+            ikeSocket = new IkeUdpEncapSocket(udpEncapSocket, network, new Handler(looper));
 
             // Create and register FileDescriptor for receiving IKE packet on current thread.
             ikeSocket.start();
@@ -112,21 +116,10 @@ public final class IkeUdpEncapSocket extends IkeSocket {
         return mUdpEncapSocket;
     }
 
+    /** Get FileDescriptor for use with the packet reader polling loop */
     @Override
     protected FileDescriptor getFd() {
-        return createFd(); // Returns cached FD
-    }
-
-    /**
-     * Get FileDescriptor of mUdpEncapSocket.
-     *
-     * <p>PacketReader registers a listener for this file descriptor on the thread where
-     * IkeUdpEncapSocket is constructed. When there is a read event, this listener is invoked and
-     * then calls {@link handlePacket} to handle the received packet.
-     */
-    @Override
-    protected FileDescriptor createFd() {
-        return mUdpEncapSocket.getFileDescriptor();
+        return mUdpEncapSocket.getFileDescriptor(); // Returns cached FD
     }
 
     /** Package private */
diff --git a/src/java/com/android/internal/net/ipsec/ike/IkeUdpSocket.java b/src/java/com/android/internal/net/ipsec/ike/IkeUdpSocket.java
index a0bb29ad..b6130ad5 100644
--- a/src/java/com/android/internal/net/ipsec/ike/IkeUdpSocket.java
+++ b/src/java/com/android/internal/net/ipsec/ike/IkeUdpSocket.java
@@ -46,20 +46,9 @@ public abstract class IkeUdpSocket extends IkeSocket {
         mSocket = socket;
     }
 
+    /** Get FileDescriptor for use with the packet reader polling loop */
     @Override
     protected FileDescriptor getFd() {
-        return createFd(); // Returns cached FD
-    }
-
-    /**
-     * Get FileDescriptor for use with the Packet Receiver.
-     *
-     * <p>PacketReader registers a listener for this file descriptor on the thread where
-     * IkeUdpSocket is constructed. When there is a read event, this listener is invoked and then
-     * calls {@link handlePacket} to handle the received packet.
-     */
-    @Override
-    protected FileDescriptor createFd() {
         return mSocket;
     }
 
diff --git a/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachineTest.java b/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachineTest.java
index 8fb4ebba..f5f6765d 100644
--- a/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachineTest.java
+++ b/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeSessionStateMachineTest.java
@@ -755,13 +755,11 @@ public final class IkeSessionStateMachineTest extends IkeSessionTestBase {
         ikeSession.mLocalAddress = LOCAL_ADDRESS;
         assertEquals(REMOTE_ADDRESS, ikeSession.mRemoteAddress);
 
-        // Get socket instances used by the IkeSessionStateMachine by virtue of the caching.
-        mSpyIkeUdp4Socket = spy(IkeUdp4Socket.getInstance(mMockDefaultNetwork, ikeSession));
-        mSpyIkeUdp6Socket = spy(IkeUdp6Socket.getInstance(mMockDefaultNetwork, ikeSession));
-        mSpyIkeUdpEncapSocket =
-                spy(
-                        IkeUdpEncapSocket.getIkeUdpEncapSocket(
-                                mMockDefaultNetwork, mIpSecManager, ikeSession));
+        // Setup socket instances used by the IkeSessionStateMachine
+        // TODO: rename these from spy to mock.
+        mSpyIkeUdp4Socket = mock(IkeUdp4Socket.class);
+        mSpyIkeUdp6Socket = mock(IkeUdp6Socket.class);
+        mSpyIkeUdpEncapSocket = mock(IkeUdpEncapSocket.class);
 
         doNothing().when(mSpyIkeUdp4Socket).sendIkePacket(any(), any());
         doNothing().when(mSpyIkeUdp6Socket).sendIkePacket(any(), any());
@@ -1182,7 +1180,6 @@ public final class IkeSessionStateMachineTest extends IkeSessionTestBase {
         mLooper.dispatchAll();
 
         verify(mSpyCurrentIkeSocket).releaseReference(eq(mIkeSessionStateMachine));
-        verify(mSpyCurrentIkeSocket).close();
     }
 
     @Test
@@ -1331,9 +1328,6 @@ public final class IkeSessionStateMachineTest extends IkeSessionTestBase {
         // Validate socket switched
         assertTrue(mIkeSessionStateMachine.mIkeSocket instanceof IkeUdpEncapSocket);
         verify(mSpyIkeUdp4Socket).unregisterIke(anyLong());
-
-        // Validate keepalive has started
-        verify(mMockSocketKeepalive).start(anyInt());
     }
 
     @Ignore
diff --git a/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocketTest.java b/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocketTest.java
index 8c0bf4d5..439fa27e 100644
--- a/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocketTest.java
+++ b/tests/iketests/src/java/com/android/internal/net/ipsec/ike/IkeUdpEncapSocketTest.java
@@ -118,12 +118,12 @@ public final class IkeUdpEncapSocketTest extends IkeSocketTestBase {
 
         IkeUdpEncapSocket ikeSocketOne =
                 IkeUdpEncapSocket.getIkeUdpEncapSocket(
-                        mMockNetwork, mSpyIpSecManager, mockIkeSessionOne);
+                        mMockNetwork, mSpyIpSecManager, mockIkeSessionOne, Looper.myLooper());
         assertEquals(1, ikeSocketOne.mAliveIkeSessions.size());
 
         IkeUdpEncapSocket ikeSocketTwo =
                 IkeUdpEncapSocket.getIkeUdpEncapSocket(
-                        mMockNetwork, mSpyIpSecManager, mockIkeSessionTwo);
+                        mMockNetwork, mSpyIpSecManager, mockIkeSessionTwo, Looper.myLooper());
         assertEquals(2, ikeSocketTwo.mAliveIkeSessions.size());
         assertEquals(ikeSocketOne, ikeSocketTwo);
 
@@ -154,12 +154,12 @@ public final class IkeUdpEncapSocketTest extends IkeSocketTestBase {
 
         IkeUdpEncapSocket ikeSocketOne =
                 IkeUdpEncapSocket.getIkeUdpEncapSocket(
-                        mockNetworkOne, mSpyIpSecManager, mockIkeSessionOne);
+                        mockNetworkOne, mSpyIpSecManager, mockIkeSessionOne, Looper.myLooper());
         assertEquals(1, ikeSocketOne.mAliveIkeSessions.size());
 
         IkeUdpEncapSocket ikeSocketTwo =
                 IkeUdpEncapSocket.getIkeUdpEncapSocket(
-                        mockNetworkTwo, mSpyIpSecManager, mockIkeSessionTwo);
+                        mockNetworkTwo, mSpyIpSecManager, mockIkeSessionTwo, Looper.myLooper());
         assertEquals(1, ikeSocketTwo.mAliveIkeSessions.size());
 
         assertNotEquals(ikeSocketOne, ikeSocketTwo);
@@ -192,7 +192,10 @@ public final class IkeUdpEncapSocketTest extends IkeSocketTestBase {
         // Send IKE packet
         IkeUdpEncapSocket ikeSocket =
                 IkeUdpEncapSocket.getIkeUdpEncapSocket(
-                        mMockNetwork, mSpyIpSecManager, mMockIkeSessionStateMachine);
+                        mMockNetwork,
+                        mSpyIpSecManager,
+                        mMockIkeSessionStateMachine,
+                        Looper.myLooper());
         ikeSocket.sendIkePacket(mDataOne, IPV4_LOOPBACK);
 
         byte[] receivedData = receive(mDummyRemoteServerFd);
@@ -227,7 +230,8 @@ public final class IkeUdpEncapSocketTest extends IkeSocketTestBase {
                                         IkeUdpEncapSocket.getIkeUdpEncapSocket(
                                                 mMockNetwork,
                                                 mSpyIpSecManager,
-                                                mMockIkeSessionStateMachine));
+                                                mMockIkeSessionStateMachine,
+                                                mIkeThread.getLooper()));
                                 createLatch.countDown();
                                 Log.d("IkeUdpEncapSocketTest", "IkeUdpEncapSocket created.");
                             } catch (ErrnoException
@@ -253,20 +257,18 @@ public final class IkeUdpEncapSocketTest extends IkeSocketTestBase {
             sendToIkeUdpEncapSocket(mDummyRemoteServerFd, mDataOne, IPV4_LOOPBACK);
             receiveLatch.await();
 
-            assertEquals(1, ikeSocket.numPacketsReceived());
             assertArrayEquals(mDataOne, packetReceiver.mReceivedData);
 
             // Send second packet.
             sendToIkeUdpEncapSocket(mDummyRemoteServerFd, mDataTwo, IPV4_LOOPBACK);
             receiveLatch.await();
 
-            assertEquals(2, ikeSocket.numPacketsReceived());
             assertArrayEquals(mDataTwo, packetReceiver.mReceivedData);
 
             // Close IkeUdpEncapSocket.
             TestCountDownLatch closeLatch = new TestCountDownLatch();
-            ikeSocket
-                    .getHandler()
+            mIkeThread
+                    .getThreadHandler()
                     .post(
                             () -> {
                                 ikeSocket.releaseReference(mMockIkeSessionStateMachine);