diff options
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) { |