Refactor the ReceiveThread with PacketReader in DhcpClient.

To implement the IP address conflict detection, we need another
ReceiveThread to listen ARP packets as well. Ideally, we should
listen both of DHCP packets and ARP packets in the same thread
with DhcpClient state machine handler.

This CL replaces the ReceiveThread with PacketReader to listen
DHCP packets. After refactoring, DHCP packets are read from the
same handler thread instead of a separate receiving thread. Then
we can also leverage the same handler thread to listen the ARP
packets simultaneously via another raw socket.

Bug: 130775067
Test: atest NetworkStackTests NetworkStackIntegrationTests

Change-Id: I23e91a6c2d99cf8053e62f2ae9d78481ece6384d

2 files changed, 84 insertions, 84 deletions

diff --git a/src/android/net/dhcp/DhcpClient.java b/src/android/net/dhcp/DhcpClient.java
index bf63c1c..e88c7cc 100644
--- a/src/android/net/dhcp/DhcpClient.java
+++ b/src/android/net/dhcp/DhcpClient.java
@@ -37,6 +37,7 @@ import static android.system.OsConstants.AF_PACKET;
 import static android.system.OsConstants.ETH_P_IP;
 import static android.system.OsConstants.IPPROTO_UDP;
 import static android.system.OsConstants.SOCK_DGRAM;
+import static android.system.OsConstants.SOCK_NONBLOCK;
 import static android.system.OsConstants.SOCK_RAW;
 import static android.system.OsConstants.SOL_SOCKET;
 import static android.system.OsConstants.SO_BROADCAST;
@@ -46,6 +47,7 @@ import static android.system.OsConstants.SO_REUSEADDR;
 import static com.android.server.util.NetworkStackConstants.IPV4_ADDR_ANY;
 
 import android.annotation.NonNull;
+import android.annotation.Nullable;
 import android.content.Context;
 import android.net.DhcpResults;
 import android.net.InetAddresses;
@@ -60,7 +62,9 @@ import android.net.metrics.DhcpErrorEvent;
 import android.net.metrics.IpConnectivityLog;
 import android.net.util.InterfaceParams;
 import android.net.util.NetworkStackUtils;
+import android.net.util.PacketReader;
 import android.net.util.SocketUtils;
+import android.os.Handler;
 import android.os.Message;
 import android.os.SystemClock;
 import android.system.ErrnoException;
@@ -210,14 +214,9 @@ public class DhcpClient extends StateMachine {
     private final Random mRandom;
     private final IpConnectivityLog mMetricsLog = new IpConnectivityLog();
 
-    // Sockets.
-    // - We use a packet socket to receive, because servers send us packets bound for IP addresses
-    //   which we have not yet configured, and the kernel protocol stack drops these.
-    // - We use a UDP socket to send, so the kernel handles ARP and routing for us (DHCP servers can
-    //   be off-link as well as on-link).
-    private FileDescriptor mPacketSock;
+    // We use a UDP socket to send, so the kernel handles ARP and routing for us (DHCP servers can
+    // be off-link as well as on-link).
     private FileDescriptor mUdpSock;
-    private ReceiveThread mReceiveThread;
 
     // State variables.
     private final StateMachine mController;
@@ -244,6 +243,8 @@ public class DhcpClient extends StateMachine {
     private Dependencies mDependencies;
     @NonNull
     private final NetworkStackIpMemoryStore mIpMemoryStore;
+    @Nullable
+    private DhcpPacketHandler mDhcpPacketHandler;
 
     // Milliseconds SystemClock timestamps used to record transition times to DhcpBoundState.
     private long mLastInitEnterTime;
@@ -396,23 +397,6 @@ public class DhcpClient extends StateMachine {
         mTransactionStartMillis = SystemClock.elapsedRealtime();
     }
 
-    private boolean initSockets() {
-        return initPacketSocket() && initUdpSocket();
-    }
-
-    private boolean initPacketSocket() {
-        try {
-            mPacketSock = Os.socket(AF_PACKET, SOCK_RAW, ETH_P_IP);
-            SocketAddress addr = makePacketSocketAddress(ETH_P_IP, mIface.index);
-            Os.bind(mPacketSock, addr);
-            NetworkStackUtils.attachDhcpFilter(mPacketSock);
-        } catch(SocketException|ErrnoException e) {
-            Log.e(TAG, "Error creating packet socket", e);
-            return false;
-        }
-        return true;
-    }
-
     private boolean initUdpSocket() {
         final int oldTag = TrafficStats.getAndSetThreadStatsTag(
                 TrafficStatsConstants.TAG_SYSTEM_DHCP);
@@ -423,7 +407,7 @@ public class DhcpClient extends StateMachine {
             Os.setsockoptInt(mUdpSock, SOL_SOCKET, SO_BROADCAST, 1);
             Os.setsockoptInt(mUdpSock, SOL_SOCKET, SO_RCVBUF, 0);
             Os.bind(mUdpSock, IPV4_ADDR_ANY, DhcpPacket.DHCP_CLIENT);
-        } catch(SocketException|ErrnoException e) {
+        } catch (SocketException | ErrnoException e) {
             Log.e(TAG, "Error creating UDP socket", e);
             return false;
         } finally {
@@ -436,59 +420,76 @@ public class DhcpClient extends StateMachine {
         try {
             Os.connect(mUdpSock, to, DhcpPacket.DHCP_SERVER);
             return true;
-        } catch (SocketException|ErrnoException e) {
+        } catch (SocketException | ErrnoException e) {
             Log.e(TAG, "Error connecting UDP socket", e);
             return false;
         }
     }
 
-    private void closeSockets() {
-        closeSocketQuietly(mUdpSock);
-        closeSocketQuietly(mPacketSock);
-    }
+    private class DhcpPacketHandler extends PacketReader {
+        private FileDescriptor mPacketSock;
 
-    class ReceiveThread extends Thread {
+        DhcpPacketHandler(Handler handler) {
+            super(handler);
+        }
 
-        private final byte[] mPacket = new byte[DhcpPacket.MAX_LENGTH];
-        private volatile boolean mStopped = false;
+        @Override
+        protected void handlePacket(byte[] recvbuf, int length) {
+            try {
+                final DhcpPacket packet = DhcpPacket.decodeFullPacket(recvbuf, length,
+                        DhcpPacket.ENCAP_L2);
+                if (DBG) Log.d(TAG, "Received packet: " + packet);
+                sendMessage(CMD_RECEIVED_PACKET, packet);
+            } catch (DhcpPacket.ParseException e) {
+                Log.e(TAG, "Can't parse packet: " + e.getMessage());
+                if (PACKET_DBG) {
+                    Log.d(TAG, HexDump.dumpHexString(recvbuf, 0, length));
+                }
+                if (e.errorCode == DhcpErrorEvent.DHCP_NO_COOKIE) {
+                    final int snetTagId = 0x534e4554;
+                    final String bugId = "31850211";
+                    final int uid = -1;
+                    final String data = DhcpPacket.ParseException.class.getName();
+                    EventLog.writeEvent(snetTagId, bugId, uid, data);
+                }
+                mMetricsLog.log(mIfaceName, new DhcpErrorEvent(e.errorCode));
+            }
+        }
 
-        public void halt() {
-            mStopped = true;
-            closeSockets();  // Interrupts the read() call the thread is blocked in.
+        @Override
+        protected FileDescriptor createFd() {
+            try {
+                mPacketSock = Os.socket(AF_PACKET, SOCK_RAW | SOCK_NONBLOCK, 0 /* protocol */);
+                NetworkStackUtils.attachDhcpFilter(mPacketSock);
+                final SocketAddress addr = makePacketSocketAddress(ETH_P_IP, mIface.index);
+                Os.bind(mPacketSock, addr);
+            } catch (SocketException | ErrnoException e) {
+                logError("Error creating packet socket", e);
+                closeFd(mPacketSock);
+                mPacketSock = null;
+                return null;
+            }
+            return mPacketSock;
         }
 
         @Override
-        public void run() {
-            if (DBG) Log.d(TAG, "Receive thread started");
-            while (!mStopped) {
-                int length = 0;  // Or compiler can't tell it's initialized if a parse error occurs.
-                try {
-                    length = Os.read(mPacketSock, mPacket, 0, mPacket.length);
-                    DhcpPacket packet = null;
-                    packet = DhcpPacket.decodeFullPacket(mPacket, length, DhcpPacket.ENCAP_L2);
-                    if (DBG) Log.d(TAG, "Received packet: " + packet);
-                    sendMessage(CMD_RECEIVED_PACKET, packet);
-                } catch (IOException|ErrnoException e) {
-                    if (!mStopped) {
-                        Log.e(TAG, "Read error", e);
-                        logError(DhcpErrorEvent.RECEIVE_ERROR);
-                    }
-                } catch (DhcpPacket.ParseException e) {
-                    Log.e(TAG, "Can't parse packet: " + e.getMessage());
-                    if (PACKET_DBG) {
-                        Log.d(TAG, HexDump.dumpHexString(mPacket, 0, length));
-                    }
-                    if (e.errorCode == DhcpErrorEvent.DHCP_NO_COOKIE) {
-                        int snetTagId = 0x534e4554;
-                        String bugId = "31850211";
-                        int uid = -1;
-                        String data = DhcpPacket.ParseException.class.getName();
-                        EventLog.writeEvent(snetTagId, bugId, uid, data);
-                    }
-                    logError(e.errorCode);
-                }
+        protected int readPacket(FileDescriptor fd, byte[] packetBuffer) throws Exception {
+            try {
+                return Os.read(fd, packetBuffer, 0, packetBuffer.length);
+            } catch (IOException | ErrnoException e) {
+                mMetricsLog.log(mIfaceName, new DhcpErrorEvent(DhcpErrorEvent.RECEIVE_ERROR));
+                throw e;
             }
-            if (DBG) Log.d(TAG, "Receive thread stopped");
+        }
+
+        @Override
+        protected void logError(@NonNull String msg, @Nullable Exception e) {
+            Log.e(TAG, msg, e);
+        }
+
+        public int transmitPacket(final ByteBuffer buf, final SocketAddress socketAddress)
+                throws ErrnoException, SocketException {
+            return Os.sendto(mPacketSock, buf.array(), 0, buf.limit(), 0, socketAddress);
         }
     }
 
@@ -500,7 +501,7 @@ public class DhcpClient extends StateMachine {
         try {
             if (encap == DhcpPacket.ENCAP_L2) {
                 if (DBG) Log.d(TAG, "Broadcasting " + description);
-                Os.sendto(mPacketSock, buf.array(), 0, buf.limit(), 0, mInterfaceBroadcastAddr);
+                mDhcpPacketHandler.transmitPacket(buf, mInterfaceBroadcastAddr);
             } else if (encap == DhcpPacket.ENCAP_BOOTP && to.equals(INADDR_BROADCAST)) {
                 if (DBG) Log.d(TAG, "Broadcasting " + description);
                 // We only send L3-encapped broadcasts in DhcpRebindingState,
@@ -517,7 +518,7 @@ public class DhcpClient extends StateMachine {
                         description, Os.getpeername(mUdpSock)));
                 Os.write(mUdpSock, buf);
             }
-        } catch(ErrnoException|IOException e) {
+        } catch (ErrnoException | IOException e) {
             Log.e(TAG, "Can't send packet: ", e);
             return false;
         }
@@ -774,21 +775,22 @@ public class DhcpClient extends StateMachine {
         @Override
         public void enter() {
             clearDhcpState();
-            if (initInterface() && initSockets()) {
-                mReceiveThread = new ReceiveThread();
-                mReceiveThread.start();
-            } else {
-                notifyFailure();
-                transitionTo(mStoppedState);
+            if (initInterface() && initUdpSocket()) {
+                mDhcpPacketHandler = new DhcpPacketHandler(getHandler());
+                if (mDhcpPacketHandler.start()) return;
+                Log.e(TAG, "Fail to start DHCP Packet Handler");
             }
+            notifyFailure();
+            transitionTo(mStoppedState);
         }
 
         @Override
         public void exit() {
-            if (mReceiveThread != null) {
-                mReceiveThread.halt();  // Also closes sockets.
-                mReceiveThread = null;
+            if (mDhcpPacketHandler != null) {
+                mDhcpPacketHandler.stop();
+                if (DBG) Log.d(TAG, "DHCP Packet Handler stopped");
             }
+            closeSocketQuietly(mUdpSock);
             clearDhcpState();
         }
 
@@ -1289,10 +1291,6 @@ public class DhcpClient extends StateMachine {
     class DhcpRebootingState extends LoggingState {
     }
 
-    private void logError(int errorCode) {
-        mMetricsLog.log(mIfaceName, new DhcpErrorEvent(errorCode));
-    }
-
     private void logState(String name, int durationMs) {
         final DhcpClientEvent event = new DhcpClientEvent.Builder()
                 .setMsg(name)
diff --git a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
index cb7d418..f32171c 100644
--- a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
+++ b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
@@ -130,6 +130,7 @@ public class IpClientIntegrationTest {
     private String mIfaceName;
     private INetd mNetd;
     private HandlerThread mPacketReaderThread;
+    private Handler mHandler;
     private TapPacketReader mPacketReader;
     private IpClient mIpc;
     private Dependencies mDependencies;
@@ -271,7 +272,7 @@ public class IpClientIntegrationTest {
     @After
     public void tearDown() throws Exception {
         if (mPacketReader != null) {
-            mPacketReader.stop(); // Also closes the socket
+            mHandler.post(() -> mPacketReader.stop()); // Also closes the socket
         }
         if (mPacketReaderThread != null) {
             mPacketReaderThread.quitSafely();
@@ -297,10 +298,11 @@ public class IpClientIntegrationTest {
         mIfaceName = iface.getInterfaceName();
         mPacketReaderThread = new HandlerThread(IpClientIntegrationTest.class.getSimpleName());
         mPacketReaderThread.start();
+        mHandler = mPacketReaderThread.getThreadHandler();
 
         final ParcelFileDescriptor tapFd = iface.getFileDescriptor();
-        mPacketReader = new TapPacketReader(mPacketReaderThread.getThreadHandler(), tapFd);
-        mPacketReader.start();
+        mPacketReader = new TapPacketReader(mHandler, tapFd);
+        mHandler.post(() -> mPacketReader.start());
     }
 
     private void setUpIpClient() throws Exception {