Merge pull request #1534 in SNORT/snort3 from ~SMINUT/snort3:appid_service_cache...

Squashed commit of the following:

commit 534af2b020c63e959f728167b5a984b00029de03
Author: Silviu Minut <sminut@cisco.com>
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 f1b920f194a896ca23c53241adc5b22b4db1b253..b3fdde38e22335a9aba4023d88e90c6c7f7842e2 100644 (file)
--- 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 <map>
+#include <cstring>
 
 #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 40ee2ccfff9f943eb74c8e81f4a527c91d12c546..eeb01dd8a446730af5cd70a262da083d1efdc30f 100644 (file)
--- 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<Map_t::iterator, bool> sit = m.emplace( std::make_pair(k, ss) );
+        Map_t::iterator it = sit.first;
 
-            std::pair<Map_t::iterator, bool> 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 5790bb427d613e6ea2c5eab17a6f3f13952785ac..09fc20ea8197893acc8e6bf008bb1a1a2108be84 100644 (file)
--- 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<B ) and !( B<A ),
+    // because then map will consider A=B.
+    CHECK_TRUE(A<B || B<A);
+}
+
 TEST(service_state_tests, service_cache)
 {
     size_t num_entries = 10, max_entries = 3;
@@ -193,21 +209,25 @@ TEST(service_state_tests, service_cache)
 
     IpProtocol proto = IpProtocol::TCP;
     uint16_t port = 3000;
-    SfIp ip;
-    ip.set("10.10.0.1");
+    SfIp ip4, ip6;
+    ip4.set("1.2.3.4");
+    ip6.set("1111.2222.3333.4444.5555.6666.7777.8888");
 
     Val_t* ss = nullptr;
     std::vector<Val_t*> 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<Val_t*>::iterator vit = --ssvec.end();
     for( size_t i=0; i<max_entries; i++, --it, --vit )
@@ -215,10 +235,10 @@ TEST(service_state_tests, service_cache)
         Map_t::iterator mit = *it;
         CHECK_TRUE( mit->second == *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 8aa600bd269fe4d211803228284dfccfc4ae54e5..b0fde364bcb26f71bafee057f7e6b3c81352e16b 100644 (file)
--- 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
-