Pull request #4175: CSCwi44108 - snort_calloc is used on non-trivial structures

Merge in SNORT/snort3 from ~MDAGON/snort3:calloc_cleanup to master

Squashed commit of the following:

commit 17f3fc999811be731ba414fb9d7cfc999044542e
Author: maya dagon <mdagon@cisco.com>
Date:   Thu Jan 25 08:51:13 2024 -0500

    detection: add c'tors, use new instead of snort_calloc

diff --git a/src/detection/detection_continuation.h b/src/detection/detection_continuation.h
index b3ca57c51e7fcd40689b10e97f91ab055e8bf47a..1df6d268753f75d16303ecb84ac7ce014cfb6586 100644 (file)
--- a/src/detection/detection_continuation.h
+++ b/src/detection/detection_continuation.h
@@ -72,7 +72,7 @@ private:
 
         State(const detection_option_tree_node_t& n, const detection_option_eval_data_t& d,
             snort::IpsOption* s, unsigned wp, uint64_t id, bool p) : data(d),
-            root(1, nullptr, d.otn, new RuleLatencyState[snort::ThreadConfig::get_instance_max()]()),
+            root(1, d.otn),
             selector(s), node(const_cast<detection_option_tree_node_t*>(&n)), waypoint(wp),
             original_waypoint(wp), sid(id), packet_number(d.p->context->packet_number),
             opt_parent(p)
@@ -83,9 +83,6 @@ private:
             root.children = &node;
         }
 
-        ~State()
-        { delete[] root.latency_state; }
-
         inline bool eval(snort::Packet&);
 
         detection_option_eval_data_t data;

diff --git a/src/detection/detection_options.cc b/src/detection/detection_options.cc
index fb4723ed39c1d0ae7b04d1b19bbcc5218ab94db0..88f56aa5daf5d573ca867496049a7540341c7370 100644 (file)
--- a/src/detection/detection_options.cc
+++ b/src/detection/detection_options.cc
@@ -46,12 +46,10 @@
 #include "latency/rule_latency_state.h"
 #include "log/messages.h"
 #include "main/snort_config.h"
-#include "main/thread_config.h"
 #include "managers/ips_manager.h"
 #include "parser/parser.h"
 #include "profiler/rule_profiler_defs.h"
 #include "protocols/packet_manager.h"
-#include "utils/util.h"
 #include "utils/util_cstring.h"
 
 #include "detection_continuation.h"
@@ -210,16 +208,6 @@ static bool detection_option_tree_compare(
     return true;
 }
 
-void free_detection_option_tree(detection_option_tree_node_t* node)
-{
-    for (int i = 0; i < node->num_children; i++)
-        free_detection_option_tree(node->children[i]);
-
-    snort_free(node->children);
-    snort_free(node->state);
-    snort_free(node);
-}
-
 class DetectionOptionTreeHashKeyOps : public HashKeyOperations
 {
 public:
@@ -269,7 +257,7 @@ public:
 
     void free_user_data(HashNode* hnode) override
     {
-        free_detection_option_tree((detection_option_tree_node_t*)hnode->data);
+        delete (detection_option_tree_node_t*)hnode->data;
     }
 
 };
@@ -836,17 +824,6 @@ void detection_option_tree_reset_otn_stats(std::vector<HashNode*>& nodes, unsign
     }
 }
 
-detection_option_tree_root_t* new_root(OptTreeNode* otn)
-{
-    detection_option_tree_root_t* p = (detection_option_tree_root_t*)
-        snort_calloc(sizeof(detection_option_tree_root_t));
-
-    p->latency_state = new RuleLatencyState[ThreadConfig::get_instance_max()]();
-    p->otn = otn;
-
-    return p;
-}
-
 void free_detection_option_root(void** existing_tree)
 {
     detection_option_tree_root_t* root;
@@ -857,21 +834,7 @@ void free_detection_option_root(void** existing_tree)
     root = (detection_option_tree_root_t*)*existing_tree;
     snort_free(root->children);
 
-    delete[] root->latency_state;
-    snort_free(root);
+    delete root;
     *existing_tree = nullptr;
 }
 
-detection_option_tree_node_t* new_node(option_type_t type, void* data)
-{
-    detection_option_tree_node_t* p =
-        (detection_option_tree_node_t*)snort_calloc(sizeof(*p));
-
-    p->option_type = type;
-    p->option_data = data;
-
-    p->state = (dot_node_state_t*)
-        snort_calloc(ThreadConfig::get_instance_max(), sizeof(*p->state));
-
-    return p;
-}

diff --git a/src/detection/detection_options.h b/src/detection/detection_options.h
index 3423d543667899b109d2abbff6e37c102d244140..93b75268fd690deb3598c4d0c0ec1e749761709e 100644 (file)
--- a/src/detection/detection_options.h
+++ b/src/detection/detection_options.h
@@ -34,8 +34,11 @@
 #include <sys/time.h>
 
 #include "detection/rule_option_types.h"
+#include "latency/rule_latency_state.h"
+#include "main/thread_config.h"
 #include "time/clock_defs.h"
 #include "trace/trace_api.h"
+#include "utils/util.h"
 
 namespace snort
 {
@@ -78,6 +81,15 @@ struct dot_node_state_t
     unsigned latency_timeouts;
     unsigned latency_suspends;
 
+    dot_node_state_t()
+    {
+        result = 0;
+        conts = nullptr;
+        memset(&last_check, 0, sizeof(last_check));
+        context_num = run_num = 0;
+        reset_profiling();
+    }
+
     // FIXIT-L perf profiler stuff should be factored of the node state struct
     void update(hr_duration delta, bool match)
     {
@@ -111,8 +123,11 @@ struct detection_option_tree_bud_t
     detection_option_tree_bud_t()
         : relative_children(0), num_children(0), children(nullptr), otn(nullptr) {}
 
-    detection_option_tree_bud_t(int num, detection_option_tree_node_t** d_otn, const OptTreeNode* r_otn)
-        : relative_children(0), num_children(num), children(d_otn), otn(r_otn) {}
+    detection_option_tree_bud_t(int num, const OptTreeNode* _otn)
+        : relative_children(0), num_children(num), children(nullptr), otn(_otn) {}
+
+protected:
+    ~detection_option_tree_bud_t() {}
 };
 
 struct detection_option_tree_node_t : public detection_option_tree_bud_t
@@ -122,18 +137,45 @@ struct detection_option_tree_node_t : public detection_option_tree_bud_t
     dot_node_state_t* state;
     int is_relative;
     option_type_t option_type;
+    
+    detection_option_tree_node_t(option_type_t type, void* data) :
+        evaluate(nullptr), option_data(data), is_relative(0), option_type(type)
+    {
+        state = new dot_node_state_t[snort::ThreadConfig::get_instance_max()];
+    }
+
+    ~detection_option_tree_node_t()
+    {
+        for (int i = 0; i < num_children; i++)
+            delete children[i];
+
+        snort_free(children);
+        delete[] state;
+    }
 };
 
 struct detection_option_tree_root_t : public detection_option_tree_bud_t
 {
     RuleLatencyState* latency_state;
 
-    detection_option_tree_root_t()
-        : detection_option_tree_bud_t(), latency_state(nullptr) {}
+    detection_option_tree_root_t() : latency_state(nullptr) {}
 
-    detection_option_tree_root_t(int num, detection_option_tree_node_t** d_otn, const OptTreeNode* r_otn,
-        RuleLatencyState* lat)
-        : detection_option_tree_bud_t(num, d_otn, r_otn), latency_state(lat) {}
+    detection_option_tree_root_t(OptTreeNode* _otn)
+        : detection_option_tree_bud_t(0, _otn)
+    {
+        latency_state = new RuleLatencyState[snort::ThreadConfig::get_instance_max()]();
+    }
+
+    detection_option_tree_root_t(int num, const OptTreeNode* _otn)
+        : detection_option_tree_bud_t(num, _otn)
+    {
+        latency_state = new RuleLatencyState[snort::ThreadConfig::get_instance_max()]();
+    }
+
+    ~detection_option_tree_root_t()
+    {
+        delete[] latency_state;
+    }
 };
 
 struct detection_option_eval_data_t
@@ -171,11 +213,7 @@ void detection_option_tree_update_otn_stats(std::vector<snort::HashNode*>&,
     std::unordered_map<SigInfo*, OtnState>&, unsigned);
 void detection_option_tree_reset_otn_stats(std::vector<snort::HashNode*>&, unsigned);
 
-detection_option_tree_root_t* new_root(OptTreeNode*);
 void free_detection_option_root(void** existing_tree);
 
-detection_option_tree_node_t* new_node(option_type_t, void*);
-void free_detection_option_tree(detection_option_tree_node_t*);
-
 #endif
 

diff --git a/src/detection/fp_create.cc b/src/detection/fp_create.cc
index 804a8e37281c6cc52d59f9e183be2a1ff67feccb..b6b76c7b10296b938583f0d7c9c261f46518bdb6 100644 (file)
--- a/src/detection/fp_create.cc
+++ b/src/detection/fp_create.cc
@@ -118,7 +118,7 @@ static int finalize_detection_option_tree(SnortConfig* sc, detection_option_tree
         if ( void* dup_node = add_detection_option_tree(sc, node) )
         {
             // FIXIT-L delete dup_node and keep original?
-            free_detection_option_tree(node);
+            delete node;
             root->children[i] = (detection_option_tree_node_t*)dup_node;
         }
         else
@@ -160,7 +160,7 @@ static int otn_create_tree(OptTreeNode* otn, void** existing_tree, Mpse::MpseTyp
         return -1;
 
     if (!*existing_tree)
-        *existing_tree = new_root(otn);
+        *existing_tree = new detection_option_tree_root_t(otn);
 
     detection_option_tree_root_t* const root = (detection_option_tree_root_t*)*existing_tree;
     detection_option_tree_bud_t* bud = root;
@@ -211,7 +211,7 @@ static int otn_create_tree(OptTreeNode* otn, void** existing_tree, Mpse::MpseTyp
         if (!child)
         {
             /* No children at this node */
-            child = new_node(opt_fp->type, option_data);
+            child = new detection_option_tree_node_t(opt_fp->type, option_data);
             child->evaluate = opt_fp->OptTestFunc;
 
             bud->children[i] = child;
@@ -241,7 +241,7 @@ static int otn_create_tree(OptTreeNode* otn, void** existing_tree, Mpse::MpseTyp
             {
                 /* No matching child node, create a new and add to array */
                 detection_option_tree_node_t** tmp_children;
-                child = new_node(opt_fp->type, option_data);
+                child = new detection_option_tree_node_t(opt_fp->type, option_data);
                 child->evaluate = opt_fp->OptTestFunc;
                 child->num_children++;
                 child->children = (detection_option_tree_node_t**)
@@ -277,7 +277,7 @@ static int otn_create_tree(OptTreeNode* otn, void** existing_tree, Mpse::MpseTyp
         return 0;
 
     /* Append a leaf node that has option data of the SigInfo/otn pointer */
-    child = new_node(RULE_OPTION_TYPE_LEAF_NODE, otn);
+    child = new detection_option_tree_node_t(RULE_OPTION_TYPE_LEAF_NODE, otn);
 
     if (bud->children[0])
     {
@@ -318,7 +318,7 @@ static int otn_create_tree(OptTreeNode* otn, void** existing_tree, Mpse::MpseTyp
         }
 
         void* option_data = fbs->ips_opt;
-        child = new_node(fbs->type, option_data);
+        child = new detection_option_tree_node_t(fbs->type, option_data);
         child->evaluate = fbs->OptTestFunc;
         child->is_relative = fbs->isRelative;
         bud->children[i++] = child;
@@ -379,7 +379,7 @@ static int pmx_create_tree(SnortConfig* sc, void* id, void** existing_tree, Mpse
     OptTreeNode* otn = (OptTreeNode*)pmx->rule_node.rnRuleData;
 
     if (!*existing_tree)
-        *existing_tree = new_root(otn);
+        *existing_tree = new detection_option_tree_root_t(otn);
 
     return otn_create_tree(otn, existing_tree, mpse_type);
 }

diff --git a/src/latency/rule_latency.cc b/src/latency/rule_latency.cc
index 21d7a72b3d763d55e770ce463804ccd818bfc7c0..fffe97a96ccac67aa638ecb5009c726cfd60e931 100644 (file)
--- a/src/latency/rule_latency.cc
+++ b/src/latency/rule_latency.cc
@@ -590,19 +590,14 @@ TEST_CASE ( "default latency rule interface", "[latency]" )
     if ( !instances )
         instances = 1;
 
-    std::unique_ptr<RuleLatencyState[]> latency_state(new RuleLatencyState[instances]());
-
     std::unique_ptr<detection_option_tree_node_t*[]> children(
         new detection_option_tree_node_t*[1]());
 
-    detection_option_tree_node_t child;
+    detection_option_tree_node_t child(RULE_OPTION_TYPE_LEAF_NODE, nullptr);
     children[0] = &child;
 
-    std::unique_ptr<dot_node_state_t[]> child_state(new dot_node_state_t[instances]());
-    child.state = child_state.get();
-
     detection_option_tree_root_t root;
-    root.latency_state = latency_state.get();
+    root.latency_state = new RuleLatencyState[instances]();
     root.num_children = 1;
     root.children = children.get();
 
@@ -644,23 +639,23 @@ TEST_CASE ( "default latency rule interface", "[latency]" )
             SECTION( "timeouts under threshold" )
             {
                 CHECK( false == RuleInterface::timeout_and_suspend(root, 2, hr_time(0_ticks), true) );
-                CHECK( child_state[0].latency_timeouts == 1 );
-                CHECK( child_state[0].latency_suspends == 0 );
+                CHECK( child.state[0].latency_timeouts == 1 );
+                CHECK( child.state[0].latency_suspends == 0 );
             }
 
             SECTION( "timeouts exceed threshold" )
             {
                 CHECK( true == RuleInterface::timeout_and_suspend(root, 1, hr_time(0_ticks), true) );
-                CHECK( child_state[0].latency_timeouts == 1 );
-                CHECK( child_state[0].latency_suspends == 1 );
+                CHECK( child.state[0].latency_timeouts == 1 );
+                CHECK( child.state[0].latency_suspends == 1 );
             }
         }
 
         SECTION( "suspend disabled" )
         {
             CHECK( false == RuleInterface::timeout_and_suspend(root, 0, hr_time(0_ticks), false) );
-            CHECK( child_state[0].latency_timeouts == 1 );
-            CHECK( child_state[0].latency_suspends == 0 );
+            CHECK( child.state[0].latency_timeouts == 1 );
+            CHECK( child.state[0].latency_suspends == 0 );
         }
     }
 }