Properly handle ICMP packet replies: The kernel sends us a copy of all incoming ICMP...

diff --git a/src/host/boostpinger.cpp b/src/host/boostpinger.cpp
index b23ff20..a979044 100644 (file)
--- a/src/host/boostpinger.cpp
+++ b/src/host/boostpinger.cpp
@@ -1,6 +1,5 @@
 #include "host/boostpinger.h"
 
-#include <iostream>
 #include <istream>
 #include <ostream>
 
@@ -209,57 +208,80 @@ void BoostPinger::handle_ping_done()
     PingDoneCallback(ping_success);
 }
 
+/**
+ * @brief Receive ICMP packets
+ * Note: Will receive -all- ICMP packets from the kernel. So if the packet doesn't match
+ * our criteria, we have to schedule another start_receive() requests. The timeout stays the same.
+ *
+ * In the future we might redesign the code to handle all ICMP packets
+ * from a single raw socket.
+ *
+ * @param bytes_transferred Number of bytes transferred
+ * @return void
+ **/
 void BoostPinger::handle_receive_icmp_packet( const size_t &bytes_transferred )
 {
     // The actual number of bytes received is committed to the buffer so that we
     // can extract it using a std::istream object.
     ReplyBuffer.commit( bytes_transferred );
 
-    istream is( &ReplyBuffer );
-    if ( !is )
+    try
     {
-        GlobalLogger.error() << "can't handle ReplyBuffer" << endl;
-        return;
-    }
+        istream is( &ReplyBuffer );
+        if ( !is )
+        {
+            GlobalLogger.error() << "can't handle ReplyBuffer" << endl;
+            return;
+        }
 
-    // Decode the reply packet.
-    IcmpPacket icmp_packet;
-    if (!(is >> icmp_packet))
-    {
-        GlobalLogger.error() << "ignoring broken ICMP packet" << endl;
-        return;
-    }
+        // Decode the reply packet.
+        IcmpPacket icmp_packet;
+        if (!(is >> icmp_packet))
+        {
+            GlobalLogger.notice() << "ignoring broken ICMP packet" << endl;
+            return;
+        }
 
-    // We can receive all ICMP packets received by the host, so we need to
-    // filter out only the echo replies that match the our identifier,
-    // expected sequence number, and destination host address (receive just the
-    // ICMP packets from the host we had ping).
-    if ( icmp_packet.match(
-            IcmpType_EchoReply, Identifier, SequenceNumber,
-            DestinationEndpoint.address())
-    )
-    {
-        ReceivedReply = true;
+        // We can receive all ICMP packets received by the host, so we need to
+        // filter out only the echo replies that match the our identifier,
+        // expected sequence number, and destination host address (receive just the
+        // ICMP packets from the host we had ping).
+        if ( icmp_packet.match(
+                IcmpType_EchoReply, Identifier, SequenceNumber,
+                DestinationEndpoint.address())
+        )
+        {
+            ReceivedReply = true;
+            print_echo_reply( icmp_packet, bytes_transferred );
 
-        print_echo_reply( icmp_packet, bytes_transferred );
+            set_ping_status( PingStatus_SuccessReply );
 
-        set_ping_status( PingStatus_SuccessReply );
-    }
-    else if ( icmp_packet.match(
-            IcmpType_DestinationUnreachable, Identifier, SequenceNumber,
-            DestinationEndpoint.address()
-    ) )
-    {
-        ReceivedReply = true;
-        print_destination_unreachable( icmp_packet );
+            IcmpPacketReceiveTimer.cancel();
+        }
+        else if ( icmp_packet.match(
+                IcmpType_DestinationUnreachable, Identifier, SequenceNumber,
+                DestinationEndpoint.address()
+        ) )
+        {
+            ReceivedReply = true;
+            print_destination_unreachable( icmp_packet );
+
+            set_ping_status( PingStatus_FailureDestinationUnreachable );
 
-        set_ping_status( PingStatus_FailureDestinationUnreachable );
-    } else
+            IcmpPacketReceiveTimer.cancel();
+        } else
+        {
+            /*
+            GlobalLogger.notice() << "unknown ICMP reply (src IP: " << icmp_packet.get_ip_header().get_source_address() << ")"
+                                            << ". Starting another recieve till timeout." << endl;
+            */
+            start_receive();
+        }
+    } catch(...)
     {
-        GlobalLogger.notice() << "unknown ICMP reply" << endl;
+        GlobalLogger.notice() << "exception during ICMP parse. Starting another recieve till timeout." << endl;
+        start_receive();
     }
-
-    IcmpPacketReceiveTimer.cancel();
 }
 
 void BoostPinger::print_echo_reply(

diff --git a/src/host/pingscheduler.cpp b/src/host/pingscheduler.cpp
index 91f6a7a..8065645 100644 (file)
--- a/src/host/pingscheduler.cpp
+++ b/src/host/pingscheduler.cpp
@@ -25,6 +25,9 @@ using I2n::Logger::GlobalLogger;
 // PingScheduler
 //-----------------------------------------------------------------------------
 
+/// Ping reply timeout. Could be made a configuration variable
+const int PingReplyTimeout = 30;
+
 PingScheduler::PingScheduler(
         const string &network_interface,
         const string &destination_address,
@@ -41,7 +44,7 @@ PingScheduler::PingScheduler(
     PingIntervalInSec( ping_interval_in_sec ),
     IpList( destination_address, nameserver ),
     HostAnalyzer( destination_address, ping_fail_percentage_limit, link_analyzer ),
-    Pinger(IoService, LocalNetworkInterfaceName, 5),
+    Pinger(IoService, LocalNetworkInterfaceName, PingReplyTimeout),
     Thread()
 {
 }

diff --git a/src/icmp/icmppacket.cpp b/src/icmp/icmppacket.cpp
index 2f75248..88746e1 100644 (file)
--- a/src/icmp/icmppacket.cpp
+++ b/src/icmp/icmppacket.cpp
@@ -1,4 +1,5 @@
 #include "icmp/icmppacket.h"
+#include <iostream>
 
 using namespace std;
 using boost::asio::ip::address;
@@ -50,10 +51,17 @@ bool IcmpPacket::match(
         const address &source_address
 ) const
 {
-    bool type_match = IcmpPayloadHeader.get_type() == type;
-    bool identifier_match = IcmpPayloadHeader.get_identifier() == identifier;
-    bool seq_num_match = IcmpPayloadHeader.get_sequence_number() == sequence_number;
-    bool address_match = IpHeader.get_source_address() == source_address;
+    bool type_match = IcmpPayloadHeader.get_type() == type ? true : false;
+    bool identifier_match = IcmpPayloadHeader.get_identifier() == identifier ? true: false;
+    bool seq_num_match = IcmpPayloadHeader.get_sequence_number() == sequence_number ? true : false;
+    bool address_match = IpHeader.get_source_address() == source_address ? true : false;
+
+    /*
+    std::cout << "Packet type: " << IcmpPayloadHeader.get_type() << ", search_type: " << type << ", match: " << (type_match ? "YES" : "NO") << std::endl;
+    std::cout << "Packet identifier: " << IcmpPayloadHeader.get_identifier() << ", search_ident: " << identifier << ", match: " << (identifier_match ? "YES" : "NO") << std::endl;
+    std::cout << "Packet seq: " << IcmpPayloadHeader.get_sequence_number() << ", search_seq: " << sequence_number << ", match: " << (seq_num_match ? "YES" : "NO") << std::endl;
+    std::cout << "Packet addr: " << IpHeader.get_source_address() << ", search_addr: " << source_address << ", match: " << (address_match ? "YES" : "NO") << std::endl;
+*/
 
     return ( type_match && identifier_match && seq_num_match && address_match );
 }