Pull request #4162: coverity: fix for stream and hash

Merge in SNORT/snort3 from ~RSHAFIQ/snort3:stream_coverity to master

Squashed commit of the following:

commit ebffbe09dda5a5733f86c683f16347716a0a51ce
Author: rshafiq <rshafiq@cisco.com>
Date:   Tue Dec 19 14:15:41 2023 -0500

    coverity: fix for stream and hash

diff --git a/src/hash/hash_key_operations.cc b/src/hash/hash_key_operations.cc
index 7dd18b22ca437c6c4448b85534f382f78d42a802..72bb2d4e2367ff99cef9997b3d286fb47428b3be 100644 (file)
--- a/src/hash/hash_key_operations.cc
+++ b/src/hash/hash_key_operations.cc
@@ -24,6 +24,8 @@
 #include "hash_key_operations.h"
 
 #include <cassert>
+#include <climits>
+#include <random>
 
 #include "main/snort_config.h"
 #include "utils/util.h"
@@ -34,25 +36,27 @@ using namespace snort;
 
 HashKeyOperations::HashKeyOperations(int rows)
 {
-    static bool one = true;
+    static std::mt19937 generator(static_cast<unsigned int>(std::random_device{}()));
 
-    if ( one ) /* one time init */
-    {
-        srand( (unsigned)time(nullptr) );
-        one = false;
-    }
-
-    if ( SnortConfig::static_hash() )
+    if (SnortConfig::static_hash()) 
     {
         seed = 3193;
         scale = 719;
         hardener = 133824503;
-    }
-    else
+    } 
+    else 
     {
-        seed = nearest_prime( (rand() % rows) + 3191);
-        scale = nearest_prime( (rand() % rows) + 709);
-        hardener = ((unsigned) rand() * rand()) + 133824503;
+        if ( rows <= 0 )
+            rows = 1;
+
+        std::uniform_int_distribution<> distr(1, rows);
+
+        seed = nearest_prime(distr(generator) + 3191);
+        scale = nearest_prime(distr(generator) + 709);
+
+        // For hardener, use a larger range distribution
+        std::uniform_int_distribution<unsigned long long> large_distr(0, ULLONG_MAX);
+        hardener = static_cast<unsigned long long>(large_distr(generator)) * large_distr(generator) + 133824503;
     }
 }
 

diff --git a/src/hash/lru_cache_shared.h b/src/hash/lru_cache_shared.h
index 426374f0cf1253e8cdb9e33164b1a46fa156e59c..9752264963a645cd05bb84709f368890ff0e0c1a 100644 (file)
--- a/src/hash/lru_cache_shared.h
+++ b/src/hash/lru_cache_shared.h
@@ -224,12 +224,14 @@ std::shared_ptr<Value> LruCacheShared<Key, Value, Hash, Eq, Purgatory>::find(con
     auto map_iter = map.find(key);
     if (map_iter == map.end())
     {
+        // coverity[missing_lock]
         stats.find_misses++;
         return nullptr;
     }
 
     //  Move entry to front of LruList
     list.splice(list.begin(), list, map_iter->second);
+    // coverity[missing_lock]
     stats.find_hits++;
     return map_iter->second->second;
 }
@@ -257,12 +259,15 @@ std::shared_ptr<Value> LruCacheShared<Key, Value, Hash, Eq, Purgatory>::find_els
     auto map_iter = map.find(key);
     if (map_iter != map.end())
     {
+        // coverity[missing_lock]
         stats.find_hits++;
         list.splice(list.begin(), list, map_iter->second); // update LRU
         return map_iter->second->second;
     }
 
+    // coverity[missing_lock]
     stats.find_misses++;
+    // coverity[missing_lock]
     stats.adds++;
     if ( new_data )
         *new_data = true;
@@ -291,6 +296,7 @@ bool LruCacheShared<Key, Value, Hash, Eq, Purgatory>::find_else_insert(const Key
     auto map_iter = map.find(key);
     if (map_iter != map.end())
     {
+        // coverity[missing_lock]
         stats.find_hits++;
         if (replace)
         {
@@ -299,13 +305,15 @@ bool LruCacheShared<Key, Value, Hash, Eq, Purgatory>::find_else_insert(const Key
             map_iter->second->second.reset();
             map_iter->second->second = data;
             increase_size(map_iter->second->second.get());
+            // coverity[missing_lock]
             stats.replaced++;
         }
         list.splice(list.begin(), list, map_iter->second); // update LRU
         return true;
     }
-
+    // coverity[missing_lock]
     stats.find_misses++;
+    // coverity[missing_lock]
     stats.adds++;
 
     //  Add key/data pair to front of list.

diff --git a/src/ports/port_object2.cc b/src/ports/port_object2.cc
index b0c7125722f7fd0478fa9353f970e5cbc7a1cc72..03120e33a2f1a009d6045ebd4ee7ed4e1cba4ad2 100644 (file)
--- a/src/ports/port_object2.cc
+++ b/src/ports/port_object2.cc
@@ -198,20 +198,17 @@ PortObject2* PortObject2Dup(PortObject& po)
     }
 
     /* Dup the input rule list */
-    if ( po.rule_list )
-    {
-        SF_LNODE* lpos = nullptr;
+    SF_LNODE* lpos = nullptr;
 
-        for (int* prid  = (int*)sflist_first(po.rule_list, &lpos);
-             prid != nullptr;
-             prid  = (int*)sflist_next(&lpos) )
-        {
-            int* prule = (int*)snort_calloc(sizeof(int));
-            *prule = *prid;
+    for (int* prid  = (int*)sflist_first(po.rule_list, &lpos);
+            prid != nullptr;
+            prid  = (int*)sflist_next(&lpos) )
+    {
+        int* prule = (int*)snort_calloc(sizeof(int));
+        *prule = *prid;
 
-            if ( ponew->rule_hash->insert(prule, prule) != HASH_OK )
-                snort_free(prule);
-        }
+        if ( ponew->rule_hash->insert(prule, prule) != HASH_OK )
+            snort_free(prule);
     }
 
     return ponew;

diff --git a/src/service_inspectors/dns/dns.cc b/src/service_inspectors/dns/dns.cc
index ac7a48248e1f59836258a1837c3a1a73c39a430a..09c3efd116aa5c340cda438b96f249d128519120 100644 (file)
--- a/src/service_inspectors/dns/dns.cc
+++ b/src/service_inspectors/dns/dns.cc
@@ -1004,7 +1004,7 @@ static void ParseDNSResponseMessage(Packet* p, DNSData* dnsSessionData, bool& ne
 
 SfIp DnsResponseIp::get_ip()
 {
-    SfIp ip;
+    SfIp ip = {};
     int family = 0;
     switch (type)
     {

diff --git a/src/service_inspectors/http_inspect/http_cutter.h b/src/service_inspectors/http_inspect/http_cutter.h
index 6d8b471a969ab7d30487e3830da534e9f086e549..b0115b65f1636f92fe3c8f32da9296b94a15b052 100644 (file)
--- a/src/service_inspectors/http_inspect/http_cutter.h
+++ b/src/service_inspectors/http_inspect/http_cutter.h
@@ -133,9 +133,9 @@ private:
 
     const bool accelerated_blocking;
     uint8_t partial_match = 0;
-    HttpEnums::CompressId compression;
+    HttpEnums::CompressId compression = HttpEnums::CompressId::CMP_NONE;
     bool decompress_failed = false;
-    uint8_t string_length;
+    uint8_t string_length = 0;
     z_stream* compress_stream = nullptr;
     ScriptFinder* const finder;
     const uint8_t* match_string;

diff --git a/src/service_inspectors/http_inspect/http_msg_section.h b/src/service_inspectors/http_inspect/http_msg_section.h
index 0b7f64dd07fec0150a793b0fe1b96585b2cf8f0d..e40bcd9e16437ab4d9de06d51ccb88e65b6ab4d9 100644 (file)
--- a/src/service_inspectors/http_inspect/http_msg_section.h
+++ b/src/service_inspectors/http_inspect/http_msg_section.h
@@ -120,10 +120,10 @@ protected:
     bool cleared = false;
 
     // Pointers to related message sections in the same transaction
-    HttpMsgRequest* request;
-    HttpMsgStatus* status;
-    HttpMsgHeader* header[2];
-    HttpMsgTrailer* trailer[2];
+    HttpMsgRequest* request = nullptr;
+    HttpMsgStatus* status = nullptr;
+    HttpMsgHeader* header[2] = {nullptr, nullptr};
+    HttpMsgTrailer* trailer[2] = {nullptr, nullptr};
 
 
     // Convenience methods shared by multiple subclasses

diff --git a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
index cb3c1079a9252630d9fd257f2e0a84217f2aae46..83f2fd7f1aec3675bdf93632299c786f69b6b028 100644 (file)
--- a/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
+++ b/src/service_inspectors/http_inspect/http_stream_splitter_reassemble.cc
@@ -252,11 +252,13 @@ void HttpStreamSplitter::decompress_copy(uint8_t* buffer, uint32_t& offset, cons
             inflateReset(compress_stream);
             compress_stream->next_in = const_cast<Bytef*>(zlib_header);
             compress_stream->avail_in = sizeof(zlib_header);
-            inflate(compress_stream, Z_SYNC_FLUSH);
-
-            // Start over at the beginning
-            decompress_copy(buffer, offset, data, length, compression, compress_stream, false,
-                infractions, events, session_data);
+            int ret = inflate(compress_stream, Z_SYNC_FLUSH);
+            if ( ret == Z_OK or ret == Z_STREAM_END)
+            { 
+                // Start over at the beginning
+                decompress_copy(buffer, offset, data, length, compression, compress_stream, false,
+                    infractions, events, session_data);
+            }
             return;
         }
         else

diff --git a/src/service_inspectors/http_inspect/http_uri_norm.cc b/src/service_inspectors/http_inspect/http_uri_norm.cc
index 71ede758cdb13c5918db86d24587af8c9772914b..767e5c3ad27e7a6b8f2215e1aa167e16b15a17fd 100644 (file)
--- a/src/service_inspectors/http_inspect/http_uri_norm.cc
+++ b/src/service_inspectors/http_inspect/http_uri_norm.cc
@@ -28,6 +28,8 @@
 #include "http_enum.h"
 #include "log/messages.h"
 
+#define MAP_SIZE 65536
+
 using namespace HttpEnums;
 using namespace snort;
 
@@ -491,9 +493,9 @@ bool UriNormalizer::classic_need_norm(const Field& uri_component, bool do_path,
     return need_norm(uri_component, do_path, uri_param, &unused, &events_sink);
 }
 
-void UriNormalizer::load_default_unicode_map(uint8_t map[65536])
+void UriNormalizer::load_default_unicode_map(uint8_t map[MAP_SIZE])
 {
-    memset(map, 0xFF, 65536);
+    memset(map, 0xFF, MAP_SIZE);
 
     // Default unicode map is just a single string of tokens of the form
     // HHHH:HH (HHHH = unicode, HH = ascii char)

diff --git a/src/service_inspectors/http_inspect/ips_http_param.h b/src/service_inspectors/http_inspect/ips_http_param.h
index dd37a26b54f4fa6cd4a143d328854ac5b616d59d..4f4882c2f4a92d1d0ef2886c8cf44d5bb353ac79 100644 (file)
--- a/src/service_inspectors/http_inspect/ips_http_param.h
+++ b/src/service_inspectors/http_inspect/ips_http_param.h
@@ -53,7 +53,7 @@ private:
     static THREAD_LOCAL snort::ProfileStats http_param_ps;
 
     std::string param;       // provide buffer containing specific parameter
-    bool nocase;             // case insensitive match
+    bool nocase = false;             // case insensitive match
     snort::LiteralSearch::Handle* search_handle;
 };
 

diff --git a/src/service_inspectors/netflow/netflow.cc b/src/service_inspectors/netflow/netflow.cc
index dfa1887c5e19f4ecb94b017dd21360509f64b8fd..7915df06f0ef2559a5ee549341106473f3bef673 100644 (file)
--- a/src/service_inspectors/netflow/netflow.cc
+++ b/src/service_inspectors/netflow/netflow.cc
@@ -123,8 +123,8 @@ 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.first_pkt_second = packet_time();
-        record.last_pkt_second = packet_time();
+        record.first_pkt_second = static_cast<uint32_t>(packet_time());
+        record.last_pkt_second = static_cast<uint32_t>(packet_time());
     }
 
     NetFlowEvent event(p, &record, match->create_host, match->create_service, swapped, serviceID);

diff --git a/src/service_inspectors/sip/sip_config.cc b/src/service_inspectors/sip/sip_config.cc
index c9ccedb414296bf1adf9b4cdc40b8b4f7c4e3a9f..33b6ef40710c17221347da8a6c6fbd75b67ad6b5 100644 (file)
--- a/src/service_inspectors/sip/sip_config.cc
+++ b/src/service_inspectors/sip/sip_config.cc
@@ -120,7 +120,7 @@ void SIP_ParseMethods(const char* cur_tokenp, uint32_t* methodsConfig, SIPMethod
     // Check whether this is a standard method
 
     i_method = SIP_findMethod(cur_tokenp, StandardMethods);
-    if (METHOD_NOT_FOUND != i_method )
+    if (METHOD_NOT_FOUND != i_method and StandardMethods[i_method].methodFlag > 0)
     {
         *methodsConfig |= 1 << (StandardMethods[i_method].methodFlag - 1);
         SIP_AddMethodToList(cur_tokenp, StandardMethods[i_method].methodFlag, pmethods);
@@ -198,9 +198,9 @@ SIPMethodNode* SIP_AddUserDefinedMethod(
         }
         i++;
     }
-    if (currentUseDefineMethod > SIP_METHOD_USER_DEFINE_MAX)
+    if (currentUseDefineMethod > SIP_METHOD_USER_DEFINE_MAX or currentUseDefineMethod <= SIP_METHOD_NULL)
     {
-        ParseError("Exceeded max number of user defined methods \n");
+        ParseError("Invalid user defined methods \n");
         return nullptr;
     }
     *methodsConfig |= 1 << (currentUseDefineMethod - 1);

diff --git a/src/service_inspectors/smtp/smtp.cc b/src/service_inspectors/smtp/smtp.cc
index 2c6f63292297e86f44db5f78e74a3b39014e70d8..5f9348e75c387fb0477dd8792dc3886e3fe47f21 100644 (file)
--- a/src/service_inspectors/smtp/smtp.cc
+++ b/src/service_inspectors/smtp/smtp.cc
@@ -541,11 +541,14 @@ void SmtpProtoConf::show() const
 static void SMTP_ResetState(Flow* ssn)
 {
     SMTPData* smtp_ssn = get_session_data(ssn);
-    smtp_ssn->state = STATE_COMMAND;
-    smtp_ssn->state_flags = (smtp_ssn->state_flags & SMTP_FLAG_ABANDON_EVT) ? SMTP_FLAG_ABANDON_EVT : 0;
+    if( smtp_ssn )
+    {
+        smtp_ssn->state = STATE_COMMAND;
+        smtp_ssn->state_flags = (smtp_ssn->state_flags & SMTP_FLAG_ABANDON_EVT) ? SMTP_FLAG_ABANDON_EVT : 0;
 
-    delete smtp_ssn->jsn;
-    smtp_ssn->jsn = nullptr;
+        delete smtp_ssn->jsn;
+        smtp_ssn->jsn = nullptr;
+    }
 }
 
 static inline int InspectPacket(Packet* p)
@@ -1068,7 +1071,7 @@ static void SMTP_ProcessClientPacket(SmtpProtoConf* config, Packet* p, SMTPData*
             break;
         case STATE_XEXCH50:
             if (smtp_normalizing)
-                SMTP_CopyToAltBuffer(p, ptr, end - ptr);
+                (void)SMTP_CopyToAltBuffer(p, ptr, end - ptr);
             if (smtp_is_data_end (p->flow))
                 smtp_ssn->state = STATE_COMMAND;
             return;

diff --git a/src/service_inspectors/smtp/smtp.h b/src/service_inspectors/smtp/smtp.h
index e469a94ff9c6cab6a3e3307b57770ffdfb56fa2f..2d8f5f7843ce11644f169e2ad0517aa97273525e 100644 (file)
--- a/src/service_inspectors/smtp/smtp.h
+++ b/src/service_inspectors/smtp/smtp.h
@@ -148,7 +148,7 @@ class SmtpMime : public snort::MimeSession
 {
 public:
     using snort::MimeSession::MimeSession;
-    SmtpProtoConf* config;
+    SmtpProtoConf* config = nullptr;
 #ifndef UNIT_TEST
 private:
 #endif

diff --git a/src/stream/udp/udp_session.h b/src/stream/udp/udp_session.h
index 811deddfabeea00a907c2258e4f945d6a1a4b456..0d94b91f0deafa95d0394bd737ff649f5070b01b 100644 (file)
--- a/src/stream/udp/udp_session.h
+++ b/src/stream/udp/udp_session.h
@@ -37,7 +37,7 @@ public:
     void clear() override;
 
 public:
-    struct timeval ssn_time;
+    struct timeval ssn_time = {};
 };
 
 void udp_stats();

diff --git a/src/stream/user/user_session.cc b/src/stream/user/user_session.cc
index ea8ee1ee0d4cd3e967ec090838dd95173824cf44..3f8e41c1e72ae2a682666dcf19436b59e4af34f9 100644 (file)
--- a/src/stream/user/user_session.cc
+++ b/src/stream/user/user_session.cc
@@ -493,6 +493,7 @@ int UserSession::process(Packet* p)
     if ( !ut.splitter or p->ptrs.decode_flags & DECODE_SOF )
         start(p, flow);
 
+    //coverity[forward_null]
     if ( p->data && p->dsize )
         ut.add_data(p);
 

diff --git a/src/stream/user/user_session.h b/src/stream/user/user_session.h
index a0b13bcc16cf900765dff58a557477dfc0475dd0..f70850dce7b27a5e995d3900e15ca47ece36ea71 100644 (file)
--- a/src/stream/user/user_session.h
+++ b/src/stream/user/user_session.h
@@ -69,7 +69,7 @@ struct UserTracker
 
     std::list<UserSegment*> seg_list;
     snort::StreamSplitter* splitter;
-    PAF_State paf_state;
+    PAF_State paf_state = {};
     unsigned total;
 };
 

diff --git a/src/utils/streambuf.cc b/src/utils/streambuf.cc
index b7c7c85d8dee529619a3d006ea2dbdc01a8b78c4..05375ea6a6a8b1d94baf5b4196808203068a352a 100644 (file)
--- a/src/utils/streambuf.cc
+++ b/src/utils/streambuf.cc
@@ -99,7 +99,7 @@ streampos istreambuf_glue::seekoff(streamoff off, ios_base::seekdir way, ios_bas
     pos = min(pos, size);
 
     size_t i = 0;
-    for (auto chunk : chunks)
+    for (auto &chunk : chunks)
     {
         auto ptr = get<0>(chunk);
         auto len = get<1>(chunk);
@@ -133,7 +133,7 @@ streampos istreambuf_glue::seekpos(streampos pos, ios_base::openmode which)
     pos = min(pos, size);
 
     int i = 0;
-    for (auto chunk : chunks)
+    for (auto &chunk : chunks)
     {
         auto ptr = get<0>(chunk);
         auto len = get<1>(chunk);

diff --git a/src/utils/util_jsnorm.cc b/src/utils/util_jsnorm.cc
index b36299fbbc7403a3419a9cc8867b93be95969a73..e33623619563c24bd32d74b330bda6a9d23e7f4f 100644 (file)
--- a/src/utils/util_jsnorm.cc
+++ b/src/utils/util_jsnorm.cc
@@ -26,6 +26,7 @@
 
 #include <cstdlib>
 #include <cstring>
+#include <vector>
 
 #include "main/thread.h"
 
@@ -1151,49 +1152,48 @@ static int JSNorm_exec(JSNormState* s, ActionJSNorm a, int c, const char* src, u
     char* cur_ptr;
     int iRet = RET_OK;
     uint16_t bcopied = 0;
-    // FIXIT-M this is large for stack. Move elsewhere.
-    char decoded_out[65535];
-    char* dest = decoded_out;
+    std::vector<char> decoded_out_vec(65535);
+    char* dest = decoded_out_vec.data();
 
-    cur_ptr = s->dest.data+ s->dest.len;
+    cur_ptr = s->dest.data + s->dest.len;
     switch (a)
     {
-    case ACT_NOP:
-        WriteJSNormChar(s, c, js);
-        break;
-    case ACT_SAVE:
-        s->overwrite = cur_ptr;
-        WriteJSNormChar(s, c, js);
-        break;
-    case ACT_SPACE:
-        if ( s->prev_event != ' ')
-        {
+        case ACT_NOP:
             WriteJSNormChar(s, c, js);
-        }
-        s->num_spaces++;
-        break;
-    case ACT_UNESCAPE:
-        if (s->overwrite && (s->overwrite < cur_ptr))
-        {
-            s->dest.len = s->overwrite - s->dest.data;
-        }
-        UnescapeDecode(src, srclen, ptr, &dest, sizeof(decoded_out), &bcopied, js, s->unicode_map);
-        WriteJSNorm(s, dest, bcopied, js);
-        break;
-    case ACT_SFCC:
-        if ( s->overwrite && (s->overwrite < cur_ptr))
-        {
-            s->dest.len = s->overwrite - s->dest.data;
-        }
-        StringFromCharCodeDecode(src, srclen, ptr, &dest, sizeof(decoded_out), &bcopied, js, s->unicode_map);
-        WriteJSNorm(s, dest, bcopied, js);
-        break;
-    case ACT_QUIT:
-        iRet = RET_QUIT;
-        WriteJSNormChar(s, c, js);
-        break;
-    default:
-        break;
+            break;
+        case ACT_SAVE:
+            s->overwrite = cur_ptr;
+            WriteJSNormChar(s, c, js);
+            break;
+        case ACT_SPACE:
+            if ( s->prev_event != ' ' )
+            {
+                WriteJSNormChar(s, c, js);
+            }
+            s->num_spaces++;
+            break;
+        case ACT_UNESCAPE:
+            if ( s->overwrite && (s->overwrite < cur_ptr) )
+            {
+                s->dest.len = s->overwrite - s->dest.data;
+            }
+            UnescapeDecode(src, srclen, ptr, &dest, decoded_out_vec.size(), &bcopied, js, s->unicode_map);
+            WriteJSNorm(s, dest, bcopied, js);
+            break;
+        case ACT_SFCC:
+            if ( s->overwrite && (s->overwrite < cur_ptr) )
+            {
+                s->dest.len = s->overwrite - s->dest.data;
+            }
+            StringFromCharCodeDecode(src, srclen, ptr, &dest, decoded_out_vec.size(), &bcopied, js, s->unicode_map);
+            WriteJSNorm(s, dest, bcopied, js);
+            break;
+        case ACT_QUIT:
+            iRet = RET_QUIT;
+            WriteJSNormChar(s, c, js);
+            break;
+        default:
+            break;
     }
 
     s->prev_event = c;