Fix potential exceptions in FdEventReader users am: a51a28854f

Change-Id: Ie1358a7b62ad5331adbeb2143de98d747e936d2b

6 files changed, 44 insertions, 33 deletions

diff --git a/common/moduleutils/src/android/net/ip/NetlinkMonitor.java b/common/moduleutils/src/android/net/ip/NetlinkMonitor.java
index 806f3ef..1bbedb6 100644
--- a/common/moduleutils/src/android/net/ip/NetlinkMonitor.java
+++ b/common/moduleutils/src/android/net/ip/NetlinkMonitor.java
@@ -106,20 +106,24 @@ public class NetlinkMonitor extends PacketReader {
         byteBuffer.order(ByteOrder.nativeOrder());
 
         while (byteBuffer.remaining() > 0) {
-            final int position = byteBuffer.position();
-            final NetlinkMessage nlMsg = NetlinkMessage.parse(byteBuffer);
-            if (nlMsg == null || nlMsg.getHeader() == null) {
-                byteBuffer.position(position);
-                mLog.e("unparsable netlink msg: " + hexify(byteBuffer));
-                break;
+            try {
+                final int position = byteBuffer.position();
+                final NetlinkMessage nlMsg = NetlinkMessage.parse(byteBuffer);
+                if (nlMsg == null || nlMsg.getHeader() == null) {
+                    byteBuffer.position(position);
+                    mLog.e("unparsable netlink msg: " + hexify(byteBuffer));
+                    break;
+                }
+
+                if (nlMsg instanceof NetlinkErrorMessage) {
+                    mLog.e("netlink error: " + nlMsg);
+                    continue;
+                }
+
+                processNetlinkMessage(nlMsg, whenMs);
+            } catch (Throwable e) {
+                mLog.e("Error handling netlink message", e);
             }
-
-            if (nlMsg instanceof NetlinkErrorMessage) {
-                mLog.e("netlink error: " + nlMsg);
-                continue;
-            }
-
-            processNetlinkMessage(nlMsg, whenMs);
         }
     }
 
diff --git a/common/moduleutils/src/android/net/util/FdEventsReader.java b/common/moduleutils/src/android/net/util/FdEventsReader.java
index ebd6c53..2b410e6 100644
--- a/common/moduleutils/src/android/net/util/FdEventsReader.java
+++ b/common/moduleutils/src/android/net/util/FdEventsReader.java
@@ -24,6 +24,7 @@ import android.os.Looper;
 import android.os.MessageQueue;
 import android.system.ErrnoException;
 import android.system.OsConstants;
+import android.util.Log;
 
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
@@ -244,6 +245,7 @@ public abstract class FdEventsReader<BufferType> {
                 handlePacket(mBuffer, bytesRead);
             } catch (Exception e) {
                 logError("handlePacket error: ", e);
+                Log.wtf(FdEventsReader.class.getSimpleName(), "Error handling packet: stopping", e);
                 break;
             }
         }
diff --git a/src/android/net/dhcp/DhcpPacketListener.java b/src/android/net/dhcp/DhcpPacketListener.java
index c9235da..656b642 100644
--- a/src/android/net/dhcp/DhcpPacketListener.java
+++ b/src/android/net/dhcp/DhcpPacketListener.java
@@ -19,6 +19,7 @@ package android.net.dhcp;
 import android.net.util.FdEventsReader;
 import android.os.Handler;
 import android.system.Os;
+import android.util.Log;
 
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
@@ -59,6 +60,8 @@ abstract class DhcpPacketListener extends FdEventsReader<DhcpPacketListener.Payl
             onReceive(packet, recvbuf.mSrcAddr, recvbuf.mSrcPort);
         } catch (DhcpPacket.ParseException e) {
             logParseError(recvbuf.mBytes, length, e);
+        } catch (Throwable e) {
+            Log.wtf(DhcpPacketListener.class.getSimpleName(), "Error handling DHCP packet", e);
         }
     }
 
diff --git a/src/android/net/ip/ConnectivityPacketTracker.java b/src/android/net/ip/ConnectivityPacketTracker.java
index bbaf61e..aa85f3a 100644
--- a/src/android/net/ip/ConnectivityPacketTracker.java
+++ b/src/android/net/ip/ConnectivityPacketTracker.java
@@ -133,9 +133,14 @@ public class ConnectivityPacketTracker {
                 return;
             }
 
-            final String summary = ConnectivityPacketSummary.summarize(
-                    mInterface.macAddr, recvbuf, length);
-            if (summary == null) return;
+            final String summary;
+            try {
+                summary = ConnectivityPacketSummary.summarize(mInterface.macAddr, recvbuf, length);
+                if (summary == null) return;
+            } catch (Exception e) {
+                if (DBG) Log.d(mTag, "Error creating packet summary", e);
+                return;
+            }
 
             if (DBG) Log.d(mTag, summary);
             addLogEntry(summary + "\n[" + HexDump.toHexString(recvbuf, 0, length) + "]");
diff --git a/src/android/net/ip/IpReachabilityMonitor.java b/src/android/net/ip/IpReachabilityMonitor.java
index db928d6..2a0ca5d 100644
--- a/src/android/net/ip/IpReachabilityMonitor.java
+++ b/src/android/net/ip/IpReachabilityMonitor.java
@@ -341,7 +341,14 @@ public class IpReachabilityMonitor {
             // TODO: Consider using NeighborEvent#isValid() here; it's more
             // strict but may interact badly if other entries are somehow in
             // NUD_INCOMPLETE (say, during network attach).
-            if (entry.getValue().nudState != StructNdMsg.NUD_FAILED) continue;
+            final NeighborEvent val = entry.getValue();
+
+            // Find all the neighbors that have gone into FAILED state.
+            // Ignore entries for which we have never received an event. If there are neighbors
+            // that never respond to ARP/ND, the kernel will send several FAILED event, then
+            // an INCOMPLETE event, and then more FAILED events. The INCOMPLETE event will
+            // populate the map and the subsequent FAILED event will be processed.
+            if (val == null || val.nudState != StructNdMsg.NUD_FAILED) continue;
 
             ip = entry.getKey();
             for (RouteInfo route : mLinkProperties.getRoutes()) {
@@ -378,10 +385,12 @@ public class IpReachabilityMonitor {
                 Log.d(TAG, "neighbour IPv4(v6): " + entry.getKey() + " neighbour state: "
                         + StructNdMsg.stringForNudState(entry.getValue().nudState));
             }
-            if (entry.getValue().nudState != StructNdMsg.NUD_REACHABLE) return;
+            final NeighborEvent val = entry.getValue();
+            // If an entry is null, consider that probing for that neighbour has completed.
+            if (val == null || val.nudState != StructNdMsg.NUD_REACHABLE) return;
         }
 
-        // All neighbours in the watchlist are in REACHABLE state and connection is stable,
+        // Probing for all neighbours in the watchlist is complete and the connection is stable,
         // restore NUD probe parameters to steadystate value. In the case where neighbours
         // are responsive, this code will run before the wakelock expires.
         setNeighbourParametersForSteadyState();
diff --git a/tests/unit/src/android/net/ip/IpReachabilityMonitorTest.kt b/tests/unit/src/android/net/ip/IpReachabilityMonitorTest.kt
index d9a97b2..863e268 100644
--- a/tests/unit/src/android/net/ip/IpReachabilityMonitorTest.kt
+++ b/tests/unit/src/android/net/ip/IpReachabilityMonitorTest.kt
@@ -49,7 +49,6 @@ import org.mockito.ArgumentMatchers.eq
 import org.mockito.Mockito.doAnswer
 import org.mockito.Mockito.doReturn
 import org.mockito.Mockito.mock
-import org.mockito.Mockito.never
 import org.mockito.Mockito.timeout
 import org.mockito.Mockito.verify
 import java.io.FileDescriptor
@@ -200,23 +199,12 @@ class IpReachabilityMonitorTest {
         handlerThread.quitSafely()
     }
 
-    // TODO: fix this bug
     @Test
-    fun testLoseProvisioning_CrashIfFirstProbeIsFailed() {
+    fun testLoseProvisioning_FirstProbeIsFailed() {
         reachabilityMonitor.updateLinkProperties(TEST_LINK_PROPERTIES)
 
-        doAnswer {
-            // Set the fd as invalid when the event listener is removed, to avoid a crash when the
-            // reader tries to close the mock fd.
-            // This does not exactly reflect behavior on close, but this test is only demonstrating
-            // a bug that causes the close, and it will be removed when the bug fixed.
-            doReturn(false).`when`(fd).valid()
-        }.`when`(neighborMonitor.msgQueue).removeOnFileDescriptorEventListener(any())
-
         neighborMonitor.enqueuePacket(makeNewNeighMessage(TEST_IPV4_DNS, NUD_FAILED))
-        verify(neighborMonitor.msgQueue, timeout(TEST_TIMEOUT_MS))
-                .removeOnFileDescriptorEventListener(any())
-        verify(callback, never()).notifyLost(eq(TEST_IPV4_DNS), anyString())
+        verify(callback, timeout(TEST_TIMEOUT_MS)).notifyLost(eq(TEST_IPV4_DNS), anyString())
     }
 
     private fun runLoseProvisioningTest(lostNeighbor: InetAddress) {