From: Michael Matirko (mmatirko) Date: Fri, 31 Oct 2025 21:39:54 +0000 (+0000) Subject: Pull request #4945: memory, filters: resolve coverity and TSAN issues X-Git-Tag: 3.9.7.0~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a4f50838586e553b8e0118e6adb9f702ea7a3a45;p=thirdparty%2Fsnort3.git Pull request #4945: memory, filters: resolve coverity and TSAN issues Merge in SNORT/snort3 from ~MMATIRKO/snort3:coverity_calamity to master Squashed commit of the following: commit 696a51e6dad7ca1c6351831ca2b08899538346b5 Author: Michael Matirko Date: Thu Oct 9 13:52:36 2025 -0400 memory: resolve race condition on global stats filters: resolve lock issues, 2k38 issues in rate_filter and sfthd stream: add additional lock/unlock when we do extra_data_log perf_monitor: don't decrement index if already zero appid: fix printf args perf_monitor: fix minor issue with int overflow ha: guard against negative shift codec: fix byte math, codec coverity issues rna: use std::move on RnaTracker to move instead of copying snort2lua: use std::move where possible stream, loggers: use std::move where possible sfthd: fix issues with printf type specifier, cppcheck issues detection_engine: use const where possible --- diff --git a/src/detection/detection_engine.cc b/src/detection/detection_engine.cc index b2a0e625c..f8f25f10e 100644 --- a/src/detection/detection_engine.cc +++ b/src/detection/detection_engine.cc @@ -367,7 +367,7 @@ bool DetectionEngine::get_replacement(std::string& s, unsigned& off) if ( Analyzer::get_switcher()->get_context()->rpl.empty() ) return false; - auto rep = Analyzer::get_switcher()->get_context()->rpl.back(); + const auto& rep = Analyzer::get_switcher()->get_context()->rpl.back(); s = rep.data; off = rep.offset; diff --git a/src/detection/fp_create.cc b/src/detection/fp_create.cc index 09a69491f..c870e032a 100644 --- a/src/detection/fp_create.cc +++ b/src/detection/fp_create.cc @@ -144,7 +144,7 @@ static bool new_sig(int num_children, detection_option_tree_node_t** nodes, OptT if ( child->option_type != RULE_OPTION_TYPE_LEAF_NODE ) continue; - OptTreeNode* cotn = (OptTreeNode*)child->option_data; + const OptTreeNode* cotn = (OptTreeNode*)child->option_data; const SigInfo& csi = cotn->sigInfo; const SigInfo& osi = otn->sigInfo; @@ -1049,7 +1049,7 @@ static void fpCreatePortObject2RuleGroup(SnortConfig* sc, PortObject2* po, PortO node = pox->rule_hash->find_next()) { unsigned sid, gid; - int* prindex = (int*)node->data; + const int* prindex = (int*)node->data; /* be safe - no rule index, ignore it */ if (prindex == nullptr) @@ -1089,7 +1089,7 @@ static void fpCreatePortObject2RuleGroup(SnortConfig* sc, PortObject2* po, PortO static void fpCreatePortTableRuleGroups(SnortConfig* sc, PortTable* p, PortObject2* poaa) { int cnt = 1; - FastPatternConfig* fp = sc->fast_pattern_config; + const FastPatternConfig* fp = sc->fast_pattern_config; if ( fp->get_debug_print_rule_group_build_details() ) LogMessage("%d Port Groups in Port Table\n",p->pt_mpo_hash->get_count()); @@ -1510,7 +1510,7 @@ static bool fp_print_port_groups(RulePortTables* port_tables) */ static void fpCreateServiceRuleGroups(SnortConfig* sc) { - FastPatternConfig* fp = sc->fast_pattern_config; + const FastPatternConfig* fp = sc->fast_pattern_config; sc->srmmTable = ServiceMapNew(); diff --git a/src/filters/rate_filter.cc b/src/filters/rate_filter.cc index eb885a4d3..f5e58d67f 100644 --- a/src/filters/rate_filter.cc +++ b/src/filters/rate_filter.cc @@ -80,7 +80,7 @@ int RateFilter_Create( #ifdef RF_DBG printf( "THRESHOLD: gid=%u, sid=%u, tracking=%d, count=%u, seconds=%u \n", - thdx->gid, thdx->sid, thdx->tracking, thdx->count, thdx->seconds); + thdx->gid, thdx->sid, thdx->tracking, unsigned(thdx->count), thdx->seconds); #endif /* Add the object to the table - */ diff --git a/src/filters/rate_filter.h b/src/filters/rate_filter.h index 55b958518..b905b2726 100644 --- a/src/filters/rate_filter.h +++ b/src/filters/rate_filter.h @@ -29,7 +29,7 @@ struct Packet; struct SnortConfig; } struct RateFilterConfig; -struct tSFRFConfigNode; +class tSFRFConfigNode; struct OptTreeNode; RateFilterConfig* RateFilter_ConfigNew(); diff --git a/src/filters/sfrf.cc b/src/filters/sfrf.cc index 861c27c4e..692ee4a1e 100644 --- a/src/filters/sfrf.cc +++ b/src/filters/sfrf.cc @@ -117,15 +117,17 @@ static void SFRF_New(unsigned nbytes) nrows = nbytes / (SFRF_BYTES); /* Create global hash table for all of the IP Nodes */ + std::lock_guard lock(sfrf_hash_mutex); rf_hash = new XHash(nrows, sizeof(tSFRFTrackingNodeKey), sizeof(tSFRFTrackingNode), nbytes); } void SFRF_Delete() { + std::lock_guard lock(sfrf_hash_mutex); + if ( !rf_hash ) return; - std::lock_guard lock(sfrf_hash_mutex); delete rf_hash; rf_hash = nullptr; } @@ -161,12 +163,21 @@ static void SFRF_SidNodeFree(void* item) int SFRF_Alloc(unsigned int memcap) { - if ( rf_hash == nullptr ) + sfrf_hash_mutex.lock(); + const XHash* current_rf_hash = rf_hash; + sfrf_hash_mutex.unlock(); + + if ( current_rf_hash == nullptr ) { SFRF_New(memcap); + sfrf_hash_mutex.lock(); if ( rf_hash == nullptr ) + { + sfrf_hash_mutex.unlock(); return -1; + } + sfrf_hash_mutex.unlock(); } return 0; } @@ -460,8 +471,8 @@ void SFRF_ShowObjects(RateFilterConfig* config) { printf(".........SFRF_ID =%d\n",cfgNode->tid); printf(".........tracking =%d\n",cfgNode->tracking); - printf(".........count =%u\n",cfgNode->count); - printf(".........seconds =%u\n",cfgNode->seconds); + printf(".........count =%u\n",cfgNode->count.load()); + printf(".........seconds =%lu\n",cfgNode->seconds); } } } @@ -481,13 +492,13 @@ static int checkSamplingPeriod(tSFRFConfigNode* cfgNode, tSFRFTrackingNode* dynN { if ( cfgNode->seconds ) { - unsigned dt = (unsigned)(curTime - dynNode->tstart); + time_t dt = curTime - dynNode->tstart; if ( dt >= cfgNode->seconds ) { // observation period is over, start a new one dynNode->tstart = curTime; - dt = (unsigned)(curTime - dynNode->tlast); + dt = curTime - dynNode->tlast; if ( dt > cfgNode->seconds ) dynNode->overRate = 0; else @@ -534,7 +545,7 @@ static int checkThreshold(tSFRFConfigNode* cfgNode, tSFRFTrackingNode* dynNode, if ( dynNode->filterState == FS_ON ) { if ( (cfgNode->timeout != 0 ) - && ((unsigned)(curTime - dynNode->revertTime) >= cfgNode->timeout)) + && (curTime - dynNode->revertTime) >= cfgNode->timeout) { if ( dynNode->count > cfgNode->count || dynNode->overRate ) { diff --git a/src/filters/sfrf.h b/src/filters/sfrf.h index c55b156fe..7aa2761a1 100644 --- a/src/filters/sfrf.h +++ b/src/filters/sfrf.h @@ -22,6 +22,7 @@ #ifndef SFRF_H #define SFRF_H +#include #include #include @@ -64,15 +65,18 @@ typedef enum FS_NEW = 0, FS_OFF, FS_ON, FS_MAX } FilterState; -struct tSFRFConfigNode +class tSFRFConfigNode { +public: int tid = 0; unsigned gid = 0; unsigned sid = 0; PolicyId policyId = 0; SFRF_TRACK tracking = SFRF_TRACK_BY_NONE; - unsigned count = 0; - unsigned seconds = 0; + time_t seconds = 0; + + // The count variable can be updated by multiple threads simultaneously, so it must be atomic + std::atomic count{0}; // Action that replaces original rule action on reaching threshold snort::IpsAction::Type newAction = 0; @@ -81,6 +85,54 @@ struct tSFRFConfigNode unsigned timeout = 0; sfip_var_t* applyTo = nullptr; + tSFRFConfigNode() = default; + + tSFRFConfigNode(const tSFRFConfigNode& other) + : tid(other.tid) + , gid(other.gid) + , sid(other.sid) + , policyId(other.policyId) + , tracking(other.tracking) + , seconds(other.seconds) + , count(other.count.load()) + , newAction(other.newAction) + , timeout(other.timeout) + , applyTo(other.applyTo) + { } + + tSFRFConfigNode(tSFRFConfigNode&& other) noexcept + : tid(other.tid) + , gid(other.gid) + , sid(other.sid) + , policyId(other.policyId) + , tracking(other.tracking) + , seconds(other.seconds) + , count(other.count.load()) + , newAction(other.newAction) + , timeout(other.timeout) + , applyTo(other.applyTo) + { } + + ~tSFRFConfigNode() = default; + + tSFRFConfigNode& operator=(const tSFRFConfigNode& other) + { + if (this != &other) + { + tid = other.tid; + gid = other.gid; + sid = other.sid; + policyId = other.policyId; + tracking = other.tracking; + seconds = other.seconds; + count.store(other.count.load()); + newAction = other.newAction; + timeout = other.timeout; + applyTo = other.applyTo; + } + return *this; + } + void init() { tid = 0; diff --git a/src/filters/sfthd.cc b/src/filters/sfthd.cc index 53e2baf5d..8c5f57075 100644 --- a/src/filters/sfthd.cc +++ b/src/filters/sfthd.cc @@ -457,16 +457,16 @@ static inline int sfthd_test_non_suppress( THD_IP_NODE* sfthd_ip_node, time_t curtime) { - unsigned dt; + uint64_t dt; if ( sfthd_node->type == THD_TYPE_DETECT ) { - dt = (unsigned)(curtime - sfthd_ip_node->tstart); + dt = curtime - sfthd_ip_node->tstart; if ( dt >= sfthd_node->seconds ) { /* reset */ sfthd_ip_node->tstart = curtime; - if ( (unsigned)(curtime - sfthd_ip_node->tlast) > sfthd_node->seconds ) + if ( static_cast(curtime - sfthd_ip_node->tlast) > sfthd_node->seconds ) sfthd_ip_node->prev = 0; else sfthd_ip_node->prev = sfthd_ip_node->count - 1; @@ -486,7 +486,7 @@ static inline int sfthd_test_non_suppress( } if ( sfthd_node->type == THD_TYPE_LIMIT ) { - dt = (unsigned)(curtime - sfthd_ip_node->tstart); + dt = curtime - sfthd_ip_node->tstart; if ( dt >= sfthd_node->seconds ) { /* reset */ @@ -505,7 +505,7 @@ static inline int sfthd_test_non_suppress( } else if ( sfthd_node->type == THD_TYPE_THRESHOLD ) { - dt = (unsigned)(curtime - sfthd_ip_node->tstart); + dt = curtime - sfthd_ip_node->tstart; if ( dt >= sfthd_node->seconds ) { sfthd_ip_node->tstart = curtime; @@ -522,7 +522,7 @@ static inline int sfthd_test_non_suppress( } else if ( sfthd_node->type == THD_TYPE_BOTH ) { - dt = (unsigned)(curtime - sfthd_ip_node->tstart); + dt = curtime - sfthd_ip_node->tstart; if ( dt >= sfthd_node->seconds ) { sfthd_ip_node->tstart = curtime; @@ -875,7 +875,7 @@ int sfthd_show_objects(ThresholdObjects* thd_objs) else { printf(".........count =%d\n",sfthd_node->count); - printf(".........seconds =%u\n",sfthd_node->seconds); + printf(".........seconds =%lu\n",sfthd_node->seconds); } } } diff --git a/src/filters/sfthd.h b/src/filters/sfthd.h index 466d823bd..b5d5ea100 100644 --- a/src/filters/sfthd.h +++ b/src/filters/sfthd.h @@ -137,7 +137,7 @@ struct THD_NODE int type = 0; int priority = 0; int count = 0; - unsigned seconds = 0; + uint64_t seconds = 0; sfip_var_t* ip_address = nullptr; }; diff --git a/src/flow/ha.cc b/src/flow/ha.cc index a60faf40b..ed72cc7b9 100644 --- a/src/flow/ha.cc +++ b/src/flow/ha.cc @@ -222,6 +222,11 @@ FlowHAClient::FlowHAClient(uint8_t length, bool session_client) : max_length(len } index = ha->handle_counter; + + // Guard against null index which could cause a shift by a negative amount + if (!index) + return; + handle = (1 << (index - 1)); ha->client_map[index] = this; ha->handle_counter++; @@ -247,7 +252,7 @@ static uint8_t write_flow_key(const Flow& flow, HAMessage& msg) msg.advance_cursor(sizeof(key->ip_l[3])); memcpy(msg.cursor, &key->ip_h[3], sizeof(key->ip_h[3])); msg.advance_cursor(sizeof(key->ip_h[3])); - memcpy(msg.cursor, ((const uint8_t*) key) + 32, KEY_SIZE_IP4 - 8); + memcpy(msg.cursor, (reinterpret_cast(key)) + 32, KEY_SIZE_IP4 - 8); msg.advance_cursor(KEY_SIZE_IP4 - 8); return KEY_TYPE_IP4; @@ -288,7 +293,7 @@ static uint8_t read_flow_key(HAMessage& msg, const HAMessageHeader* hdr, FlowKey key.ip_h[2] = htonl(0xFFFF); msg.advance_cursor(sizeof(key.ip_h[3])); /* The remainder of the key */ - memcpy(((uint8_t*) &key) + 32, msg.cursor, KEY_SIZE_IP4 - 8); + memcpy((reinterpret_cast(&key)) + 32, msg.cursor, KEY_SIZE_IP4 - 8); msg.advance_cursor(KEY_SIZE_IP4 - 8); return KEY_SIZE_IP4; @@ -334,7 +339,7 @@ static uint16_t calculate_update_msg_content_length(Flow& flow, bool full) // at the beginning of the content section. static void write_msg_header(const Flow& flow, HAEvent event, uint16_t content_length, HAMessage& msg) { - HAMessageHeader* hdr = (HAMessageHeader*) msg.cursor; + HAMessageHeader* hdr = reinterpret_cast(msg.cursor); hdr->event = (uint8_t) event; hdr->version = HA_MESSAGE_VERSION; hdr->total_length = content_length; @@ -344,7 +349,7 @@ static void write_msg_header(const Flow& flow, HAEvent event, uint16_t content_l static uint16_t update_msg_header_length(const HAMessage& msg) { - HAMessageHeader* hdr = (HAMessageHeader*) msg.buffer; + HAMessageHeader* hdr = reinterpret_cast(msg.buffer); hdr->total_length = msg.cursor_position(); return hdr->total_length; } @@ -359,7 +364,7 @@ static void write_update_msg_client(FlowHAClient* client, Flow& flow, HAMessage& // Preemptively insert the client header. If production fails, roll back the message cursor // to its original position. uint8_t* original_cursor = msg.cursor; - HAClientHeader* header = (HAClientHeader*) original_cursor; + HAClientHeader* header = reinterpret_cast(original_cursor); header->client = client->index; msg.advance_cursor(sizeof(HAClientHeader)); if (!client->produce(flow, msg)) @@ -410,7 +415,7 @@ static Flow* consume_ha_update_message(HAMessage& msg, const FlowKey& key, Packe break; } - HAClientHeader* header = (HAClientHeader*) msg.cursor; + HAClientHeader* header = reinterpret_cast(msg.cursor); if ((header->client >= ha->handle_counter) || (ha->client_map[header->client] == nullptr)) { ErrorMessage("Consuming HA Update message - invalid client index\n"); @@ -479,7 +484,7 @@ static Flow* consume_ha_message(HAMessage& msg, return nullptr; } - const HAMessageHeader* hdr = (HAMessageHeader*) msg.cursor; + const HAMessageHeader* hdr = reinterpret_cast(msg.cursor); if (hdr->version != HA_MESSAGE_VERSION) { diff --git a/src/helpers/test/grouped_list_test.cc b/src/helpers/test/grouped_list_test.cc index 51df74944..555158c38 100644 --- a/src/helpers/test/grouped_list_test.cc +++ b/src/helpers/test/grouped_list_test.cc @@ -72,7 +72,7 @@ TEST_CASE("Basic", "[Double list]") SECTION("1 element") { const Type data = {1, "one"}; - Elem* el = new Elem(cont, group, data); + const Elem* el = new Elem(cont, group, data); CHECK(el != nullptr); @@ -84,9 +84,9 @@ TEST_CASE("Basic", "[Double list]") const Type data1 = {1, "one"}; const Type data2 = {2, "two"}; const Type data3 = {3, "three"}; - Elem* el1 = new Elem(cont, group, data1); - Elem* el2 = new Elem(cont, group, data2); - Elem* el3 = new Elem(cont, group, data3); + const Elem* el1 = new Elem(cont, group, data1); + const Elem* el2 = new Elem(cont, group, data2); + const Elem* el3 = new Elem(cont, group, data3); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -107,11 +107,11 @@ TEST_CASE("Groups", "[Double list]") const Type data3 = {3, "three"}; Elem* group_a = nullptr; - Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el1 = new Elem(cont, group_a, data1); CHECK(group_a == el1); - Elem* el2 = new Elem(cont, group_a, data2); + const Elem* el2 = new Elem(cont, group_a, data2); CHECK(group_a == el2); - Elem* el3 = new Elem(cont, group_a, data3); + const Elem* el3 = new Elem(cont, group_a, data3); CHECK(group_a == el3); check_container(cont, data1, data2, data3); @@ -132,11 +132,11 @@ TEST_CASE("Groups", "[Double list]") Elem* group_b = nullptr; Elem* group_c = nullptr; - Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el1 = new Elem(cont, group_a, data1); CHECK(group_a == el1); - Elem* el2 = new Elem(cont, group_b, data2); + const Elem* el2 = new Elem(cont, group_b, data2); CHECK(group_b == el2); - Elem* el3 = new Elem(cont, group_c, data3); + const Elem* el3 = new Elem(cont, group_c, data3); CHECK(group_c == el3); check_container(cont, data1, data2, data3); @@ -207,11 +207,11 @@ TEST_CASE("Groups", "[Double list]") const Type data3 = {3, "three"}; Elem* group_a = nullptr; - Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el1 = new Elem(cont, group_a, data1); CHECK(group_a == el1); Elem* el2 = new Elem(cont, group_a, data2); CHECK(group_a == el2); - Elem* el3 = new Elem(cont, group_a, data3); + const Elem* el3 = new Elem(cont, group_a, data3); CHECK(group_a == el3); check_container(cont, data1, data2, data3); @@ -238,9 +238,9 @@ TEST_CASE("Groups", "[Double list]") Elem* el1 = new Elem(cont, group_a, data1); CHECK(group_a == el1); - Elem* el2 = new Elem(cont, group_a, data2); + const Elem* el2 = new Elem(cont, group_a, data2); CHECK(group_a == el2); - Elem* el3 = new Elem(cont, group_a, data3); + const Elem* el3 = new Elem(cont, group_a, data3); CHECK(group_a == el3); check_container(cont, data1, data2, data3); @@ -265,9 +265,9 @@ TEST_CASE("Groups", "[Double list]") const Type data3 = {3, "three"}; Elem* group_a = nullptr; - Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el1 = new Elem(cont, group_a, data1); CHECK(group_a == el1); - Elem* el2 = new Elem(cont, group_a, data2); + const Elem* el2 = new Elem(cont, group_a, data2); CHECK(group_a == el2); Elem* el3 = new Elem(cont, group_a, data3); CHECK(group_a == el3); @@ -381,7 +381,7 @@ TEST_CASE("Memory management (value by copy)", "[Double list]") SECTION("delete group") { const Type data = {1, "one"}; - Elem* el = new Elem(cont, group_a, data); + const Elem* el = new Elem(cont, group_a, data); CHECK(el != nullptr); @@ -397,8 +397,8 @@ TEST_CASE("Memory management (value by copy)", "[Double list]") { const Type data1 = {1, "one"}; const Type data2 = {2, "two"}; - Elem* el1 = new Elem(cont, group_a, data1); - Elem* el2 = new Elem(cont, group_b, data2); + const Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el2 = new Elem(cont, group_b, data2); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -418,8 +418,8 @@ TEST_CASE("Memory management (value by copy)", "[Double list]") { const Type data1 = {1, "one"}; const Type data2 = {2, "two"}; - Elem* el1 = new Elem(cont, group_a, data1); - Elem* el2 = new Elem(cont, group_b, data2); + const Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el2 = new Elem(cont, group_b, data2); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -439,9 +439,9 @@ TEST_CASE("Memory management (value by copy)", "[Double list]") const Type data1 = {1, "one"}; const Type data2 = {2, "two"}; const Type data3 = {3, "three"}; - Elem* el1 = new Elem(cont, group_a, data1); - Elem* el2 = new Elem(cont, group_b, data2); - Elem* el3 = new Elem(cont, group_b, data3); + const Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el2 = new Elem(cont, group_b, data2); + const Elem* el3 = new Elem(cont, group_b, data3); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -463,9 +463,9 @@ TEST_CASE("Memory management (value by copy)", "[Double list]") const Type data1 = {1, "one"}; const Type data2 = {2, "two"}; const Type data3 = {3, "three"}; - Elem* el1 = new Elem(cont, group_a, data1); - Elem* el2 = new Elem(cont, group_b, data2); - Elem* el3 = new Elem(cont, group_b, data3); + const Elem* el1 = new Elem(cont, group_a, data1); + const Elem* el2 = new Elem(cont, group_b, data2); + const Elem* el3 = new Elem(cont, group_b, data3); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -503,7 +503,7 @@ TEST_CASE("Memory management (value in-place)", "[Double list]") SECTION("delete group") { - Elem* el = new Elem(cont, group_a, 1, "one"); + const Elem* el = new Elem(cont, group_a, 1, "one"); CHECK(el != nullptr); @@ -517,8 +517,8 @@ TEST_CASE("Memory management (value in-place)", "[Double list]") SECTION("delete element, then delete group") { - Elem* el1 = new Elem(cont, group_a, 1, "one"); - Elem* el2 = new Elem(cont, group_b, 2, "two"); + const Elem* el1 = new Elem(cont, group_a, 1, "one"); + const Elem* el2 = new Elem(cont, group_b, 2, "two"); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -536,8 +536,8 @@ TEST_CASE("Memory management (value in-place)", "[Double list]") SECTION("delete group, then delete element") { - Elem* el1 = new Elem(cont, group_a, 1, "one"); - Elem* el2 = new Elem(cont, group_b, 2, "two"); + const Elem* el1 = new Elem(cont, group_a, 1, "one"); + const Elem* el2 = new Elem(cont, group_b, 2, "two"); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -554,9 +554,9 @@ TEST_CASE("Memory management (value in-place)", "[Double list]") SECTION("delete elements (remain), then delete group") { - Elem* el1 = new Elem(cont, group_a, 1, "one"); - Elem* el2 = new Elem(cont, group_b, 2, "two"); - Elem* el3 = new Elem(cont, group_b, 3, "three"); + const Elem* el1 = new Elem(cont, group_a, 1, "one"); + const Elem* el2 = new Elem(cont, group_b, 2, "two"); + const Elem* el3 = new Elem(cont, group_b, 3, "three"); CHECK(el1 != nullptr); CHECK(el2 != nullptr); @@ -575,9 +575,9 @@ TEST_CASE("Memory management (value in-place)", "[Double list]") SECTION("delete group, then delete elements (remain)") { - Elem* el1 = new Elem(cont, group_a, 1, "one"); - Elem* el2 = new Elem(cont, group_b, 2, "two"); - Elem* el3 = new Elem(cont, group_b, 3, "three"); + const Elem* el1 = new Elem(cont, group_a, 1, "one"); + const Elem* el2 = new Elem(cont, group_b, 2, "two"); + const Elem* el3 = new Elem(cont, group_b, 3, "three"); CHECK(el1 != nullptr); CHECK(el2 != nullptr); diff --git a/src/host_tracker/host_cache_module.cc b/src/host_tracker/host_cache_module.cc index 2d898e398..2197324f9 100644 --- a/src/host_tracker/host_cache_module.cc +++ b/src/host_tracker/host_cache_module.cc @@ -79,7 +79,7 @@ static int host_cache_get_segment_stats(lua_State* L) static int host_cache_delete_host(lua_State* L) { - HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); + const HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); if ( mod ) { const char* ips = luaL_optstring(L, 1, nullptr); @@ -111,7 +111,7 @@ static int host_cache_delete_host(lua_State* L) static int host_cache_delete_network_proto(lua_State* L) { - HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); + const HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); if ( mod ) { const char* ips = luaL_optstring(L, 1, nullptr); @@ -151,7 +151,7 @@ static int host_cache_delete_network_proto(lua_State* L) static int host_cache_delete_transport_proto(lua_State* L) { - HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); + const HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); if ( mod ) { const char* ips = luaL_optstring(L, 1, nullptr); @@ -190,7 +190,7 @@ static int host_cache_delete_transport_proto(lua_State* L) static int host_cache_delete_service(lua_State* L) { - HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); + const HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); if ( mod ) { const char* ips = luaL_optstring(L, 1, nullptr); @@ -237,7 +237,7 @@ static int host_cache_delete_service(lua_State* L) static int host_cache_delete_client(lua_State* L) { - HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); + const HostCacheModule* mod = (HostCacheModule*) ModuleManager::get_module(HOST_CACHE_NAME); if ( mod ) { const char* ips = luaL_optstring(L, 1, nullptr); @@ -496,7 +496,7 @@ string HostCacheModule::get_host_cache_segment_stats(int seg_idx) + to_string(lru_data.size()) + " trackers, memcap: " + to_string(host_cache.get_max_size()) + " bytes\n"; - PegCount* counts = (PegCount*) host_cache.get_counts(); + const PegCount* counts = (PegCount*) host_cache.get_counts(); for ( int i = 0; pegs[i].type != CountType::END; i++ ) { if ( counts[i] ) @@ -551,7 +551,7 @@ string HostCacheModule::get_host_cache_stats() + to_string(lru_data.size()) + " trackers, memcap: " + to_string(host_cache.get_max_size()) + " bytes\n"; - PegCount* counts = (PegCount*) host_cache.get_counts(); + const PegCount* counts = (PegCount*) host_cache.get_counts(); const PegInfo* pegs = host_cache.get_pegs(); for ( int i = 0; pegs[i].type != CountType::END; i++ ) diff --git a/src/ips_options/ips_byte_math.cc b/src/ips_options/ips_byte_math.cc index 9bd18cffb..59f0ec947 100644 --- a/src/ips_options/ips_byte_math.cc +++ b/src/ips_options/ips_byte_math.cc @@ -245,6 +245,8 @@ int ByteMathOption::calc(uint32_t& value, const uint32_t rvalue) } case BM_DIVIDE: assert(rvalue != 0); + if (!rvalue) + return NO_MATCH; value /= rvalue; break; @@ -495,7 +497,7 @@ static void mod_dtor(Module* m) static IpsOption* byte_math_ctor(Module* p, IpsInfo&) { - ByteMathModule* m = (ByteMathModule*)p; + ByteMathModule* m = reinterpret_cast(p); ByteMathData& data = m->data; data.result_var = AddVarNameToList(data.result_name); diff --git a/src/loggers/alert_talos.cc b/src/loggers/alert_talos.cc index 5360c2a44..f6a697d08 100644 --- a/src/loggers/alert_talos.cc +++ b/src/loggers/alert_talos.cc @@ -127,7 +127,7 @@ void TalosLogger::open() if ( sep_pos != string::npos ) ifname = ifname.substr(sep_pos+1); - talos_log->name = ifname; + talos_log->name = std::move(ifname); } void TalosLogger::close() @@ -185,14 +185,14 @@ void TalosLogger::alert(Packet*, const char* msg, const Event& event) message.pop_back(); rule.key = key.str(); - rule.msg = message; + rule.msg = std::move(message); rule.gid = gid; rule.sid = sid; rule.rev = rev; rule.count = 1; // rule not in map, add it - alerts[key.str()] = rule; + alerts[key.str()] = std::move(rule); } //------------------------------------------------------------------------- diff --git a/src/managers/codec_manager.cc b/src/managers/codec_manager.cc index 64217bfd0..7e97cc783 100644 --- a/src/managers/codec_manager.cc +++ b/src/managers/codec_manager.cc @@ -142,7 +142,10 @@ void CodecManager::instantiate(CodecApiWrapper& wrap, Module* m) static std::size_t codec_id = 1; if (codec_id >= s_protocols.size()) + { ParseError("A maximum of 256 codecs can be registered"); + return; + } if (cd_api->pinit) cd_api->pinit(); diff --git a/src/managers/plugin_manager.cc b/src/managers/plugin_manager.cc index e87ab9b3d..45ca102f5 100644 --- a/src/managers/plugin_manager.cc +++ b/src/managers/plugin_manager.cc @@ -218,7 +218,7 @@ static bool register_plugin( return false; // keep the old one } - p.key = key; + p.key = std::move(key); p.api = api; p.handle = std::move(handle); p.source = file; diff --git a/src/memory/dev_notes.txt b/src/memory/dev_notes.txt index 066992fdc..514e721b2 100644 --- a/src/memory/dev_notes.txt +++ b/src/memory/dev_notes.txt @@ -84,3 +84,14 @@ well enough below the actual hard limit, for example the limit enforced by cgrou processing of a single packet will not push us over. It must also allow for additional heap overhead. +Note regarding statistics collection in the Memory module: + +Locking for statistics collection is minimized as the write operation (update_global_stats) +is protected by a standard lock to ensure multiple threads don't update the global stats +simultaneously. We are able to bail out if another thread is currently updating the statistics, +as they will be approximately equal for either of the simulteneous callers. + +Read access (Module::sum_stats) can be protected via a shared lock, since +all that we require here is that stats are not being updated while we are reading them. +Thus, we never should block to acquire a writer lock, and the critical section is minimal for +the Module::sum_stats call. \ No newline at end of file diff --git a/src/memory/memory_module.cc b/src/memory/memory_module.cc index 286eb90c2..981292f18 100644 --- a/src/memory/memory_module.cc +++ b/src/memory/memory_module.cc @@ -120,8 +120,15 @@ PegCount* MemoryModule::get_counts() const void MemoryModule::sum_stats(bool dump_stats) { - memory::MemoryCap::update_global_stats(); + if (mem_global_stats_mutex.try_lock()) + { + memory::MemoryCap::update_global_stats(); + mem_global_stats_mutex.unlock(); + } + + mem_global_stats_mutex.lock_shared(); Module::sum_stats(dump_stats); + mem_global_stats_mutex.unlock(); } void MemoryModule::set_trace(const Trace* trace) const diff --git a/src/memory/memory_module.h b/src/memory/memory_module.h index 5aa05eaaa..66f89a9b2 100644 --- a/src/memory/memory_module.h +++ b/src/memory/memory_module.h @@ -23,6 +23,8 @@ #include "framework/module.h" +#include + class MemoryModule : public snort::Module { public: @@ -40,6 +42,8 @@ public: void set_trace(const snort::Trace*) const override; const snort::TraceOption* get_trace_options() const override; + + std::shared_mutex mem_global_stats_mutex; }; extern THREAD_LOCAL const snort::Trace* memory_trace; diff --git a/src/network_inspectors/perf_monitor/perf_monitor.cc b/src/network_inspectors/perf_monitor/perf_monitor.cc index 7c2dd891a..612087279 100644 --- a/src/network_inspectors/perf_monitor/perf_monitor.cc +++ b/src/network_inspectors/perf_monitor/perf_monitor.cc @@ -210,6 +210,8 @@ void PerfMonitor::show(const SnortConfig*) const void PerfMonitor::disable_tracker(size_t i) { + assert (i < trackers->size() && i > 0); + WarningMessage("Disabling %s\n", (*trackers)[i]->get_name().c_str()); auto tracker = trackers->at(i); @@ -253,10 +255,16 @@ void PerfMonitor::tinit() if (config->perf_flags & PERF_CPU ) trackers->emplace_back(new CPUTracker(config)); - for (unsigned i = 0; i < trackers->size(); i++) + // Second condition is to prevent overflow, shouldn't happen in practice + for (unsigned i = 0; i < trackers->size() && i < std::numeric_limits::max(); i++) { if (!(*trackers)[i]->open(true)) - disable_tracker(i--); + { + disable_tracker(i); + + if (i > 0) + i--; + } } for (auto& tracker : *trackers) @@ -265,7 +273,7 @@ void PerfMonitor::tinit() bool PerfMonReloadTuner::tinit() { - PerfMonitor* pm = (PerfMonitor*)PigPen::get_inspector(PERF_NAME, true); + PerfMonitor* pm = reinterpret_cast(PigPen::get_inspector(PERF_NAME, true)); auto* new_constraints = pm->get_constraints(); if (new_constraints->flow_ip_enabled) @@ -317,9 +325,16 @@ void PerfMonitor::tterm() void PerfMonitor::rotate() { - for ( unsigned i = 0; i < trackers->size(); i++ ) - if ( !(*trackers)[i]->rotate() ) - disable_tracker(i--); + for (auto& tracker : *trackers) + { + if (!tracker->rotate()) + { + auto found_trk = std::find(trackers->begin(), trackers->end(), tracker); + size_t pos = found_trk - trackers->begin(); + if (found_trk != trackers->end() && pos > 0 && pos < trackers->size()) + disable_tracker(pos - 1); + } + } } void PerfMonitor::swap_constraints(PerfConstraints* constraints) @@ -452,7 +467,7 @@ static void mod_dtor(Module* m) { delete m; } static Inspector* pm_ctor(Module* m) -{ return new PerfMonitor(((PerfMonModule*)m)->get_config()); } +{ return new PerfMonitor((reinterpret_cast(m))->get_config()); } static void pm_dtor(Inspector* p) { delete p; } diff --git a/src/network_inspectors/rna/rna_pnd.cc b/src/network_inspectors/rna/rna_pnd.cc index 8274bbd3b..9500d6387 100644 --- a/src/network_inspectors/rna/rna_pnd.cc +++ b/src/network_inspectors/rna/rna_pnd.cc @@ -715,7 +715,7 @@ void RnaPnd::discover_network_ethernet(const Packet* p) if ( !etherType or etherType > static_cast(ProtocolId::ETHERTYPE_MINIMUM) ) { - generate_new_host_mac(p, rt); + generate_new_host_mac(p, std::move(rt)); return; } @@ -723,7 +723,7 @@ void RnaPnd::discover_network_ethernet(const Packet* p) if ( llc->s.s.DSAP != llc->s.s.SSAP ) { - generate_new_host_mac(p, rt); + generate_new_host_mac(p, std::move(rt)); return; } @@ -745,7 +745,7 @@ void RnaPnd::discover_network_ethernet(const Packet* p) } if ( retval ) - generate_new_host_mac(p, rt, true); + generate_new_host_mac(p, std::move(rt), true); return; } diff --git a/src/parser/parse_conf.cc b/src/parser/parse_conf.cc index 938cf85e3..10ffc75ac 100644 --- a/src/parser/parse_conf.cc +++ b/src/parser/parse_conf.cc @@ -183,7 +183,7 @@ const char* get_config_file(const char* arg, std::string& file) if ( relative_to_include_dir(arg, file) ) return "I"; - file = hint; + file = std::move(hint); if ( relative_to_parse_dir(arg, file) ) return "F"; diff --git a/src/parser/var_dependency.cc b/src/parser/var_dependency.cc index 222a0fb02..00af5eed4 100644 --- a/src/parser/var_dependency.cc +++ b/src/parser/var_dependency.cc @@ -62,7 +62,7 @@ static std::list extract_variable_names(const char* input) ; std::string variable(input + begin + 1, end - begin - 1); - variable_names.push_back(variable); + variable_names.push_back(std::move(variable)); begin = end - 1; } diff --git a/src/parser/vars.cc b/src/parser/vars.cc index fb598907a..40d765fd8 100644 --- a/src/parser/vars.cc +++ b/src/parser/vars.cc @@ -489,7 +489,7 @@ const std::string ExpandVars(const std::string& input_str) { case '-': if (var_contents.empty()) - var_contents = var_aux; + var_contents = std::move(var_aux); break; case '?': diff --git a/src/payload_injector/payload_injector.cc b/src/payload_injector/payload_injector.cc index 4d21af8d1..2607eea02 100644 --- a/src/payload_injector/payload_injector.cc +++ b/src/payload_injector/payload_injector.cc @@ -163,5 +163,8 @@ const char* PayloadInjector::get_err_string(InjectionReturnStatus status) { auto iter = InjectionErrorToString.find(status); assert (iter != InjectionErrorToString.end()); - return iter->second; + if (iter != InjectionErrorToString.end()) + return iter->second; + else + return nullptr; } diff --git a/src/service_inspectors/netflow/netflow.cc b/src/service_inspectors/netflow/netflow.cc index 36728296f..1e1a3549f 100644 --- a/src/service_inspectors/netflow/netflow.cc +++ b/src/service_inspectors/netflow/netflow.cc @@ -124,7 +124,10 @@ static void publish_netflow_event(const Packet* p, const NetFlowRule* match, Net // LAST_PKT_SECOND - if these aren't set, assume the current wire pkt time if (!record.first_pkt_second or !record.last_pkt_second) { + // Record structures are uint_32 so we must use that here + //coverity[y2k38_safety] record.first_pkt_second = static_cast(packet_time()); + //coverity[y2k38_safety] record.last_pkt_second = static_cast(packet_time()); } diff --git a/src/service_inspectors/netflow/netflow_module.cc b/src/service_inspectors/netflow/netflow_module.cc index b3de30bbb..0a32ff396 100644 --- a/src/service_inspectors/netflow/netflow_module.cc +++ b/src/service_inspectors/netflow/netflow_module.cc @@ -30,6 +30,7 @@ #include "netflow_cache.h" #include "netflow_module.h" +#include "log/messages.h" #include "utils/util.h" using namespace snort; @@ -242,6 +243,14 @@ void NetFlowModule::parse_service_id_file(const std::string& serv_id_file_path) while( std::getline(ss, tmp_str, '\t') ) tokens.push_back(tmp_str); + if ( tokens.size() != 3 ) + { + ParseWarning(WARN_CONF, + "netflow: malformed service id line (expected 3 tab-separated tokens, got %u): %s", + static_cast(tokens.size()), serv_line.c_str()); + continue; + } + // Format is uint16_t srv_port = std::stoi(tokens[0]); std::string proto_str = tokens[1]; @@ -267,7 +276,7 @@ PegCount* NetFlowModule::get_counts() const netflow_stats.netflow_cache_bytes_in_use = 0; netflow_stats.template_cache_bytes_in_use = 0; } - return (PegCount*)&netflow_stats; + return reinterpret_cast(&netflow_stats); } const PegInfo* NetFlowModule::get_pegs() const diff --git a/src/stream/base/stream_module.cc b/src/stream/base/stream_module.cc index f92aa3372..0dd3156a3 100644 --- a/src/stream/base/stream_module.cc +++ b/src/stream/base/stream_module.cc @@ -234,14 +234,14 @@ uncompleted queue*/ #endif ); SfIp src_ip,src_subnet; - if (!df->set_ip(source_ip, src_ip, src_subnet)) + if (!df->set_ip(std::move(source_ip), src_ip, src_subnet)) { LogRespond(ctrlcon, "Invalid source ip\n"); delete df; return -1; } SfIp dst_ip,dst_subnet; - if (!df->set_ip(destination_ip, dst_ip, dst_subnet)) + if (!df->set_ip(std::move(destination_ip), dst_ip, dst_subnet)) { LogRespond(ctrlcon, "Invalid destination ip\n"); delete df; @@ -326,14 +326,14 @@ static int dump_flows_summary(lua_State* L) DumpFlowsSummary* dfs = new DumpFlowsSummary(ctrlcon); SfIp src_ip,src_subnet; - if (!dfs->set_ip(source_ip, src_ip, src_subnet)) + if (!dfs->set_ip(std::move(source_ip), src_ip, src_subnet)) { LogRespond(ctrlcon, "Invalid source ip\n"); delete dfs; return -1; } SfIp dst_ip,dst_subnet; - if (!dfs->set_ip(destination_ip, dst_ip, dst_subnet)) + if (!dfs->set_ip(std::move(destination_ip), dst_ip, dst_subnet)) { LogRespond(ctrlcon, "Invalid destination ip\n"); delete dfs; diff --git a/src/stream/stream.cc b/src/stream/stream.cc index 3d88004a7..0611241a7 100644 --- a/src/stream/stream.cc +++ b/src/stream/stream.cc @@ -499,8 +499,10 @@ StreamSplitter* Stream::get_splitter(Flow* flow, bool to_server) void Stream::log_extra_data( Flow* flow, uint32_t mask, const AlertInfo& alert_info) { + std::lock_guard xtra_lock(stream_xtra_mutex); if ( mask && stream.extra_data_log ) { + stream.extra_data_log( flow, stream.extra_data_context, stream.xtradata_map, stream.xtradata_func_count, mask, alert_info); @@ -777,7 +779,7 @@ uint16_t Stream::get_mss(Flow* flow, bool to_server) { assert(flow and flow->session and flow->pkt_type == PktType::TCP); - TcpSession* tcp_session = (TcpSession*)flow->session; + TcpSession* tcp_session = reinterpret_cast(flow->session); return tcp_session->get_mss(to_server); } @@ -785,7 +787,7 @@ uint8_t Stream::get_tcp_options_len(Flow* flow, bool to_server) { assert(flow and flow->session and flow->pkt_type == PktType::TCP); - TcpSession* tcp_session = (TcpSession*)flow->session; + TcpSession* tcp_session = reinterpret_cast(flow->session); return tcp_session->get_tcp_options_len(to_server); } @@ -801,7 +803,7 @@ bool Stream::can_set_no_ack_mode(Flow* flow) { assert(flow and flow->session and flow->pkt_type == PktType::TCP); - TcpSession* tcp_session = (TcpSession*)flow->session; + TcpSession* tcp_session = reinterpret_cast(flow->session); return tcp_session->can_set_no_ack(); } @@ -809,7 +811,7 @@ bool Stream::set_no_ack_mode(Flow* flow, bool on_off) { assert(flow and flow->session and flow->pkt_type == PktType::TCP); - TcpSession* tcp_session = (TcpSession*)flow->session; + TcpSession* tcp_session = reinterpret_cast(flow->session); return tcp_session->set_no_ack(on_off); } @@ -818,9 +820,9 @@ void Stream::partial_flush(Flow* flow, bool to_server) if ( flow->pkt_type == PktType::TCP ) { if ( to_server ) - ((TcpSession*)flow->session)->server.perform_partial_flush(); + reinterpret_cast(flow->session)->server.perform_partial_flush(); else - ((TcpSession*)flow->session)->client.perform_partial_flush(); + reinterpret_cast(flow->session)->client.perform_partial_flush(); } } @@ -829,7 +831,7 @@ bool Stream::get_held_pkt_seq(Flow* flow, uint32_t& seq) if (!flow or !flow->session or !(flow->pkt_type == PktType::TCP)) return false; - TcpSession* tcp_session = (TcpSession*)flow->session; + TcpSession* tcp_session = reinterpret_cast(flow->session); if (tcp_session->held_packet_dir == SSN_DIR_NONE) return false; diff --git a/src/utils/util_unfold.cc b/src/utils/util_unfold.cc index 325de7e74..4d5de1c1b 100644 --- a/src/utils/util_unfold.cc +++ b/src/utils/util_unfold.cc @@ -95,7 +95,8 @@ int sf_unfold_header(const uint8_t* inbuf, uint32_t inbuf_size, uint8_t* outbuf, if (n < outbuf_size) *outbuf_ptr = '\0'; else - outbuf[outbuf_size - 1] = '\0'; + if (outbuf_size) + outbuf[outbuf_size - 1] = '\0'; *output_bytes = outbuf_ptr - outbuf; if (folded) diff --git a/tools/snort2lua/config_states/config_classification.cc b/tools/snort2lua/config_states/config_classification.cc index 3f1d970bb..8b5c0a635 100644 --- a/tools/snort2lua/config_states/config_classification.cc +++ b/tools/snort2lua/config_states/config_classification.cc @@ -62,6 +62,7 @@ bool Classification::convert(std::istringstream& data_stream) if (!(data_stream >> priority)) return false; + // coverity[tainted_scalar] table_api.add_option("priority", priority); return true; } diff --git a/tools/snort2lua/config_states/config_paf_max.cc b/tools/snort2lua/config_states/config_paf_max.cc index e60ad5f53..81ab4252c 100644 --- a/tools/snort2lua/config_states/config_paf_max.cc +++ b/tools/snort2lua/config_states/config_paf_max.cc @@ -54,7 +54,7 @@ bool PafMax::convert(std::istringstream& data_stream) if (!(data_stream >> val)) return true; - + // coverity[tainted_scalar] data_api.failed_conversion(data_stream, std::to_string(val)); } else diff --git a/tools/snort2lua/config_states/config_policy_id.cc b/tools/snort2lua/config_states/config_policy_id.cc index d2636268b..81799aa26 100644 --- a/tools/snort2lua/config_states/config_policy_id.cc +++ b/tools/snort2lua/config_states/config_policy_id.cc @@ -42,6 +42,7 @@ bool PolicyId::convert(std::istringstream& data_stream) if (data_stream >> policy_id) { cv.get_table_api().open_table("ips"); + // coverity[tainted_scalar] cv.get_table_api().add_option("id", policy_id); cv.get_table_api().close_table(); @@ -57,6 +58,7 @@ bool PolicyId::convert(std::istringstream& data_stream) if (data_stream >> policy_id) { + // coverity[tainted_scalar] data_api.failed_conversion(data_stream, std::to_string(policy_id)); rc = false; } diff --git a/tools/snort2lua/config_states/config_ppm.cc b/tools/snort2lua/config_states/config_ppm.cc index 2e24aa58c..042961844 100644 --- a/tools/snort2lua/config_states/config_ppm.cc +++ b/tools/snort2lua/config_states/config_ppm.cc @@ -114,6 +114,7 @@ bool Ppm::convert(std::istringstream& data_stream) { table_api.add_diff_option_comment("suspend-timeout", "max_suspend_time"); table_api.add_comment("seconds changed to milliseconds"); + // coverity[tainted_scalar] tmpval = table_api.add_option("max_suspend_time", opt * 1000); } diff --git a/tools/snort2lua/config_states/config_profile.cc b/tools/snort2lua/config_states/config_profile.cc index 90996627c..a0fd406aa 100644 --- a/tools/snort2lua/config_states/config_profile.cc +++ b/tools/snort2lua/config_states/config_profile.cc @@ -163,7 +163,7 @@ bool Profilers::convert(std::istringstream& data_stream) add_or_append("sort", "no_matches"); } else - add_or_append("sort", val); + add_or_append("sort", std::move(val)); } else { diff --git a/tools/snort2lua/conversion_state.h b/tools/snort2lua/conversion_state.h index b23355ea2..250320ba5 100644 --- a/tools/snort2lua/conversion_state.h +++ b/tools/snort2lua/conversion_state.h @@ -133,6 +133,7 @@ protected: if (append) table_api.append_option(opt_name, val); else + // coverity[tainted_scalar] table_api.add_option(opt_name, val); return true; } @@ -175,6 +176,7 @@ protected: if (stream >> val) { val = !val ? -1 : ( val == -1 ? 0 : val ); + // coverity[tainted_scalar] table_api.add_option(opt_name, val); return true; } diff --git a/tools/snort2lua/data/data_types/dt_option.cc b/tools/snort2lua/data/data_types/dt_option.cc index 2214e8cfb..672e9ace6 100644 --- a/tools/snort2lua/data/data_types/dt_option.cc +++ b/tools/snort2lua/data/data_types/dt_option.cc @@ -26,12 +26,12 @@ Option::Option(std::string val, int d) if (val.front() == '$') { val.erase(val.begin()); - this->value = std::string(val); + this->value = std::string(std::move(val)); this->type = OptionType::VAR; } else { - this->value = std::string(val); + this->value = std::string(std::move(val)); this->type = OptionType::STRING; } } @@ -39,7 +39,7 @@ Option::Option(std::string val, int d) Option::Option(const std::string& n, int val, int d) { this->name = n; - this->value = std::to_string(val); + this->value = std::to_string(std::move(val)); this->depth = d; this->type = OptionType::INT; } @@ -60,12 +60,12 @@ Option::Option(const std::string& opt_name, std::string val, int d) if (val.front() == '$') { val.erase(val.begin()); - this->value = std::string(val); + this->value = std::string(std::move(val)); this->type = OptionType::VAR; } else { - this->value = std::string(val); + this->value = std::string(std::move(val)); this->type = OptionType::STRING; } } diff --git a/tools/snort2lua/data/data_types/dt_rule.cc b/tools/snort2lua/data/data_types/dt_rule.cc index c764a367b..908b21dfb 100644 --- a/tools/snort2lua/data/data_types/dt_rule.cc +++ b/tools/snort2lua/data/data_types/dt_rule.cc @@ -251,7 +251,7 @@ void Rule::resolve_pcre_buffer_options() name == "http_uri" || name == "raw_data") { - curr_sticky_buffer = name; + curr_sticky_buffer = std::move(name); ++iter; } else if (name == "http_header" || @@ -266,7 +266,7 @@ void Rule::resolve_pcre_buffer_options() } else { - curr_sticky_buffer = name; + curr_sticky_buffer = std::move(name); ++iter; } } diff --git a/tools/snort2lua/data/data_types/dt_var.cc b/tools/snort2lua/data/data_types/dt_var.cc index 4b411b93a..d7ea58d3a 100644 --- a/tools/snort2lua/data/data_types/dt_var.cc +++ b/tools/snort2lua/data/data_types/dt_var.cc @@ -73,7 +73,7 @@ bool Variable::add_value(std::string elem) if (elem.size() <= 1) { - s = elem; + s = std::move(elem); end = ""; } else @@ -81,7 +81,7 @@ bool Variable::add_value(std::string elem) const std::size_t pos = elem.find('$', 1); if (pos == std::string::npos) { - s = elem; + s = std::move(elem); end = ""; } else @@ -121,7 +121,7 @@ bool Variable::add_value(std::string elem) VarData* vd = new VarData(); vd->type = VarType::VARIABLE; - vd->data = s; + vd->data = std::move(s); vars.push_back(vd); } else if (!vars.empty() && vars.back()->type == VarType::STRING) @@ -138,12 +138,12 @@ bool Variable::add_value(std::string elem) if (!vars.empty() and s != " ") s.insert(0, " "); - vd->data = s; + vd->data = std::move(s); vars.push_back(vd); } if (!end.empty()) - return add_value(end); + return add_value(std::move(end)); return true; } diff --git a/tools/snort2lua/keyword_states/kws_attribute_table.cc b/tools/snort2lua/keyword_states/kws_attribute_table.cc index f58ab9dd5..0725bd039 100644 --- a/tools/snort2lua/keyword_states/kws_attribute_table.cc +++ b/tools/snort2lua/keyword_states/kws_attribute_table.cc @@ -386,7 +386,7 @@ void AttributeTable::parse_entry() // add this pair to the map. if (!id.empty() && !value.empty()) - attr_map[id] = value; + attr_map[id] = std::move(value); } void AttributeTable::parse_map_entries() @@ -435,7 +435,7 @@ bool AttributeTable::convert(std::istringstream& data_stream) data_api.failed_conversion(data_stream, error_string); return false; } - file = full_file; + file = std::move(full_file); } table_api.open_table("hosts"); diff --git a/tools/snort2lua/output_states/out_csv.cc b/tools/snort2lua/output_states/out_csv.cc index 3c41bfab2..46daf11dd 100644 --- a/tools/snort2lua/output_states/out_csv.cc +++ b/tools/snort2lua/output_states/out_csv.cc @@ -230,6 +230,7 @@ bool AlertCsv::convert(std::istringstream& data_stream) else limit = (limit + 1024*1024 - 1) / (1024*1024); + // coverity[tainted_scalar] retval = table_api.add_option("limit", limit) && retval; retval = table_api.add_comment("limit now in MB, converted") && retval; retval = table_api.add_deleted_comment("units") && retval; diff --git a/tools/snort2lua/output_states/out_fast.cc b/tools/snort2lua/output_states/out_fast.cc index 3116a3e69..80542a272 100644 --- a/tools/snort2lua/output_states/out_fast.cc +++ b/tools/snort2lua/output_states/out_fast.cc @@ -80,6 +80,7 @@ bool AlertFast::convert(std::istringstream& data_stream) else limit = (limit + 1024*1024 - 1) / (1024*1024); + // coverity[tainted_scalar] retval = table_api.add_option("limit", limit) && retval; retval = table_api.add_comment("limit now in MB, converted") && retval; retval = table_api.add_deleted_comment("units") && retval; diff --git a/tools/snort2lua/output_states/out_full.cc b/tools/snort2lua/output_states/out_full.cc index b81bc3fb4..bf14f3d10 100644 --- a/tools/snort2lua/output_states/out_full.cc +++ b/tools/snort2lua/output_states/out_full.cc @@ -65,6 +65,7 @@ bool AlertFull::convert(std::istringstream& data_stream) else limit = (limit + 1024*1024 - 1) / (1024*1024); + // coverity[tainted_scalar] retval = table_api.add_option("limit", limit) && retval; retval = table_api.add_comment("limit now in MB, converted") && retval; retval = table_api.add_deleted_comment("units") && retval; diff --git a/tools/snort2lua/output_states/out_tcpdump.cc b/tools/snort2lua/output_states/out_tcpdump.cc index fbd7efa0f..70f11f0d8 100644 --- a/tools/snort2lua/output_states/out_tcpdump.cc +++ b/tools/snort2lua/output_states/out_tcpdump.cc @@ -65,6 +65,7 @@ bool LogTcpDump::convert(std::istringstream& data_stream) else limit = (limit + 1024*1024 - 1) / (1024*1024); + // coverity[tainted_scalar] retval = table_api.add_option("limit", limit) && retval; retval = table_api.add_comment("limit now in MB, converted") && retval; retval = table_api.add_deleted_comment("units") && retval; diff --git a/tools/snort2lua/preprocessor_states/pps_dcerpc.cc b/tools/snort2lua/preprocessor_states/pps_dcerpc.cc index 8ee36e3e5..479cdab86 100644 --- a/tools/snort2lua/preprocessor_states/pps_dcerpc.cc +++ b/tools/snort2lua/preprocessor_states/pps_dcerpc.cc @@ -124,6 +124,7 @@ bool Dcerpc::parse_int_and_add_to_all(const std::string& opt_name, std::istrings if (stream >> val) { + // coverity[tainted_scalar] return add_option_to_all(opt_name, val, co_only); } diff --git a/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc b/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc index 0676e31c5..a6fcc6576 100644 --- a/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc +++ b/tools/snort2lua/preprocessor_states/pps_ftp_telnet_protocol.cc @@ -145,7 +145,7 @@ bool FtpServer::parse_alt_max_cmd(std::istringstream& stream) Command c; c.name = std::string(elem); c.length = len; - commands.push_back(c); + commands.push_back(std::move(c)); updated = true; } else @@ -203,26 +203,26 @@ bool FtpServer::parse_cmd_validity_cmd(std::istringstream& data_stream) if (it == commands.end()) { Command c; - c.name = std::string(command); - c.format = std::string(format); + c.name = std::string(std::move(command)); + c.format = std::string(std::move(format)); c.length = command_default_len; - commands.push_back(c); + commands.push_back(std::move(c)); updated = true; } // command exists, but format unspecified (length likely specified) else if (it->format.empty()) { - it->format = std::string(format); + it->format = std::string(std::move(format)); updated = true; } // command && format exists, but format is different. create new command else if (format != it->format) { Command c; - c.name = std::string(command); - c.format = std::string(format); + c.name = std::string(std::move(command)); + c.format = std::string(std::move(format)); c.length = it->length; - commands.push_back(c); + commands.push_back(std::move(c)); updated = true; } else @@ -742,7 +742,10 @@ bool Telnet::convert(std::istringstream& data_stream) int i_val; if (data_stream >> i_val) + { + // coverity[tainted_scalar] tmpval = table_api.add_option("ayt_attack_thresh", i_val); + } else tmpval = false; } diff --git a/tools/snort2lua/preprocessor_states/pps_http_inspect.cc b/tools/snort2lua/preprocessor_states/pps_http_inspect.cc index d103c1feb..568796a0d 100644 --- a/tools/snort2lua/preprocessor_states/pps_http_inspect.cc +++ b/tools/snort2lua/preprocessor_states/pps_http_inspect.cc @@ -106,9 +106,11 @@ bool HttpInspect::convert(std::istringstream& data_stream) std::string codemap; int code_page; + // coverity[tainted_scalar] if ( (data_stream >> codemap) && (data_stream >> code_page)) { + // coverity[tainted_scalar] tmpval = table_api.add_option("iis_unicode_map_file", codemap); tmpval = table_api.add_option("iis_unicode_code_page", code_page) && tmpval; } diff --git a/tools/snort2lua/preprocessor_states/pps_smtp.cc b/tools/snort2lua/preprocessor_states/pps_smtp.cc index c91ed2f1b..ba0ef8642 100644 --- a/tools/snort2lua/preprocessor_states/pps_smtp.cc +++ b/tools/snort2lua/preprocessor_states/pps_smtp.cc @@ -86,7 +86,7 @@ bool Smtp::parse_alt_max_cmd(std::istringstream& stream) Command c; c.name = std::string(elem); c.length = len; - commands.push_back(c); + commands.push_back(std::move(c)); } else { diff --git a/tools/snort2lua/preprocessor_states/pps_stream5_global.cc b/tools/snort2lua/preprocessor_states/pps_stream5_global.cc index 915870b9e..dbf56762a 100644 --- a/tools/snort2lua/preprocessor_states/pps_stream5_global.cc +++ b/tools/snort2lua/preprocessor_states/pps_stream5_global.cc @@ -199,7 +199,10 @@ bool StreamGlobal::convert(std::istringstream& data_stream) } } if ( emit_max_flows ) + { + // coverity[tainted_scalar] table_api.add_option("max_flows", tcp_max + udp_max + icmp_max + ip_max); + } if ( emit_pruning_timeout ) table_api.add_option("pruning_timeout", INT_MAX == pruning_timeout ? 30 : pruning_timeout); diff --git a/tools/snort2lua/preprocessor_states/pps_stream5_tcp.cc b/tools/snort2lua/preprocessor_states/pps_stream5_tcp.cc index 1ce62689d..7b01fc4e8 100644 --- a/tools/snort2lua/preprocessor_states/pps_stream5_tcp.cc +++ b/tools/snort2lua/preprocessor_states/pps_stream5_tcp.cc @@ -87,7 +87,9 @@ bool StreamTcp::parse_small_segments(std::istringstream& stream) return false; table_api.open_table("small_segments"); + // coverity[tainted_scalar] table_api.add_option("count", consec_segs); + // coverity[tainted_scalar] table_api.add_option("maximum_size", min_bytes); table_api.close_table(); @@ -96,7 +98,10 @@ bool StreamTcp::parse_small_segments(std::istringstream& stream) uint16_t port; while (stream >> port) + { + // coverity[tainted_scalar] ignore_ports += " " + std::to_string(port); + } table_api.add_deleted_comment(ignore_ports); } @@ -346,7 +351,10 @@ bool StreamTcp::convert(std::istringstream& data_stream) int val; if ( arg_stream >> val ) + { + // coverity[tainted_scalar] table_api.add_option("require_3whs", val); + } else table_api.add_option("require_3whs", 0); } @@ -361,7 +369,7 @@ bool StreamTcp::convert(std::istringstream& data_stream) while (arg_stream >> tmp) addr += " " + tmp; - add_to_bindings(&Binder::add_when_net, addr); + add_to_bindings(&Binder::add_when_net, std::move(addr)); } else { diff --git a/tools/snort2lua/rule_states/rule_pcre.cc b/tools/snort2lua/rule_states/rule_pcre.cc index abf26ead8..6af6c7a61 100644 --- a/tools/snort2lua/rule_states/rule_pcre.cc +++ b/tools/snort2lua/rule_states/rule_pcre.cc @@ -124,7 +124,7 @@ bool Pcre::convert(std::istringstream& data_stream) if (!sticky_buffer.empty()) { - buffer = sticky_buffer; + buffer = std::move(sticky_buffer); if (sticky_buffer_set) rule_api.bad_rule(data_stream, diff --git a/tools/snort2lua/rule_states/rule_ttl.cc b/tools/snort2lua/rule_states/rule_ttl.cc index 4098e822b..edc16e9ec 100644 --- a/tools/snort2lua/rule_states/rule_ttl.cc +++ b/tools/snort2lua/rule_states/rule_ttl.cc @@ -49,7 +49,7 @@ bool Ttl::convert(std::istringstream& stream) std::string new_val; if ( arg.find('-') == std::string::npos ) - new_val = arg; + new_val = std::move(arg); else { if ( arg.find('-') != arg.rfind('-') ) @@ -79,6 +79,7 @@ bool Ttl::convert(std::istringstream& stream) arg_stream >> low; arg_stream.ignore(1); arg_stream >> high; + // coverity[tainted_scalar] new_val = std::to_string(low) + "<=>" + std::to_string(high); } } diff --git a/tools/u2boat/u2boat.cc b/tools/u2boat/u2boat.cc index 880ce79ad..8ccb84c98 100644 --- a/tools/u2boat/u2boat.cc +++ b/tools/u2boat/u2boat.cc @@ -205,6 +205,8 @@ static int GetRecord(FILE* input, u2record* rec) /* Read in the data portion of the record */ if (rec->length > buffer_size) { + // If we fail to allocate here, we simply error out + // coverity[tainted_scalar] tmp = (uint8_t*)malloc(rec->length * sizeof(uint8_t)); if (tmp == nullptr) { @@ -221,6 +223,7 @@ static int GetRecord(FILE* input, u2record* rec) buffer_size = rec->length; } } + // coverity[tainted_scalar] items_read = fread(rec->data, sizeof(uint8_t), rec->length, input); if (items_read != rec->length) { diff --git a/tools/u2spewfoo/u2spewfoo.cc b/tools/u2spewfoo/u2spewfoo.cc index ab89e30ad..36cf53144 100644 --- a/tools/u2spewfoo/u2spewfoo.cc +++ b/tools/u2spewfoo/u2spewfoo.cc @@ -129,6 +129,7 @@ static bool get_record(u2iterator* it, u2record* record) s_pos = ftell(it->file); + // coverity[tainted_scalar] tmp = (uint8_t*)realloc(record->data, record->length); if (!tmp) @@ -556,14 +557,19 @@ static int u2dump(char* file) while ( get_record(it, &record) ) { if ( record.type == UNIFIED2_EVENT3 and record.length == sizeof(Unified2Event) ) + { event3_dump(&record); - + } else if ( (record.type == UNIFIED2_PACKET) or (record.type == UNIFIED2_BUFFER) ) + { + // coverity[tainted_scalar] packet_dump(&record); - + } else if (record.type == UNIFIED2_EXTRA_DATA) + { + // coverity[tainted_scalar] extradata_dump(&record); - + } // deprecated else if ( record.type == UNIFIED2_IDS_EVENT_VLAN and record.length == sizeof(Unified2IDSEvent) )