From cd71d0950be00b310190dce9a37c479dcfb66fab Mon Sep 17 00:00:00 2001 From: Christian Herdtweck Date: Mon, 4 May 2015 16:09:10 +0200 Subject: [PATCH] created arg recursion_count to async_resolve and many other to avoid infinite loops * arg recursion_count replaces cname_count which up to now identified problems in post-processing * created simple loop test * added option to configure recursion depth limit to DnsMaster minor other changes: * corrected docu for resolved-ip-ttl-threshold option: is not milliseconds but seconds * add test to ResolverBase: check number of callbacks and warn if suspiciously many --- src/config/option/resolvedipttlthresholdoption.cpp | 2 +- src/dns/dnscache.cpp | 9 +- src/dns/dnsmaster.cpp | 11 ++ src/dns/dnsmaster.h | 5 + src/dns/dnsresolver.cpp | 125 ++++++++++++-------- src/dns/dnsresolver.h | 25 +++-- src/dns/ippseudoresolver.h | 2 +- src/dns/resolverbase.cpp | 38 +++++-- src/dns/resolverbase.h | 11 +- src/host/pingscheduler.cpp | 4 +- src/host/pingscheduler.h | 3 +- src/main.cpp | 2 + test/data/icmp_destinationunreachable.pcap | Bin 166 -> 166 bytes test/data/icmp_echoreply.pcap | Bin 100 -> 100 bytes test/data/icmp_echorequest.pcap | Bin 94 -> 94 bytes test/data/icmp_timeexceeded.pcap | Bin 122 -> 122 bytes test/test_dns.cpp | 79 ++++++++++--- test/test_icmppacket.cpp | 10 +- 18 files changed, 223 insertions(+), 103 deletions(-) diff --git a/src/config/option/resolvedipttlthresholdoption.cpp b/src/config/option/resolvedipttlthresholdoption.cpp index a8f9672..66809d5 100644 --- a/src/config/option/resolvedipttlthresholdoption.cpp +++ b/src/config/option/resolvedipttlthresholdoption.cpp @@ -35,7 +35,7 @@ ResolvedIpTtlThresholdOption::ResolvedIpTtlThresholdOption() : ConfigurationOption( "resolved-ip-ttl-threshold", value()->default_value( 10 ), - "Minimum time in milliseconds that IP has to be valid when testing for refresh" + "Minimum time in seconds that IP has to be valid when testing for refresh" ) { } diff --git a/src/dns/dnscache.cpp b/src/dns/dnscache.cpp index 4c524e7..b1c4f33 100644 --- a/src/dns/dnscache.cpp +++ b/src/dns/dnscache.cpp @@ -46,7 +46,6 @@ using I2n::Logger::GlobalLogger; namespace Config { int SaveTimerSeconds = 60; - int MaxRetrievalRecursions = 10; } // ----------------------------------------------------------------------------- @@ -369,6 +368,8 @@ HostAddressVec DnsCache::get_ips_recursive(const std::string &hostname, HostAddressVec result = get_ips(current_host, check_up_to_date); int n_recursions = 0; uint32_t min_cname_ttl = 0xffff; // largest possible unsigned 4-byte value + int max_recursion_count = DnsMaster::get_instance() + ->get_max_recursion_count(); while ( result.empty() ) { current_cname = get_cname(current_host, check_up_to_date); @@ -376,7 +377,7 @@ HostAddressVec DnsCache::get_ips_recursive(const std::string &hostname, break; current_host = key_for_hostname(current_cname.Host); - if (++n_recursions >= Config::MaxRetrievalRecursions) + if (++n_recursions >= max_recursion_count) { GlobalLogger.warning() << "DnsCache: reached recursion limit of " << n_recursions << " in recursive IP retrieval!"; @@ -426,9 +427,11 @@ std::string DnsCache::get_first_outdated_cname(const std::string &hostname, std::string first_outdated = hostname; Cname cname; int n_recursions = 0; + int max_recursion_count = DnsMaster::get_instance() + ->get_max_recursion_count(); while (true) { - if (++n_recursions >= Config::MaxRetrievalRecursions) + if (++n_recursions >= max_recursion_count) { GlobalLogger.warning() << "DnsCache: reached recursion limit of " << n_recursions << " in search of outdated CNAMEs!"; diff --git a/src/dns/dnsmaster.cpp b/src/dns/dnsmaster.cpp index 7235a1e..b0b4049 100644 --- a/src/dns/dnsmaster.cpp +++ b/src/dns/dnsmaster.cpp @@ -41,6 +41,7 @@ void DnsMaster::create_master(const IoServiceItem &io_serv, const int resolved_ip_ttl_threshold, const int min_time_between_resolves, const int max_address_resolution_attempts, + const int max_recursion_count, const std::string &cache_file) { GlobalLogger.info() << "DnsMaster: Creating cache"; @@ -54,6 +55,7 @@ void DnsMaster::create_master(const IoServiceItem &io_serv, default_name_server, resolved_ip_ttl_threshold, max_address_resolution_attempts, + max_recursion_count, cache); } @@ -61,6 +63,7 @@ void DnsMaster::create_master(const IoServiceItem &io_serv, const boost::asio::ip::address &default_name_server, const int resolved_ip_ttl_threshold, const int max_address_resolution_attempts, + const int max_recursion_count, const DnsCacheItem &cache) { if (TheOnlyInstance) @@ -75,6 +78,7 @@ void DnsMaster::create_master(const IoServiceItem &io_serv, default_name_server, resolved_ip_ttl_threshold, max_address_resolution_attempts, + max_recursion_count, cache) ); } @@ -83,11 +87,13 @@ DnsMaster::DnsMaster(const IoServiceItem &io_serv, const boost::asio::ip::address &default_name_server, const int resolved_ip_ttl_threshold, const int max_address_resolution_attempts, + const int max_recursion_count, const DnsCacheItem &cache) : IoService( io_serv ) , DefaultNameServer( default_name_server ) , ResolvedIpTtlThreshold( resolved_ip_ttl_threshold ) , MaxAddressResolutionAttempts( max_address_resolution_attempts ) + , MaxRecursionCount( max_recursion_count ) , Cache(cache) , ResolverMap() { @@ -221,6 +227,11 @@ int DnsMaster::get_max_address_resolution_attempts() const return MaxAddressResolutionAttempts; } +int DnsMaster::get_max_recursion_count() const +{ + return MaxRecursionCount; +} + std::string to_string(const DnsIpProtocol &protocol) { switch (protocol) diff --git a/src/dns/dnsmaster.h b/src/dns/dnsmaster.h index 424d8ba..3b52cdf 100644 --- a/src/dns/dnsmaster.h +++ b/src/dns/dnsmaster.h @@ -95,6 +95,7 @@ private: const boost::asio::ip::address &default_name_server, const int resolved_ip_ttl_threshold, const int max_address_resolution_attempts, + const int max_recursion_count, const DnsCacheItem &cache); public: static void create_master(const IoServiceItem &io_serv, @@ -102,11 +103,13 @@ public: const int resolved_ip_ttl_threshold, const int min_time_between_resolves, const int max_address_resolution_attempts, + const int max_recursion_count, const std::string &cache_file); static void create_master(const IoServiceItem &io_serv, const boost::asio::ip::address &default_name_server, const int resolved_ip_ttl_threshold, const int max_address_resolution_attempts, + const int max_recursion_count, const DnsCacheItem &cache); // needed for unit test static DnsMasterItem& get_instance(); ~DnsMaster(); @@ -116,6 +119,7 @@ public: //boost::asio::ip::address &get_default_name_server() const; // unused int get_resolved_ip_ttl_threshold() const; int get_max_address_resolution_attempts() const; + int get_max_recursion_count() const; // variables private: @@ -123,6 +127,7 @@ private: const boost::asio::ip::address DefaultNameServer; const int ResolvedIpTtlThreshold; const int MaxAddressResolutionAttempts; + const int MaxRecursionCount; DnsCacheItem Cache; resolver_map_type ResolverMap; diff --git a/src/dns/dnsresolver.cpp b/src/dns/dnsresolver.cpp index 52c485c..634a439 100644 --- a/src/dns/dnsresolver.cpp +++ b/src/dns/dnsresolver.cpp @@ -106,7 +106,7 @@ DnsResolver::~DnsResolver() * copied here code from boost::net::dns::resolve.hpp, since want async * operation and that is used only internally, there */ -void DnsResolver::do_resolve() +void DnsResolver::do_resolve(const int recursion_count) { // check if resolving already if (IsResolving) @@ -138,13 +138,14 @@ void DnsResolver::do_resolve() memcpy( &RequestId, message_id.data, sizeof(RequestId) ); dns_message.id( RequestId ); GlobalLogger.debug() << LogPrefix << "Request has ID " - << std::showbase << std::hex << dns_message.id(); + << std::showbase << std::hex << dns_message.id(); // setup receipt of reply Socket.async_receive_from( boost::asio::buffer(ReceiveBuffer.get_array()), NameServer, boost::bind( &DnsResolver::handle_dns_result, this, + recursion_count, boost::asio::placeholders::error, boost::asio::placeholders::bytes_transferred) ); @@ -154,7 +155,8 @@ void DnsResolver::do_resolve() seconds(Config::ResolveTimeoutSeconds)); ResolveTimeoutTimer.async_wait( boost::bind( &DnsResolver::handle_resolve_timeout, - this, boost::asio::placeholders::error) ); + this, recursion_count, + boost::asio::placeholders::error) ); // send dns request dns_message.encode(RequestBuffer); @@ -170,27 +172,28 @@ void DnsResolver::do_resolve() GlobalLogger.warning() << LogPrefix << "Sending of DNS request message failed: " << err.what(); - schedule_retry(); + schedule_retry(recursion_count); return; } if ( bytes_sent == 0 ) { GlobalLogger.warning() << LogPrefix << "Empty DNS request sent!"; - schedule_retry(); + schedule_retry(recursion_count); return; } } -void DnsResolver::handle_dns_result(const boost::system::error_code &error, +void DnsResolver::handle_dns_result(const int recursion_count, + const boost::system::error_code &error, const std::size_t bytes_transferred) { if (error) { GlobalLogger.info() << LogPrefix << "DNS resolve resulted in error " << error << " --> request retry"; - schedule_retry(); + schedule_retry(recursion_count); return; } else if ( OperationCancelled ) @@ -230,25 +233,25 @@ void DnsResolver::handle_dns_result(const boost::system::error_code &error, GlobalLogger.debug() << LogPrefix <<"Checking ANSWERS section of dns reply"; gather_results(result_message.answers(), &result_ips, &result_cnames, &result_name_servers); - // results should have the logical order - // Hostname [ --> cname1 --> cname2 --> ... --> cnameN ] [ --> ips ] - // for cname count to be correct; rest of code should not be affected if not // remember cname list (if there were any) + // results should have the logical order + // Hostname [ --> cname1 --> cname2 --> ... --> cnameN ] [ --> ips ]; + // otherwise just have unneccessary cnames in cache BOOST_FOREACH( const src_cname_pair &host_and_cname, result_cnames ) ResolverBase::update_cache(host_and_cname.first, host_and_cname.second); if ( !result_ips.empty() ) - handle_ips( result_ips ); + handle_ips( recursion_count, result_ips ); else if ( !result_cnames.empty() ) // no IPs but at least one cname --> find the "last" cname and // re-start resolving with that - handle_cname(result_cnames); + handle_cname(recursion_count, result_cnames); else { // no answers --> cannot proceed GlobalLogger.warning() << LogPrefix << "No IP nor CNAME received! " << "--> request retry"; - schedule_retry(); + schedule_retry(recursion_count); } } @@ -257,7 +260,7 @@ void DnsResolver::handle_dns_result(const boost::system::error_code &error, * * can be run on anwers(), autorities() and additional() sections of dns reply * messages - * + * * @param rr_list: input list of resource records * @param result_ips: output vector of ips * @param result_cnames: output vector of cnames @@ -354,27 +357,29 @@ void DnsResolver::gather_results(const boost::net::dns::rr_list_t *rr_list, } -void DnsResolver::handle_unavailable() +void DnsResolver::handle_unavailable(const int recursion_count) { // schedule new attempt in quite a while StaleDataLongtermTimer.expires_from_now( seconds(Config::StaleDataLongtermSeconds)); StaleDataLongtermTimer.async_wait( - boost::bind( &DnsResolver::wait_timer_timeout_handler, - this, boost::asio::placeholders::error + boost::bind( &DnsResolver::wait_timer_timeout_handler, this, + recursion_count, + boost::asio::placeholders::error ) ); LongtermTimerIsActive = true; // for now, admit failure bool was_success = false; - finalize_resolve(was_success); + finalize_resolve(was_success, recursion_count); } -void DnsResolver::handle_ips(const std::vector &result_ips) +void DnsResolver::handle_ips(const int recursion_count, + const std::vector &result_ips) { - // received at least one IP which could be for the queried host name + // received at least one IP which could be for the queried host name // or the cname at the "end" of the cname list; // but all IPs should be for the same HostAddressVec addr_list; @@ -398,16 +403,18 @@ void DnsResolver::handle_ips(const std::vector &result_ips) // clean up bool was_success = true; - finalize_resolve(was_success); + finalize_resolve(was_success, recursion_count); } -void DnsResolver::handle_cname(const std::vector &result_cnames) +void DnsResolver::handle_cname(const int recursion_count, + const std::vector &result_cnames) { // find the "last" cname in the list // Hostname --> cname1 --> cname2 --> ... --> cnameN // We assume here that this list might not be in order but that all cnames - // form a single list (form one connected list and not several isolated) + // form a single list (form one connected list and not several independent + // lists) std::string last_cname = ""; bool could_be_last; @@ -438,7 +445,7 @@ void DnsResolver::handle_cname(const std::vector &result_cnames) BOOST_FOREACH( const src_cname_pair &host_and_cname, result_cnames ) GlobalLogger.info() << LogPrefix << host_and_cname.first << " --> " << host_and_cname.second.Host; - handle_unavailable(); + handle_unavailable(recursion_count); } else { // check cache for IP for this cname @@ -448,8 +455,7 @@ void DnsResolver::handle_cname(const std::vector &result_cnames) if ( !cached_data.empty() ) { bool was_success = true; - int cname_count = 1; // define cache access as only 1 - finalize_resolve(was_success, cname_count); + finalize_resolve(was_success, recursion_count+1); } else { // get resolver for canonical name @@ -458,7 +464,7 @@ void DnsResolver::handle_cname(const std::vector &result_cnames) callback_type callback = boost::bind( &DnsResolver::cname_resolve_callback, this, _1, _2 ); - resolver->async_resolve( callback ); + resolver->async_resolve( callback, recursion_count+1 ); // treat a CNAME as a partial result: not enough to run callbacks // from finalize_resolve, but enough to stop timers and reset @@ -469,8 +475,12 @@ void DnsResolver::handle_cname(const std::vector &result_cnames) } +/** + * the recursion_count here is really the one from the recursion, not the one + * forwarded from async_resolve! + */ void DnsResolver::cname_resolve_callback(const bool was_success, - const int cname_count) + const int recursion_count) { if ( OperationCancelled ) { // async_resolve was cancelled --> callbacks already called @@ -480,22 +490,35 @@ void DnsResolver::cname_resolve_callback(const bool was_success, } else if (was_success) { - GlobalLogger.debug() << LogPrefix << "CNAME resolution succeeded"; - finalize_resolve(was_success, cname_count+1); + GlobalLogger.debug() << LogPrefix << "CNAME resolution succeeded after " + << recursion_count << " recursions"; + finalize_resolve(was_success, recursion_count); } else { - GlobalLogger.info() << LogPrefix << "CNAME resolution failed"; + GlobalLogger.info() << LogPrefix << "CNAME resolution failed after " + << recursion_count << " recursions"; // no use to schedule retry in this case since cname resolver must have // failed several times and we can only re-start the same procedure with // the same information. But can re-try later - handle_unavailable(); + handle_unavailable(recursion_count); } } +/** + * @brief always called at end of resolving process + * + * runs callbacks, resets timers and checks state consistency; only thing that + * is "left alive" is the long-term timer that might cause a re-start of + * resolution after a while + * + * @param was_success: indicates whether resolution was successfull + * @param recursion_count number of recursions or (if not successfull) negative + * value indicating who called this function + */ void DnsResolver::finalize_resolve(const bool was_success, - const int cname_count) + const int recursion_count) { // some consistency checks; failure might indicate a situation I had not // anticipated during programming but might not be harmfull yet @@ -516,12 +539,12 @@ void DnsResolver::finalize_resolve(const bool was_success, stop_trying(was_success); // schedule callbacks, clearing callback list - ResolverBase::schedule_callbacks(was_success, cname_count); + ResolverBase::schedule_callbacks(was_success, recursion_count); // finalize GlobalLogger.notice() << LogPrefix << "finalized resolve" << " with success = " << was_success - << " and cname_count = " << cname_count; + << " and recursion_count = " << recursion_count; IsResolving = false; } @@ -554,7 +577,7 @@ void DnsResolver::stop_trying(bool was_success) * Is true from call to async_resolve until callbacks * --> returns true if waiting for result or (short-term) retry * - * However, does NOT tell you if the (long-term) stale timeout is active! + * However, does NOT tell you if the (long-term) stale timeout is active! * That timer has no effect on result, need to check is_waiting_to_resolve * for that */ @@ -566,7 +589,7 @@ bool DnsResolver::is_resolving() const /** * returns true if either is_resolving or the long-term timer is active - * + * * is_resolving returns true if the short-term retry timer is active */ bool DnsResolver::is_waiting_to_resolve() const @@ -606,22 +629,23 @@ void DnsResolver::cancel_resolve() if ( IsResolving ) { bool was_success = false; - int cname_count = 1; - finalize_resolve(was_success, cname_count); + int recursion_count = -1; + finalize_resolve(was_success, recursion_count); } // also cancel the long-term timer StaleDataLongtermTimer.cancel(); LongtermTimerIsActive = false; - // set after finalize_resolve, so can check in finalize_resolve that + // set after finalize_resolve, so can check in finalize_resolve that // OperationCancelled is never true OperationCancelled = true; } -void DnsResolver::handle_resolve_timeout(const boost::system::error_code &error) +void DnsResolver::handle_resolve_timeout(const int recursion_count, + const boost::system::error_code &error) { if ( error == boost::asio::error::operation_aborted ) // cancelled { @@ -634,7 +658,7 @@ void DnsResolver::handle_resolve_timeout(const boost::system::error_code &error) GlobalLogger.warning() << LogPrefix << "resolve timeout handler received error " << error << " --> request retry"; - schedule_retry(); + schedule_retry(recursion_count); } else if ( OperationCancelled ) { // async_resolve was cancelled --> callbacks already called @@ -645,12 +669,12 @@ void DnsResolver::handle_resolve_timeout(const boost::system::error_code &error) else { GlobalLogger.notice() << LogPrefix << "DNS resolving timed out"; - schedule_retry(); + schedule_retry(recursion_count); } } -void DnsResolver::schedule_retry() +void DnsResolver::schedule_retry(const int recursion_count) { // cancel timers ResolveTimeoutTimer.cancel(); @@ -664,8 +688,8 @@ void DnsResolver::schedule_retry() { // too many re-tries GlobalLogger.info() << LogPrefix << "Not scheduling a retry since " << "RetryCount " << RetryCount << " too high"; - handle_unavailable(); // will call stop_trying i.e. reset RetryCount - } + handle_unavailable(recursion_count); // will call stop_trying + } // --> reset RetryCount else { // schedule retry GlobalLogger.info() << LogPrefix << "Scheduling a retry (RetryCount=" @@ -674,11 +698,12 @@ void DnsResolver::schedule_retry() seconds(Config::PauseBeforeRetrySeconds)); PauseBeforeRetryTimer.async_wait( boost::bind( &DnsResolver::wait_timer_timeout_handler, - this, boost::asio::placeholders::error) ); + this, recursion_count, + boost::asio::placeholders::error) ); } } -void DnsResolver::wait_timer_timeout_handler( +void DnsResolver::wait_timer_timeout_handler( const int recursion_count, const boost::system::error_code &error) { if ( error == boost::asio::error::operation_aborted ) // cancelled @@ -694,7 +719,7 @@ void DnsResolver::wait_timer_timeout_handler( << "resolve wait handler received error " << error << "! Try to finalize resolve"; bool was_success = false; - finalize_resolve(was_success); + finalize_resolve(was_success, recursion_count); } else if ( OperationCancelled ) { // async_resolve was cancelled --> callbacks already called @@ -706,7 +731,7 @@ void DnsResolver::wait_timer_timeout_handler( { GlobalLogger.info() << LogPrefix << "Done waiting --> re-try resolve"; IsResolving = false; // will be set to true immediately in do_resolve - do_resolve(); + do_resolve(recursion_count); } } diff --git a/src/dns/dnsresolver.h b/src/dns/dnsresolver.h index 0918fd0..f2d45c3 100644 --- a/src/dns/dnsresolver.h +++ b/src/dns/dnsresolver.h @@ -70,22 +70,27 @@ public: // implementation of ResolverBase::async_resolve protected: - void do_resolve(); + void do_resolve(const int recursion_count); // internal helper functions private: - void handle_resolve_timeout(const boost::system::error_code &error); - void handle_dns_result(const boost::system::error_code &error, + void handle_resolve_timeout(const int recursion_count, + const boost::system::error_code &error); + void handle_dns_result(const int recursion_count, + const boost::system::error_code &error, const std::size_t bytes_transferred); - void handle_unavailable(); - void handle_ips(const std::vector &result_ips); - void handle_cname(const std::vector &result_cnames); + void handle_unavailable(const int recursion_count); + void handle_ips(const int recursion_count, + const std::vector &result_ips); + void handle_cname(const int recursion_count, + const std::vector &result_cnames); void cname_resolve_callback(const bool was_success, - const int cname_count); - void schedule_retry(); - void finalize_resolve(const bool success, const int cname_count=0); + const int recursion_count); + void schedule_retry(const int recursion_count); + void finalize_resolve(const bool success, const int recursion_count); void stop_trying(const bool was_success); - void wait_timer_timeout_handler(const boost::system::error_code &error); + void wait_timer_timeout_handler(const int recursion_count, + const boost::system::error_code &error); void gather_results( const boost::net::dns::rr_list_t *rr_list, std::vector *result_ips, std::vector *result_cnames, diff --git a/src/dns/ippseudoresolver.h b/src/dns/ippseudoresolver.h index eeac609..a256a06 100644 --- a/src/dns/ippseudoresolver.h +++ b/src/dns/ippseudoresolver.h @@ -74,7 +74,7 @@ public: // implementation of ResolverBase::async_resolve protected: - void do_resolve() + void do_resolve(const int recursion_count) { ResolverBase::schedule_callbacks(true, 0); } }; #endif diff --git a/src/dns/resolverbase.cpp b/src/dns/resolverbase.cpp index 9f36e68..362b3b9 100644 --- a/src/dns/resolverbase.cpp +++ b/src/dns/resolverbase.cpp @@ -25,6 +25,10 @@ #include +#include + +using I2n::Logger::GlobalLogger; + ResolverBase::~ResolverBase() { } @@ -32,15 +36,32 @@ ResolverBase::~ResolverBase() /** * callbacks should be of type * void resolve_callback(const bool was_success, - * const int cname_count) + * const int recursion_count) */ -void ResolverBase::async_resolve(const callback_type &callback) +void ResolverBase::async_resolve(const callback_type &callback, + const int recursion_count) { - // remember callback - CallbackList.push(callback); + // limit depth of recursion + if (recursion_count >= DnsMaster::get_instance()->get_max_recursion_count()) + { // if recursed too much, schedule callback with was_success = false + bool was_success = false; + IoService->post( boost::bind(callback, was_success, recursion_count) ); + } + else + { + // remember callback + CallbackList.push(callback); + + // check for trouble + if (CallbackList.size() > 10) + GlobalLogger.warning() << "ResolverBase: have " + << CallbackList.size() << " callback[s] listed for " << Hostname + << "!"; + + // let subclass do the resolving + do_resolve(recursion_count); + } - // let subclass do the resolving - do_resolve(); } ResolverBase::ResolverBase(const IoServiceItem &io_serv, @@ -92,14 +113,13 @@ HostAddressVec ResolverBase::get_cached_ips_recursively(const std::string host, } void ResolverBase::schedule_callbacks(const bool was_success, - const int cname_count) + const int recursion_count) { while ( !CallbackList.empty() ) { callback_type callback = CallbackList.front(); CallbackList.pop(); - IoService->post( boost::bind( callback, - was_success, cname_count ) ); + IoService->post( boost::bind(callback, was_success, recursion_count) ); } } diff --git a/src/dns/resolverbase.h b/src/dns/resolverbase.h index 8262124..5e1fb6f 100644 --- a/src/dns/resolverbase.h +++ b/src/dns/resolverbase.h @@ -49,9 +49,10 @@ public: /** * callbacks should be of type * void resolve_callback(const bool was_success, - * const int cname_count) + * const int recursion_count) */ - void async_resolve(const callback_type &callback); + void async_resolve(const callback_type &callback, + const int recursion_count=0); virtual void cancel_resolve() = 0; virtual bool is_resolving() const = 0; virtual bool is_waiting_to_resolve() const = 0; @@ -78,7 +79,9 @@ protected: // functions for subclasses protected: - virtual void do_resolve() = 0; + // recursion_count is guaranteed to be below + // DnsMaster::get_max_recursion_count + virtual void do_resolve(const int recursion_count) = 0; // convenient forwards to DnsCache that just insert Hostname void update_cache( const HostAddressVec &new_ips ) const; @@ -92,7 +95,7 @@ protected: const bool check_up_to_date=false) const; void schedule_callbacks(const bool was_success, - const int cname_count); + const int recursion_count); }; diff --git a/src/host/pingscheduler.cpp b/src/host/pingscheduler.cpp index 430b5e0..b72dcba 100644 --- a/src/host/pingscheduler.cpp +++ b/src/host/pingscheduler.cpp @@ -410,11 +410,11 @@ void PingScheduler::start_resolving_ping_address() } void PingScheduler::dns_resolve_callback(const bool was_success, - const int cname_count) + const int recursion_count) { GlobalLogger.info() << LogPrefix << "dns resolution finished " << "with success = " << was_success << " " - << "and cname_count = " << cname_count; + << "after " << recursion_count << " recursions"; if ( was_success ) { diff --git a/src/host/pingscheduler.h b/src/host/pingscheduler.h index 0de5711..106f69c 100644 --- a/src/host/pingscheduler.h +++ b/src/host/pingscheduler.h @@ -82,7 +82,8 @@ private: void update_dns_resolver( PingProtocol current_protocol ); void ping_when_ready(); - void dns_resolve_callback(const bool was_success, const int cname_count); + void dns_resolve_callback(const bool was_success, + const int recursion_count); void start_resolving_ping_address(); void update_log_prefix(); diff --git a/src/main.cpp b/src/main.cpp index 5fffb50..d080334 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -513,12 +513,14 @@ int main( int argc, const char *argv[] ) boost::asio::ip::address::from_string( configuration->get_nameserver() ); + int max_recursion_count = 10; DnsMaster::create_master( io_service, name_server_ip, configuration->get_resolved_ip_ttl_threshold(), configuration->get_min_time_between_resolves(), configuration->get_max_address_resolution_attempts(), + max_recursion_count, configuration->get_dns_cache_file() ); init_pingers( io_service, configuration, status_notifier, &scheduler_list ); diff --git a/test/data/icmp_destinationunreachable.pcap b/test/data/icmp_destinationunreachable.pcap index 550a6e2c807c61415de4017fb08118b0c8b29be9..b1309f6e06c1388238903640a527d4b08729aac6 100644 GIT binary patch delta 42 tcmZ3+xQuavlLEJx;y)nPVP^jGhXDv&85lxNo^W8WXVl3CiBC*&0RZo*3ex}p delta 42 tcmZ3+xQuavlfoJS#(zNE%gp@e4+9XmGBAXkJmJ7#P5}%ml0stSq4F&)J diff --git a/test/data/icmp_echoreply.pcap b/test/data/icmp_echoreply.pcap index 78167f0d49943d52b0dff422cb75a22eca6ce84e..8dd78196f65fbcf3ddca9307c079bb0d6d02d90f 100644 GIT binary patch delta 12 TcmYdEnc&94EvBe5(Mt*d6~zMv delta 12 TcmYdEnc&8h-Q3jT;>7e+0JEYC0ssI2 diff --git a/test/data/icmp_timeexceeded.pcap b/test/data/icmp_timeexceeded.pcap index b20f769912981f2eec343e44747c6661b52014b3..2f60c5070354ef6a69886bb6d142bd3be4a3b7f4 100644 GIT binary patch delta 57 zcmb=bn&4)_EvBf$&G6+v0}!|}Flg{`J1{UZY6pPCZ`|6&#KDlT*y}L^V?kzKx^8Z2 JadBdLDgckz4}$;z delta 57 zcmb=bn&4)#Mu4%Go8ilU1|V={V9?;>c3@y+)D8fN-?+7liGv|wvDae;#)8beblu$4 J;^M^gQ~=W65jy|? diff --git a/test/test_dns.cpp b/test/test_dns.cpp index 8273aee..11e7e9d 100644 --- a/test/test_dns.cpp +++ b/test/test_dns.cpp @@ -39,17 +39,18 @@ on this file might be covered by the GNU General Public License. using boost::asio::ip::address; +// constants for master +address name_server = address::from_string("127.0.0.1"); +const int resolved_ip_ttl_threshold = 5; // this should be greater than... +const uint32_t min_time_between_resolves = 3; // ... this! +const int max_address_resolution_attempts = 2; +const int max_recursion_count = 10; +const std::string cache_file = DnsCache::DoNotUseCacheFile; + //------------------------------------------------------------------------------ // Global Test fixture (created once for test suite) //------------------------------------------------------------------------------ -// constants for master -address name_server = address::from_string("127.0.0.1"); -int resolved_ip_ttl_threshold = 5; -uint32_t min_time_between_resolves = 3; // should be < resolved_ip_ttl_threshold -int max_address_resolution_attempts = 2; -std::string cache_file = DnsCache::DoNotUseCacheFile; - // rely on boost that there will only be one instance struct GlobalFixture; GlobalFixture *global_fixture; @@ -87,6 +88,7 @@ struct GlobalFixture name_server, resolved_ip_ttl_threshold, max_address_resolution_attempts, + max_recursion_count, Cache); Master = DnsMaster::get_instance(); @@ -162,6 +164,13 @@ struct GlobalFixture Cname("host_ttl_below_thresh.test", 2) ); } + { // create a CNAME loop cname_loop1.test --> cname_loop2.test + // --> cname_loop3.test --> cname_loop1.test + Cache->update( "cname_loop1.test", Cname("cname_loop2.test", 60) ); + Cache->update( "cname_loop2.test", Cname("cname_loop3.test", 60) ); + Cache->update( "cname_loop3.test", Cname("cname_loop1.test", 60) ); + } + BOOST_TEST_MESSAGE( "Done filling cache." ); } @@ -390,7 +399,6 @@ BOOST_AUTO_TEST_CASE( cache_outdated_test ) BOOST_AUTO_TEST_CASE( cache_ttl_below_thresh_test ) { - // in fill_cache, HostAddressVec ips = Cache->get_ips("host_ttl_below_thresh.test", false); BOOST_REQUIRE_EQUAL( ips.size(), 1 ); HostAddress ip = ips.front(); @@ -400,6 +408,14 @@ BOOST_AUTO_TEST_CASE( cache_ttl_below_thresh_test ) BOOST_CHECK_EQUAL( cname.Ttl.get_value(), min_time_between_resolves ); } +BOOST_AUTO_TEST_CASE( cache_retrieve_loop ) +{ + BOOST_CHECK_EQUAL( Cache->get_ips_recursive("cname_loop1.test").size(), 0 ); + BOOST_CHECK_EQUAL( Cache->get_ips_recursive("cname_loop2.test").size(), 0 ); + BOOST_CHECK_EQUAL( Cache->get_ips_recursive("cname_loop3.test").size(), 0 ); +} + + BOOST_AUTO_TEST_SUITE_END() // of TestDnsCache @@ -432,16 +448,35 @@ BOOST_AUTO_TEST_CASE( create_resolver_v4 ) } -void resolve_callback(IoServiceItem io_serv, ResolverItem resolver, - const bool was_success, const int cname_count) +struct ResolveCallback { - resolver->cancel_resolve(); - io_serv->stop(); - BOOST_TEST_MESSAGE( "Stopped io_service" ); - BOOST_CHECK( was_success ); - BOOST_CHECK_EQUAL( cname_count, 0 ); -} +public: + IoServiceItem IoService; + ResolverItem Resolver; + int CallCount; + bool LastCallWasSuccess; + int LastCallRecursionCount; + + ResolveCallback(IoServiceItem &io_serv, ResolverItem &resolver) + : IoService(io_serv) + , Resolver(resolver) + , CallCount(0) + , LastCallWasSuccess(true) + , LastCallRecursionCount(-10) + {} + + void call(const bool was_success, const int recursion_count) + { + BOOST_TEST_MESSAGE( "Callback called" ); + ++CallCount; + Resolver->cancel_resolve(); + IoService->stop(); + + LastCallWasSuccess = was_success; + LastCallRecursionCount = recursion_count; + } +}; BOOST_AUTO_TEST_CASE( do_resolve ) { @@ -453,16 +488,26 @@ BOOST_AUTO_TEST_CASE( do_resolve ) BOOST_CHECK( !resolver->is_waiting_to_resolve() ); // set callback - callback_type callback = boost::bind( resolve_callback, IoService, resolver, + ResolveCallback *callback_obj = new ResolveCallback(IoService, resolver); + // have to manually delete this obj in the end! + callback_type callback = boost::bind( &ResolveCallback::call, callback_obj, _1, _2 ); + // start resolving resolver->async_resolve(callback); BOOST_CHECK( resolver->is_resolving() ); IoService->run(); // this will block until io service is stopped in resolve_callback + BOOST_TEST_MESSAGE( "Back to synchronous operation" ); + IoService->reset(); // check for result + BOOST_CHECK_EQUAL( callback_obj->CallCount, 1 ); + BOOST_CHECK( callback_obj->LastCallWasSuccess ); + BOOST_CHECK_EQUAL( callback_obj->LastCallRecursionCount, 0 ); BOOST_CHECK( resolver->have_up_to_date_ip() ); + + delete callback_obj; } BOOST_AUTO_TEST_SUITE_END() // of TestDnsResolver diff --git a/test/test_icmppacket.cpp b/test/test_icmppacket.cpp index a8ea85d..0638e5f 100644 --- a/test/test_icmppacket.cpp +++ b/test/test_icmppacket.cpp @@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE( EchoRequest ) BOOST_REQUIRE( packet ); BOOST_CHECK_EQUAL( packet->get_type_v4(), Icmpv4Type_EchoRequest ); BOOST_CHECK_EQUAL( packet->get_source_address().to_string(), - "172.16.1.141"); + "11.22.33.44"); BOOST_CHECK_EQUAL( packet->get_destination_address().to_string(), "38.100.128.10"); BOOST_CHECK_EQUAL( packet->get_icmp_code(), 0 ); @@ -104,7 +104,7 @@ BOOST_AUTO_TEST_CASE( EchoReply ) BOOST_CHECK_EQUAL( packet->get_source_address().to_string(), "38.100.128.10"); BOOST_CHECK_EQUAL( packet->get_destination_address().to_string(), - "172.16.1.141"); + "11.22.33.44"); BOOST_CHECK_EQUAL( packet->get_icmp_code(), 0 ); BOOST_CHECK_EQUAL( packet->get_icmp_size(), 20 ); BOOST_CHECK( packet->match_echo_reply(0xaf36, 1, @@ -122,7 +122,7 @@ BOOST_AUTO_TEST_CASE( TimeExceeded ) BOOST_CHECK_EQUAL( packet->get_source_address().to_string(), "193.186.7.1"); BOOST_CHECK_EQUAL( packet->get_destination_address().to_string(), - "172.16.1.141"); + "11.22.33.44"); BOOST_CHECK_EQUAL( packet->get_icmp_code(), 0 ); BOOST_CHECK_EQUAL( packet->get_icmp_size(), 48 ); BOOST_CHECK( packet->match_time_exceeded(0x4ae3, 1, @@ -138,9 +138,9 @@ BOOST_AUTO_TEST_CASE( DestinationUnreachable ) BOOST_REQUIRE( packet ); BOOST_CHECK_EQUAL( packet->get_type_v4(), Icmpv4Type_DestinationUnreachable ); BOOST_CHECK_EQUAL( packet->get_source_address().to_string(), - "172.16.1.254"); + "11.22.33.254"); BOOST_CHECK_EQUAL( packet->get_destination_address().to_string(), - "172.16.1.141"); + "11.22.33.44"); BOOST_CHECK_EQUAL( packet->get_icmp_code(), 3 ); BOOST_CHECK_EQUAL( packet->get_icmp_size(), 92 ); BOOST_CHECK( packet->match_destination_unreachable(0x077b, 1, -- 1.7.1