Merge pull request #2907 in SNORT/snort3 from ~MDAGON/snort3:nhi_memory to master

Squashed commit of the following:

commit 5dc2f46fb2ec58c29d8760bc74274cdb51571da7
Author: Maya Dagon <mdagon@cisco.com>
Date:   Thu May 27 14:06:39 2021 -0400

    Code Review

commit ef675a1befeccbd27e3f0fd208a0726da17483ef
Author: Maya Dagon <mdagon@cisco.com>
Date:   Thu May 27 12:22:06 2021 -0400

    code review

commit b0cd942dddef767021e96dfbed2d47b0cc9c20c2
Author: Maya Dagon <mdagon@cisco.com>
Date:   Wed May 26 11:56:50 2021 -0400

    Remove sizeof(uint8_t) for consistency

commit 97cd8ecca1a45aa80c65ad31a2b54e91fff0209b
Author: Maya Dagon <mdagon@cisco.com>
Date:   Mon May 24 13:27:48 2021 -0400

    http_inspect: additional memory tracking

diff --git a/src/service_inspectors/http_inspect/http_field.cc b/src/service_inspectors/http_inspect/http_field.cc
index a0a2259b4e12e5c97ae97dadf2dc3066d3f28117..edad4c17e47fcc29faff06976bc6c2d6f8a5e460 100644 (file)
--- a/src/service_inspectors/http_inspect/http_field.cc
+++ b/src/service_inspectors/http_inspect/http_field.cc
@@ -23,6 +23,7 @@
 
 #include "http_field.h"
 
+#include "flow/flow_data.h"
 #include "http_common.h"
 #include "http_enum.h"
 #include "http_test_manager.h"
@@ -78,6 +79,18 @@ void Field::set(const Field& f)
     // Both Fields cannot be responsible for deleting the buffer so do not copy own_the_buffer
 }
 
+void Field::update_allocations(snort::FlowData* flow_data)
+{
+    if (own_the_buffer && (len > 0))
+        flow_data->update_allocations(len);
+}
+
+void Field::update_deallocations(snort::FlowData* flow_data)
+{
+    if (own_the_buffer && (len > 0))
+        flow_data->update_deallocations(len);
+}
+
 #ifdef REG_TEST
 void Field::print(FILE* output, const char* name) const
 {

diff --git a/src/service_inspectors/http_inspect/http_field.h b/src/service_inspectors/http_inspect/http_field.h
index 0246ee3315f032e330d801b12bb20ce11f4ba7b4..2356bc2aea8ffa1adf868f6b32479f776cffb423 100644 (file)
--- a/src/service_inspectors/http_inspect/http_field.h
+++ b/src/service_inspectors/http_inspect/http_field.h
@@ -27,6 +27,11 @@
 #include "http_common.h"
 #include "http_enum.h"
 
+namespace snort
+{
+class FlowData;
+}
+
 // Individual pieces of the message found during parsing.
 // Length values <= 0 are StatusCode values and imply that the start pointer is meaningless.
 // Never use the start pointer without verifying that length > 0.
@@ -46,6 +51,8 @@ public:
     void set(const Field& f);
     void set(HttpCommon::StatusCode stat_code);
     void set(int32_t length) { set(static_cast<HttpCommon::StatusCode>(length)); }
+    void update_allocations(snort::FlowData* flow_data);
+    void update_deallocations(snort::FlowData* flow_data);
 
 #ifdef REG_TEST
     void print(FILE* output, const char* name) const;

diff --git a/src/service_inspectors/http_inspect/http_flow_data.cc b/src/service_inspectors/http_inspect/http_flow_data.cc
index a986dac88c8cfbf528bf827b3feaebc109668e1c..9fd7e2eb5ebf628d3e5f7e318a0d36c4c3edd5da 100644 (file)
--- a/src/service_inspectors/http_inspect/http_flow_data.cc
+++ b/src/service_inspectors/http_inspect/http_flow_data.cc
@@ -247,6 +247,7 @@ bool HttpFlowData::add_to_pipeline(HttpTransaction* latest)
     {
         pipeline = new HttpTransaction*[MAX_PIPELINE];
         HttpModule::increment_peg_counts(PEG_PIPELINED_FLOWS);
+        update_allocations(sizeof(HttpTransaction*) * MAX_PIPELINE);
     }
     assert(!pipeline_overflow && !pipeline_underflow);
     int new_back = (pipeline_back+1) % MAX_PIPELINE;
@@ -279,6 +280,8 @@ void HttpFlowData::delete_pipeline()
     {
         delete pipeline[k];
     }
+    if (pipeline != nullptr)
+        update_deallocations(sizeof(HttpTransaction*) * MAX_PIPELINE);
     delete[] pipeline;
 }
 

diff --git a/src/service_inspectors/http_inspect/http_msg_head_shared.cc b/src/service_inspectors/http_inspect/http_msg_head_shared.cc
index de06b4050e6c92ff9e7e2225223ba26e242e00b8..300b5ac1c93825f6b3ac8197a5be42322809713a 100755 (executable)
--- a/src/service_inspectors/http_inspect/http_msg_head_shared.cc
+++ b/src/service_inspectors/http_inspect/http_msg_head_shared.cc
@@ -34,13 +34,34 @@ using namespace HttpCommon;
 using namespace HttpEnums;
 using namespace snort;
 
+// Memory calculation:
+// Approximations based on assumptions:
+// - there will be a cookie
+// - http_header and http_cookie rule options will be required
+// - classic normalization on the headers will not be trivial
+// - individual normalized headers won't be a lot and 500 bytes will cover them
+//
+// Header infrastructure (header_line, header_name, header_name_id, header_value):
+// session_data_->num_head_lines[source_id_] * (3 * sizeof(Field) + sizeof(HeaderId))
+//
+// The entire headers consist of http_raw_header and http_raw_cookie. Because there is
+// a cookie it will be necessary to write out the full msg_text into these two buffers,
+// resulting in allocations totaling approximately msg_text.length(). These raw buffers
+// in turn will need to be normalized, requiring another msg_text.length().
+// Total cost: 2 * msg_text.length().
+//
+// Plus 500 bytes for normalized headers.
 HttpMsgHeadShared::HttpMsgHeadShared(const uint8_t* buffer, const uint16_t buf_size, HttpFlowData* session_data_,
     HttpCommon::SourceId source_id_, bool buf_owner, snort::Flow* flow_,
     const HttpParaList* params_): HttpMsgSection(buffer, buf_size, session_data_, source_id_,
-    buf_owner, flow_, params_), own_msg_buffer(buf_owner)
+    buf_owner, flow_, params_), own_msg_buffer(buf_owner),
+    extra_memory_allocations(session_data_->num_head_lines[source_id_] *
+                            (3 * sizeof(Field) + sizeof(HeaderId)) + 2 * msg_text.length() + 500)
 {
     if (own_msg_buffer)
         session_data->update_allocations(msg_text.length());
+
+    session_data->update_allocations(extra_memory_allocations);
 }
 
 HttpMsgHeadShared::~HttpMsgHeadShared()
@@ -59,6 +80,8 @@ HttpMsgHeadShared::~HttpMsgHeadShared()
 
     if (own_msg_buffer)
         session_data->update_deallocations(msg_text.length());
+
+    session_data->update_deallocations(extra_memory_allocations);
 }
 
 // All the header processing that is done for every message (i.e. not just-in-time) is done here.

diff --git a/src/service_inspectors/http_inspect/http_msg_head_shared.h b/src/service_inspectors/http_inspect/http_msg_head_shared.h
index d84574e1306fe7a6b9cc31a50d1873c052f910d7..5a93fb23eead5c2ca9ceba947f9e3edc7a5fabb6 100755 (executable)
--- a/src/service_inspectors/http_inspect/http_msg_head_shared.h
+++ b/src/service_inspectors/http_inspect/http_msg_head_shared.h
@@ -132,6 +132,7 @@ private:
     bool file_cache_index_computed = false;
 
     bool own_msg_buffer;
+    const uint32_t extra_memory_allocations;
 };
 
 #endif

diff --git a/src/service_inspectors/http_inspect/http_msg_request.cc b/src/service_inspectors/http_inspect/http_msg_request.cc
index c14f8eafcfcb42d1ac1140070170eb5bd12cf44a..c59df1f49ce0ac4441085d2dd1a6741fdf682741 100644 (file)
--- a/src/service_inspectors/http_inspect/http_msg_request.cc
+++ b/src/service_inspectors/http_inspect/http_msg_request.cc
@@ -117,7 +117,7 @@ void HttpMsgRequest::parse_start_line()
     {
         uri = new HttpUri(start_line.start() + first_end + 1, last_begin - first_end - 1,
             method_id, params->uri_param, transaction->get_infractions(source_id),
-            session_data->events[source_id]);
+            session_data->events[source_id], session_data);
     }
     else
     {
@@ -162,7 +162,7 @@ bool HttpMsgRequest::handle_zero_nine()
                 uri_end--);
             uri = new HttpUri(start_line.start() + uri_begin, uri_end - uri_begin + 1, method_id,
                 params->uri_param, transaction->get_infractions(source_id),
-                session_data->events[source_id]);
+                session_data->events[source_id], session_data);
         }
         else
         {

diff --git a/src/service_inspectors/http_inspect/http_uri.cc b/src/service_inspectors/http_inspect/http_uri.cc
index 0b852af4bfa7f9f255048c3569c2888ea4bda33a..2507f01db494f76c159c1508e89840829b1f46b8 100644 (file)
--- a/src/service_inspectors/http_inspect/http_uri.cc
+++ b/src/service_inspectors/http_inspect/http_uri.cc
@@ -25,12 +25,28 @@
 
 #include "http_common.h"
 #include "http_enum.h"
+#include "http_flow_data.h"
 #include "hash/hash_key_operations.h"
 
 using namespace HttpCommon;
 using namespace HttpEnums;
 using namespace snort;
 
+HttpUri::HttpUri(const uint8_t* start, int32_t length, HttpEnums::MethodId method_id_,
+    const HttpParaList::UriParam& uri_param_, HttpInfractions* infractions_,
+    HttpEventGen* events_, HttpFlowData* session_data_) :
+    uri(length, start), infractions(infractions_), events(events_), method_id(method_id_),
+    uri_param(uri_param_), session_data(session_data_)
+{
+    normalize();
+    classic_norm.update_allocations(session_data);
+}
+
+HttpUri::~HttpUri()
+{
+    classic_norm.update_deallocations(session_data);
+}
+
 void HttpUri::parse_uri()
 {
     // Four basic types of HTTP URI
@@ -404,3 +420,4 @@ const Field& HttpUri::get_norm_host()
 
     return host_norm;
 }
+

diff --git a/src/service_inspectors/http_inspect/http_uri.h b/src/service_inspectors/http_inspect/http_uri.h
index cf63033043eba3a7d01de654839de7fcdc605ba8..d61c4cbe4bf7ee7ff41ad2b008e8d1810c59087a 100644 (file)
--- a/src/service_inspectors/http_inspect/http_uri.h
+++ b/src/service_inspectors/http_inspect/http_uri.h
@@ -20,11 +20,13 @@
 #ifndef HTTP_URI_H
 #define HTTP_URI_H
 
-#include "http_str_to_code.h"
+#include "http_event.h"
+#include "http_field.h"
 #include "http_module.h"
+#include "http_str_to_code.h"
 #include "http_uri_norm.h"
-#include "http_field.h"
-#include "http_event.h"
+
+class HttpFlowData;
 
 static const int MAX_SCHEME_LENGTH = 36; // schemes longer than 36 characters are malformed
 static const int LONG_SCHEME_LENGTH = 10; // schemes longer than 10 characters will alert
@@ -38,10 +40,8 @@ class HttpUri
 public:
     HttpUri(const uint8_t* start, int32_t length, HttpEnums::MethodId method_id_,
         const HttpParaList::UriParam& uri_param_, HttpInfractions* infractions_,
-        HttpEventGen* events_) :
-        uri(length, start), infractions(infractions_), events(events_), method_id(method_id_),
-        uri_param(uri_param_)
-        { normalize(); }
+        HttpEventGen* events_, HttpFlowData* session_data_);
+    ~HttpUri();
     const Field& get_uri() const { return uri; }
     HttpEnums::UriType get_uri_type() { return uri_type; }
     const Field& get_scheme() { return scheme; }
@@ -83,6 +83,7 @@ private:
     HttpEnums::UriType uri_type = HttpEnums::URI__NOT_COMPUTE;
     const HttpEnums::MethodId method_id;
     const HttpParaList::UriParam& uri_param;
+    HttpFlowData* const session_data;
 
     void normalize();
     void parse_uri();

diff --git a/src/service_inspectors/http_inspect/test/http_module_test.cc b/src/service_inspectors/http_inspect/test/http_module_test.cc
index 3aab7541ed57549eeac76e3763ae403d9df6e6a1..0d8f0fafc0c0cb81bc1f08b0f13c3f710cc8904d 100755 (executable)
--- a/src/service_inspectors/http_inspect/test/http_module_test.cc
+++ b/src/service_inspectors/http_inspect/test/http_module_test.cc
@@ -70,6 +70,8 @@ HttpJsNorm::HttpJsNorm(const HttpParaList::UriParam& uri_param_, int64_t normali
 HttpJsNorm::~HttpJsNorm() = default;
 void HttpJsNorm::configure(){}
 int64_t Parameter::get_int(char const*) { return 0; }
+void FlowData::update_allocations(unsigned long) {}
+void FlowData::update_deallocations(unsigned long) {}
 
 TEST_GROUP(http_peg_count_test)
 {

diff --git a/src/service_inspectors/http_inspect/test/http_msg_head_shared_util_test.cc b/src/service_inspectors/http_inspect/test/http_msg_head_shared_util_test.cc
index e57e5957edd8edd6fa7a84376b2a14843e5abff8..ddd840441e5893e4fd4e8a5bb5296e2dcba12996 100644 (file)
--- a/src/service_inspectors/http_inspect/test/http_msg_head_shared_util_test.cc
+++ b/src/service_inspectors/http_inspect/test/http_msg_head_shared_util_test.cc
@@ -37,6 +37,8 @@ using namespace snort;
 // Stubs whose sole purpose is to make the test code link
 long HttpTestManager::print_amount {};
 bool HttpTestManager::print_hex {};
+void FlowData::update_allocations(unsigned long) {}
+void FlowData::update_deallocations(unsigned long) {}
 
 // Tests for get_next_code()
 TEST_GROUP(get_next_code)

diff --git a/src/service_inspectors/http_inspect/test/http_normalizers_test.cc b/src/service_inspectors/http_inspect/test/http_normalizers_test.cc
index 9dfb54b79bea331b66330fcd8191ddac278b9f8e..29dcfefd3928ec26fae772a77a22487c41bbe02e 100644 (file)
--- a/src/service_inspectors/http_inspect/test/http_normalizers_test.cc
+++ b/src/service_inspectors/http_inspect/test/http_normalizers_test.cc
@@ -40,6 +40,8 @@ const bool HttpEnums::is_sp_tab[256] {};
 const bool HttpEnums::is_sp_tab_quote_dquote[256] {};
 long HttpTestManager::print_amount {};
 bool HttpTestManager::print_hex {};
+void FlowData::update_allocations(unsigned long) {}
+void FlowData::update_deallocations(unsigned long) {}
 
 TEST_GROUP(norm_decimal_integer_test) {};
 

diff --git a/src/service_inspectors/http_inspect/test/http_uri_norm_test.cc b/src/service_inspectors/http_inspect/test/http_uri_norm_test.cc
index f98e616a83665ae603b63b3df8af950ba1bed17c..a7e1b3fb7a612f2190c0321e604ce7432d061d96 100755 (executable)
--- a/src/service_inspectors/http_inspect/test/http_uri_norm_test.cc
+++ b/src/service_inspectors/http_inspect/test/http_uri_norm_test.cc
@@ -59,6 +59,8 @@ HttpJsNorm::HttpJsNorm(const HttpParaList::UriParam& uri_param_, int64_t normali
 HttpJsNorm::~HttpJsNorm() = default;
 void HttpJsNorm::configure() {}
 int64_t Parameter::get_int(char const*) { return 0; }
+void FlowData::update_allocations(unsigned long) {}
+void FlowData::update_deallocations(unsigned long) {}
 
 TEST_GROUP(http_inspect_uri_norm)
 {