From: Mike Stepanek (mstepane) Date: Thu, 7 Mar 2019 16:10:21 +0000 (-0500) Subject: Merge pull request #1534 in SNORT/snort3 from ~SMINUT/snort3:appid_service_cache... X-Git-Tag: 3.0.0-251~28 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f961ce70d261a0694b6557cf88252f2d1b0fbc7a;p=thirdparty%2Fsnort3.git Merge pull request #1534 in SNORT/snort3 from ~SMINUT/snort3:appid_service_cache to master Squashed commit of the following: commit 534af2b020c63e959f728167b5a984b00029de03 Author: Silviu Minut Date: Mon Mar 4 16:55:38 2019 -0500 appid: fix AppIdServiceStateKey::operator<(). appid: replace the custom AppIdServiceCacheKey::operator< with memcmp in both service_state.h and host_port_app_cache.cc. appid: get rid of the map::find() in MapList::add(), just try to emplace directly. appid: pass HostPortKey by reference in HostPortKey::operator<(). appid: add unit test to make sure the AppIdServiceStateKey::operator<() is OK and modify existing service cache memcap test to alternate ipv4 and ipv6 addresses. sfip: add a FIXIT for checking that the current implementation of _is_lesser(), which only compares same-family ips is OK. --- diff --git a/src/network_inspectors/appid/host_port_app_cache.cc b/src/network_inspectors/appid/host_port_app_cache.cc index f1b920f19..b3fdde38e 100644 --- a/src/network_inspectors/appid/host_port_app_cache.cc +++ b/src/network_inspectors/appid/host_port_app_cache.cc @@ -26,6 +26,7 @@ #include "host_port_app_cache.h" #include +#include #include "log/messages.h" #include "main/thread.h" @@ -45,23 +46,9 @@ struct HostPortKey padding = 0; } - bool operator<(HostPortKey right) const + bool operator<(const HostPortKey& right) const { - if ( ip.less_than(right.ip) ) - return true; - else if ( right.ip.less_than(ip) ) - return false; - else - { - if ( port < right.port) - return true; - else if ( right.port < port ) - return false; - else if ( proto < right.proto) - return true; - else - return false; - } + return memcmp((const uint8_t*) this, (const uint8_t*) &right, sizeof(*this)) < 0; } SfIp ip; diff --git a/src/network_inspectors/appid/service_state.h b/src/network_inspectors/appid/service_state.h index 40ee2ccff..eeb01dd8a 100644 --- a/src/network_inspectors/appid/service_state.h +++ b/src/network_inspectors/appid/service_state.h @@ -181,27 +181,9 @@ public: padding[0] = padding[1] = padding[2] = 0; } - bool operator<(AppIdServiceStateKey right) const + bool operator<(const AppIdServiceStateKey& right) const { - if ( ip.less_than(right.ip) ) - return true; - else if ( right.ip.less_than(ip) ) - return false; - else - { - if ( port < right.port ) - return true; - else if ( right.port < port ) - return false; - else if ( proto < right.proto ) - return true; - else if ( right.proto < proto ) - return false; - else if ( level < right.level ) - return true; - else - return false; - } + return memcmp((const uint8_t*) this, (const uint8_t*) &right, sizeof(*this)) < 0; } private: @@ -229,26 +211,28 @@ public: { Val_t* ss = nullptr; - Map_t::iterator it = m.find(k); - if ( it == m.end() ) - { - // Prune the map to make room for the new sds if memcap is hit - if ( mem_used + sz > memcap ) - remove( q.front() ); - - ss = new Val_t; + // Try to emplace k first, with a nullptr. + std::pair sit = m.emplace( std::make_pair(k, ss) ); + Map_t::iterator it = sit.first; - std::pair sit = m.emplace(std::make_pair(k,ss)); - q.emplace_back(sit.first); + if ( sit.second ) + { + // emplace succeeded + ss = it->second = new Val_t; + q.emplace_back(it); mem_used += sz; ss->qptr = --q.end(); // remember our place in the queue + + if ( mem_used > memcap ) + remove( q.front() ); } - else { + else + { ss = it->second; if ( do_touch ) - touch(ss->qptr); + touch( ss->qptr ); } - + return ss; } diff --git a/src/network_inspectors/appid/test/service_state_test.cc b/src/network_inspectors/appid/test/service_state_test.cc index 5790bb427..09fc20ea8 100644 --- a/src/network_inspectors/appid/test/service_state_test.cc +++ b/src/network_inspectors/appid/test/service_state_test.cc @@ -185,6 +185,22 @@ TEST(service_state_tests, set_service_id_failed_with_valid) CHECK_TRUE(sds.get_state() == SERVICE_ID_STATE::VALID); } +TEST(service_state_tests, appid_service_state_key_comparison_test) +{ + SfIp ip4, ip6; + ip4.set("1.2.3.4"); + ip6.set("1111.2222.3333.4444.5555.6666.7777.8888"); + IpProtocol proto = IpProtocol::TCP; + uint16_t port=3000; + + Key_t A(&ip4, proto, port, 0); + Key_t B(&ip6, proto, port, 0); + + // We must never be in a situation where !( A ssvec; - - // Insert past the memcap, and check the memcap is not exceeded: + + + // Insert (ipv4 and ipv6) past the memcap, and check the memcap is not exceeded. for( size_t i = 1; i <= num_entries; i++, port++ ) { - ss = ServiceCache.add( Key_t(&ip, proto, port, 0) ); + const SfIp* ip = ( i%2 == 1 ? &ip4 : &ip6 ); + ss = ServiceCache.add( Key_t(ip, proto, port, 0) ); CHECK_TRUE(ServiceCache.size() == ( i <= max_entries ? i : max_entries)); ssvec.push_back(ss); } - // The cache should now be port 8, 9, 10 + // The cache should now be ip6:3007, ip4:3008, ip6:3009. + // Check that the order in the cache is correct. Queue_t::iterator it = ServiceCache.newest(); std::vector::iterator vit = --ssvec.end(); for( size_t i=0; isecond == *vit ); } - - // Now get an entry in the cache and check that it got touched: - port -= 1; - ss = ServiceCache.get( Key_t(&ip, proto, port, 0) ); + + // Now get e.g. the oldest from the cache and check that it got touched: + it = ServiceCache.oldest(); + ss = ServiceCache.get( (*it)->first, true ); CHECK_TRUE( ss != nullptr ); CHECK_TRUE( ss->qptr == ServiceCache.newest() ); } diff --git a/src/sfip/sf_ip.h b/src/sfip/sf_ip.h index 8aa600bd2..b0fde364b 100644 --- a/src/sfip/sf_ip.h +++ b/src/sfip/sf_ip.h @@ -252,6 +252,10 @@ inline bool SfIp::_is_equals(const SfIp& rhs) const return false; } +// FIXIT-L: when comparing ip4 vs ip6 we have !(ip4 < ip6) and !(ip6 < ip4). +// This may be OK in some cases, but will not work e.g. on a binary tree +// (stl::map) with SfIp as keys, whose implementation relies only on "<". +// This affects SfIp::less_than() and SfIp::greater_than(). inline bool SfIp::_is_lesser(const SfIp& rhs) const { if (is_ip4()) @@ -474,4 +478,3 @@ inline std::ostream& operator<<(std::ostream& os, const SfIp* addr) SO_PUBLIC const char* snort_inet_ntop(int family, const void* ip_raw, char* buf, int bufsize); } // namespace snort #endif -