From: Bhargava Jandhyala (bjandhya) Date: Tue, 4 May 2021 07:15:04 +0000 (+0000) Subject: Merge pull request #2864 in SNORT/snort3 from ~DIPANDIT/snort3:smb_deadlock_main... X-Git-Tag: 3.1.5.0~17 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=066731cfd3e68e58cee5e6adbbe859ca8a036fd1;p=thirdparty%2Fsnort3.git Merge pull request #2864 in SNORT/snort3 from ~DIPANDIT/snort3:smb_deadlock_main to master Squashed commit of the following: commit 0e71ce321233e6c850d5fb2af7d0ec7e9b854091 Author: Dipto Pandit (dipandit) 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) 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 ac7fbcc59..459b42ec3 100644 --- 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 Purgatory = std::vector>> 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& 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 > 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 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& data, bool replace) return false; } +template +std::shared_ptr LruCacheShared:: +find_else_insert(const Key& key, std::shared_ptr& data, LcsInsertStatus* status, bool replace) +{ + LruMapIter map_iter; + + Purgatory tmp_data; + std::lock_guard 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 std::vector< std::pair> > LruCacheShared::get_all_data() diff --git a/src/hash/test/lru_cache_shared_test.cc b/src/hash/test/lru_cache_shared_test.cc index c2777498a..acbc9e6dd 100644 --- 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 data(new std::string("12345")); - LruCacheShared > lru_cache(1); + std::shared_ptr data2(new std::string("54321")); + LruCacheShared > 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 f3839771c..47f3f93c2 100644 --- 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 new_session = std::shared_ptr(new Value()); + return this->find_else_insert(key, new_session, nullptr).get(); + } }; using Dce2Smb2SessionCache =