From: Christian Herdtweck Date: Wed, 18 Mar 2015 15:01:32 +0000 (+0100) Subject: fixed bugs that caused DestinationUnreachable / TimeExceeded messages to NEVER match... X-Git-Url: http://developer.intra2net.com/git/?a=commitdiff_plain;h=7edd33cf6a9f20bb5ec59fbefa819d3949e7c169;p=pingcheck fixed bugs that caused DestinationUnreachable / TimeExceeded messages to NEVER match anything also added function to get ttl of original request IP packet but unfortunately that always returns 1 if time exceeded :-( Also improved error reporting in ICMP packet parsing process --- diff --git a/src/icmp/icmpdata_pingfailreply.cpp b/src/icmp/icmpdata_pingfailreply.cpp index 967f273..1481f05 100644 --- a/src/icmp/icmpdata_pingfailreply.cpp +++ b/src/icmp/icmpdata_pingfailreply.cpp @@ -23,6 +23,10 @@ */ #include "icmp/icmpdata_pingfailreply.h" +#include "boost_assert_handler.h" + +#include +using I2n::Logger::GlobalLogger; IcmpData_PingFailReply::IcmpData_PingFailReply(const std::size_t size_arg) @@ -44,36 +48,82 @@ bool IcmpData_PingFailReply::match_ping_request( uint16_t IcmpData_PingFailReply::get_icmp_identifier() const -{ return get_icmp_request_data(5); } +{ return get_icmp_request_data(4); } uint16_t IcmpData_PingFailReply::get_icmp_sequence_number() const -{ return get_icmp_request_data(7); } - +{ return get_icmp_request_data(6); } + +/** + * @brief access a given byte within request ICMP data + * + * first tests whether data actually IS a reply for an ICMP request and what + * IP version this is, then access the given byte and byte+1 within request + * ICMP header/data + * + * @returns 2-byte value based on byte and byte+1 in icmp data + */ uint16_t IcmpData_PingFailReply::get_icmp_request_data( - const int data_offset) const + const int icmp_start_byte) const { // payload should be the original query, which is an IP packet. // first check wheter that IP packet contains an ICMP message at all - bool is_icmp; + bool is_icmp = false; int offset = 4; // the 4 uninteresting bytes we need to skip - if (IcmpData::raw_data[offset+0] == 4) // IPv4 + uint8_t version_with_ihl = static_cast(IcmpData::raw_data[offset]); + uint8_t version = (version_with_ihl & 0xF0) >> 4; + + if (version == 4) // IPv4 { is_icmp = IcmpData::raw_data[offset+9] == 1; offset += 20; // 20 byte for IPv4 header } - else if (IcmpData::raw_data[offset+0] == 6) // IPv6 + else if (version == 6) // IPv6 { is_icmp = IcmpData::raw_data[offset+6] == 58; offset += 40; // 40 byte for IPv6 header } + else + { + GlobalLogger.error() << "Request IP header is neither IPv4 nor v6!" + << std::endl; + BOOST_ASSERT( !"Source IP header is neither IPv4 nor v6!" ); + } if ( !is_icmp ) return static_cast(-1); else // if it is an icmp message, then the icmp packet comes right after - // the IP header. Inside the icmp packet we need bytes 5-8 - return IcmpData::raw_data.decode16(offset+data_offset, - offset+data_offset+1); + // the IP header. Inside the icmp packet we need given byte offset + return IcmpData::raw_data.decode16(offset+icmp_start_byte, + offset+icmp_start_byte+1); +} + +/** + * copying code from ip header parsing ... + * If we need more functions of this sort, could create a data stream from + * raw_data, read a IpHeader from it and get info from that + * + * Unfortunately, the value in packet is not the original one, but the one + * change en route to destination, so in case of TimeExceeded, it is always 1 + */ +uint8_t IcmpData_PingFailReply::get_ip_ttl() const +{ + int offset = 4; // the 4 uninteresting bytes we need to skip + uint8_t version_with_ihl = static_cast(IcmpData::raw_data[offset]); + uint8_t version = (version_with_ihl & 0xF0) >> 4; + + if (version == 4) // IPv4 + offset += 8; // byte 8 within IP header + else if (version == 6) // IPv6 + offset += 7; + else + { + GlobalLogger.error() << "Request IP header is neither IPv4 nor v6!" + << std::endl; + BOOST_ASSERT( !"Source IP header is neither IPv4 nor v6!" ); + } + + return IcmpData::raw_data[offset]; } // (created using vim -- the world's best text editor) diff --git a/src/icmp/icmpdata_pingfailreply.h b/src/icmp/icmpdata_pingfailreply.h index f47cc79..ecdc8ae 100644 --- a/src/icmp/icmpdata_pingfailreply.h +++ b/src/icmp/icmpdata_pingfailreply.h @@ -74,10 +74,12 @@ public: uint16_t get_icmp_sequence_number() const; + uint8_t get_ip_ttl() const; + // not implementing print nor to_string protected: - uint16_t get_icmp_request_data(const int data_offset) const; + uint16_t get_icmp_request_data(const int icmp_start_byte) const; bool match_ping_request(const uint16_t identifier, const uint16_t sequence_number) const; diff --git a/src/icmp/icmppacket.cpp b/src/icmp/icmppacket.cpp index b8021da..a6a6c3f 100644 --- a/src/icmp/icmppacket.cpp +++ b/src/icmp/icmppacket.cpp @@ -50,7 +50,10 @@ IcmpPacket::IcmpPacket(const icmp::socket::protocol_type &protocol) else if ( icmp::v6() == protocol ) ip_head_ptr.reset( new Ipv6Header() ); else // pingcheck-type throwing of exceptions: + { + GlobalLogger.error() << "Invalid IP version, need 4 or 6!"; BOOST_ASSERT( !"Invalid IP version, need 4 or 6!" ); + } } // IP header is created with only 0s, is not used in write, anyway @@ -66,7 +69,10 @@ IcmpPacket::IcmpPacket(const icmp::socket::protocol_type &protocol, else if ( icmp::v6() == protocol ) ip_head_ptr.reset( new Ipv6Header() ); else // pingcheck-type throwing of exceptions: + { + GlobalLogger.error() << "Invalid IP version, need 4 or 6!"; BOOST_ASSERT( !"Invalid IP version, need 4 or 6!" ); + } // create checksum icmp_header.calc_checksum( icmp_data_ptr->calc_checksum_part() ); @@ -106,9 +112,8 @@ bool IcmpPacket::match_time_exceeded(const uint16_t identifier, const uint16_t sequence_number, const address &destination_address) const { - return ip_head_ptr->get_source_address() == destination_address - && icmp_data_ptr->match_time_exceeded(identifier, - sequence_number); + return icmp_data_ptr->match_time_exceeded(identifier, sequence_number); + // do not test ip since probably comes from router on route to dest. addr } bool IcmpPacket::match_echo_reply(const uint16_t identifier, @@ -321,9 +326,17 @@ std::string IcmpPacket::to_string() const { std::stringstream buf; buf << "[IcmpPacket: "; - buf << ip_head_ptr->to_string() << ", "; + if (ip_head_ptr) + buf << ip_head_ptr->to_string() << ", "; + else + buf << "no ip header, "; + buf << icmp_header.to_string() << ", "; - buf << icmp_data_ptr->to_string(); + + if (icmp_data_ptr) + buf << icmp_data_ptr->to_string(); + else + buf << "no icmp data"; buf << "]"; return buf.str(); } diff --git a/src/icmp/icmppacketfactory.cpp b/src/icmp/icmppacketfactory.cpp index 6e9624d..4cd9839 100644 --- a/src/icmp/icmppacketfactory.cpp +++ b/src/icmp/icmppacketfactory.cpp @@ -197,6 +197,12 @@ IcmpPacketItem IcmpPacketFactory::create_icmp_packet( if ( dump_mode == 2 || ( dump_mode==1 && !icmp_packet ) ) dump_packet(data_backup.str()); + if (icmp_packet) + GlobalLogger.debug() << "Read packet " << icmp_packet->to_string() + << endl; + else + GlobalLogger.debug() << "Read packet failed" << endl; + return icmp_packet; } @@ -235,8 +241,8 @@ IcmpPacketItem IcmpPacketFactory::create_icmp_packet_echo_request( IcmpPacketItem icmp_packet( new IcmpPacket( protocol, icmp_header, icmp_data ) ); - GlobalLogger.debug() << "IcmpPacketFactory: created " - << icmp_packet->to_string() << std::endl; + GlobalLogger.debug() << "IcmpPacketFactory: created echo request packet" + << std::endl; return icmp_packet; } diff --git a/src/icmp/icmppinger.cpp b/src/icmp/icmppinger.cpp index 67f3b34..f733702 100644 --- a/src/icmp/icmppinger.cpp +++ b/src/icmp/icmppinger.cpp @@ -298,7 +298,7 @@ bool IcmpPinger::handle_receive_icmp_packet(const IcmpPacketItem icmp_packet, { // continue, might be an old packet // or return false right away, do not want packet anyway... - GlobalLogger.info() + GlobalLogger.debug() << DestinationEndpoint.address().to_string() << ": Not interested in packets since we already got a reply" << endl; @@ -363,9 +363,10 @@ bool IcmpPinger::handle_receive_icmp_packet(const IcmpPacketItem icmp_packet, } else { - GlobalLogger.info() + GlobalLogger.debug() << DestinationEndpoint.address().to_string() - << ": Received packet that does not match or is old" << endl; + << ": Received packet that does not match or has wrong seq.nr" + << endl; } return does_match; @@ -465,6 +466,10 @@ IcmpPacketDistributor::IcmpPacketDistributor( ReplyBuffer(), PingerList() { + // set TTL for testing + //const boost::asio::ip::unicast::hops option( 3 ); + //Socket->set_option(option); + NetworkInterface NetInterface( network_interface, *Socket ); @@ -543,11 +548,16 @@ void IcmpPacketDistributor::handle_receive( break; } if (!packet_matches) - GlobalLogger.warning() << "Packet did not match any pinger" + GlobalLogger.info() << "Packet did not match any pinger" << std::endl; } } + catch ( const std::exception &ex ) + { + GlobalLogger.notice() << "Exception during ICMP parse: " << ex.what() + << std::endl; + } catch ( ... ) { GlobalLogger.notice() << "Exception during ICMP parse." << std::endl; diff --git a/src/icmp/icmptimeexceededdata.cpp b/src/icmp/icmptimeexceededdata.cpp index 5adf940..4eb31d4 100644 --- a/src/icmp/icmptimeexceededdata.cpp +++ b/src/icmp/icmptimeexceededdata.cpp @@ -56,11 +56,18 @@ void IcmpTimeExceededData::print( GlobalLogger.info() << "Time exceeded pinging " << remote_address << " (icmp_seq=" << sequence_number << ")!" << std::endl; + // TTL given as arg is the one of the return packet, so not of interest + // IcmpData_PingFailReply::get_ip_ttl() always gives 1 } std::string IcmpTimeExceededData::to_string() const { - return "[TimeExceededData]"; + std::stringstream buf; + buf << "[TimeExceededData: ID=" << std::showbase << std::hex + << IcmpData_PingFailReply::get_icmp_identifier() << ",seq.nr=" + << IcmpData_PingFailReply::get_icmp_sequence_number() << "]"; + + return buf.str(); } // (created using vim -- the world's best text editor)