Merge pull request #2864 in SNORT/snort3 from ~DIPANDIT/snort3:smb_deadlock_main...

Squashed commit of the following:

commit 0e71ce321233e6c850d5fb2af7d0ec7e9b854091
Author: Dipto Pandit (dipandit) <dipandit@cisco.com>
Date:   Mon Apr 26 03:17:22 2021 -0400

    dce_rpc: use find_else_insert in smb session cache to avoid deadlock

commit 247e355e2d1ea43051f5e2e508857c4227dc29e3
Author: Dipto Pandit (dipandit) <dipandit@cisco.com>
Date:   Mon Apr 26 02:44:30 2021 -0400

    hash: add new insert method in lru_cache_shared

diff --git a/src/hash/lru_cache_shared.h b/src/hash/lru_cache_shared.h
index ac7fbcc59ea30e414385de9875f471b3cd6312a2..459b42ec39a1e9b8f7422842cb1e3341742f39fb 100644 (file)
--- a/src/hash/lru_cache_shared.h
+++ b/src/hash/lru_cache_shared.h
@@ -48,6 +48,12 @@ struct LruCacheSharedStats
     PegCount replaced = 0;      // found entry and replaced it
 };
 
+enum class LcsInsertStatus {
+    LCS_ITEM_PRESENT,
+    LCS_ITEM_INSERTED,
+    LCS_ITEM_REPLACED
+};
+
 template<typename Key, typename Value, typename Hash, typename Eq = std::equal_to<Key>,
     typename Purgatory = std::vector<std::shared_ptr<Value>>>
 class LruCacheShared
@@ -82,6 +88,9 @@ public:
     // Returns true if found or replaced, takes a ref to a user managed entry
     bool find_else_insert(const Key& key, std::shared_ptr<Value>& data, bool replace = false);
 
+    // Returns the found or inserted data, takes a ref to user managed entry.
+    Data find_else_insert(const Key&, Data&, LcsInsertStatus*, bool = false);
+
     // Return all data from the LruCache in order (most recently used to least)
     std::vector<std::pair<Key, Data> > get_all_data();
 
@@ -243,7 +252,6 @@ find_else_create(const Key& key, bool* new_data)
     // return the data pointer (below), or else, some other thread might
     // delete it before we got a chance to return it.
     Purgatory tmp_data;
-    Data data = Data(new Value);
 
     std::lock_guard<std::mutex> cache_lock(cache_mutex);
 
@@ -259,6 +267,7 @@ find_else_create(const Key& key, bool* new_data)
     stats.adds++;
     if ( new_data )
         *new_data = true;
+    Data data = Data(new Value);
 
     //  Add key/data pair to front of list.
     list.emplace_front(std::make_pair(key, data));
@@ -313,6 +322,50 @@ find_else_insert(const Key& key, std::shared_ptr<Value>& data, bool replace)
     return false;
 }
 
+template<typename Key, typename Value, typename Hash, typename Eq, typename Purgatory>
+std::shared_ptr<Value> LruCacheShared<Key, Value, Hash, Eq, Purgatory>::
+find_else_insert(const Key& key, std::shared_ptr<Value>& data, LcsInsertStatus* status, bool replace)
+{
+    LruMapIter map_iter;
+
+    Purgatory tmp_data;
+    std::lock_guard<std::mutex> cache_lock(cache_mutex);
+
+    map_iter = map.find(key);
+    if (map_iter != map.end())
+    {
+        stats.find_hits++;
+        if (status) *status = LcsInsertStatus::LCS_ITEM_PRESENT;
+        if (replace)
+        {
+            // Explicitly calling the reset so its more clear that destructor could be called for the object
+            decrease_size(map_iter->second->second.get());
+            map_iter->second->second.reset();
+            map_iter->second->second = data;
+            increase_size(map_iter->second->second.get());
+            stats.replaced++;
+            if (status) *status = LcsInsertStatus::LCS_ITEM_REPLACED;
+        }
+        list.splice(list.begin(), list, map_iter->second); // update LRU
+        return map_iter->second->second;
+    }
+
+    stats.find_misses++;
+    stats.adds++;
+    if (status) *status = LcsInsertStatus::LCS_ITEM_INSERTED;
+
+    //  Add key/data pair to front of list.
+    list.emplace_front(std::make_pair(key, data));
+    increase_size(data.get());
+
+    //  Add list iterator for the new entry to map.
+    map[key] = list.begin();
+
+    prune(tmp_data);
+
+    return data;
+}
+
 template<typename Key, typename Value, typename Hash, typename Eq, typename Purgatory>
 std::vector< std::pair<Key, std::shared_ptr<Value>> >
 LruCacheShared<Key, Value, Hash, Eq, Purgatory>::get_all_data()

diff --git a/src/hash/test/lru_cache_shared_test.cc b/src/hash/test/lru_cache_shared_test.cc
index c2777498a7a2801a23c9932219c890e42390a56f..acbc9e6dd00b44238a67b7fc9757a474aae12387 100644 (file)
--- a/src/hash/test/lru_cache_shared_test.cc
+++ b/src/hash/test/lru_cache_shared_test.cc
@@ -147,13 +147,28 @@ TEST(lru_cache_shared, remove_test)
 TEST(lru_cache_shared, find_else_insert)
 {
     std::shared_ptr<std::string> data(new std::string("12345"));
-    LruCacheShared<int, std::string, std::hash<int> > lru_cache(1);
+    std::shared_ptr<std::string> data2(new std::string("54321"));
+    LruCacheShared<int, std::string, std::hash<int> > lru_cache(2);
+    LcsInsertStatus status;
 
     CHECK(false == lru_cache.find_else_insert(1,data));
     CHECK(1 == lru_cache.size());
 
     CHECK(true == lru_cache.find_else_insert(1,data));
     CHECK(1 == lru_cache.size());
+
+    CHECK(data == lru_cache.find_else_insert(1,data,&status));
+    CHECK(status == LcsInsertStatus::LCS_ITEM_PRESENT);
+    CHECK(1 == lru_cache.size());
+
+    CHECK(data2 == lru_cache.find_else_insert(2,data2,&status));
+    CHECK(status == LcsInsertStatus::LCS_ITEM_INSERTED);
+    CHECK(2 == lru_cache.size());
+
+    CHECK(data2 == lru_cache.find_else_insert(1,data2,&status, true));
+    CHECK(status == LcsInsertStatus::LCS_ITEM_REPLACED);
+    CHECK(*(lru_cache.find(1)) == "54321");
+    CHECK(2 == lru_cache.size());
 }
 
 //  Test statistics counters.

diff --git a/src/service_inspectors/dce_rpc/dce_smb2_session_cache.h b/src/service_inspectors/dce_rpc/dce_smb2_session_cache.h
index f3839771cc6549c5d36c55d00f60fc23bfd2af5d..47f3f93c2ea3b30f6567e18210328cdf94505a69 100644 (file)
--- a/src/service_inspectors/dce_rpc/dce_smb2_session_cache.h
+++ b/src/service_inspectors/dce_rpc/dce_smb2_session_cache.h
@@ -43,7 +43,10 @@ public:
     Value* find_session(Key key)
     { return this->find(key).get(); }
     Value* find_else_create_session(Key key)
-    { return this->find_else_create(key, nullptr).get(); }
+    {
+        std::shared_ptr<Value> new_session = std::shared_ptr<Value>(new Value());
+        return this->find_else_insert(key, new_session, nullptr).get();
+    }
 };
 
 using Dce2Smb2SessionCache =