Merge pull request #3135 in SNORT/snort3 from ~KATHARVE/snort3:hpack-refactor to...

Squashed commit of the following:

commit c6891e039474b8ce2b2c0f318fe2dd053bac550b
Author: Katura Harvey <katharve@cisco.com>
Date:   Wed Oct 27 10:41:52 2021 -0400

    http2_inspect: refactor decoded_headers_buffer for hpack decoding

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.cc b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
index 6dad237f401a238896a143f195824db7b941ab57..dcfa41049d9119d9b2123f21504eac89f7e9e4e7 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.cc
@@ -51,13 +51,12 @@ Http2HeadersFrame::Http2HeadersFrame(const uint8_t* header_buffer, const uint32_
 
     // Set up HPACK decoding
     hpack_decoder = &session_data->hpack_decoder[source_id];
-    decoded_headers = new uint8_t[MAX_OCTETS];
 }
 
 
 Http2HeadersFrame::~Http2HeadersFrame()
 {
-    delete[] decoded_headers;
+    hpack_decoder->cleanup();
 }
 
 void Http2HeadersFrame::clear()
@@ -74,10 +73,9 @@ void Http2HeadersFrame::process_decoded_headers(HttpFlowData* http_flow, SourceI
     if (session_data->abort_flow[source_id])
         return;
 
-    http1_header = hpack_decoder->get_decoded_headers(decoded_headers);
+    hpack_decoder->set_decoded_headers(http1_header);
     StreamBuffer stream_buf;
 
-
     // http_inspect scan() of headers
     // If we're processing a header truncated immediately after the start line, http1_header will
     // be empty. Don't call scan on the empty buffer because it will create a cutter and the check

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame.h b/src/service_inspectors/http2_inspect/http2_headers_frame.h
index 8a1b74eaee95e86e2bd31d5f74609b3f9001e782..08f6b85d1b9ea41838d0af6fd265cb9f089bf430 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame.h
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame.h
@@ -50,7 +50,6 @@ protected:
     void process_decoded_headers(HttpFlowData* http_flow, HttpCommon::SourceId hi_source_id);
     uint8_t get_flags_mask() const override;
 
-    uint8_t* decoded_headers = nullptr; // working buffer to store decoded headers
     Field http1_header;                 // finalized headers to be passed to NHI
     uint32_t xtradata_mask = 0;
     bool detection_required = false;

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
index a8651422190599b8399fbe8bcc5f76e6c3fc3dac..e48db69e6a31ac7e39d459a9d32a1758fcde023a 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_header.cc
@@ -57,7 +57,7 @@ Http2HeadersFrameHeader::Http2HeadersFrameHeader(const uint8_t* header_buffer,
     const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ?
         data.length() - hpack_headers_offset : 0;
     if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset),
-        encoded_headers_length, decoded_headers, start_line_generator, false))
+        encoded_headers_length, start_line_generator, false))
     {
         if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE))
         {

diff --git a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
index e90a879b460798513c281c565fcb92b69f856d01..ccf9d3b67f92b241df887d53c5925cf6699c265a 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
+++ b/src/service_inspectors/http2_inspect/http2_headers_frame_trailer.cc
@@ -59,7 +59,7 @@ Http2HeadersFrameTrailer::Http2HeadersFrameTrailer(const uint8_t* header_buffer,
     const uint32_t encoded_headers_length = (data.length() > hpack_headers_offset) ?
         data.length() - hpack_headers_offset : 0;
     if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset),
-        encoded_headers_length, decoded_headers, nullptr, true))
+        encoded_headers_length, nullptr, true))
     {
         if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE))
         {

diff --git a/src/service_inspectors/http2_inspect/http2_hpack.cc b/src/service_inspectors/http2_inspect/http2_hpack.cc
index 6d23cd12d6923c7247d4e69794b43971c16016e0..f9c9c2b11644f8728469ba076445982a3d1b88c7 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_hpack.cc
+++ b/src/service_inspectors/http2_inspect/http2_hpack.cc
@@ -370,7 +370,7 @@ bool Http2HpackDecoder::decode_header_line(const uint8_t* encoded_header_buffer,
 // does not output the start line or decoded headers - this function must be followed by calls to
 // generate_start_line() and get_decoded_headers() to generate and obtain these fields.
 bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers,
-    const uint32_t encoded_headers_length, uint8_t* decoded_headers,
+    const uint32_t encoded_headers_length,
     Http2StartLine *start_line_generator, bool trailers)
 {
     uint32_t total_bytes_consumed = 0;
@@ -378,7 +378,7 @@ bool Http2HpackDecoder::decode_headers(const uint8_t* encoded_headers,
     uint32_t line_bytes_written = 0;
     bool success = true;
     start_line = start_line_generator;
-    decoded_headers_size = 0;
+    decoded_headers = new uint8_t[MAX_OCTETS];
     is_trailers = trailers;
     pseudo_headers_allowed = !is_trailers;
 
@@ -425,7 +425,24 @@ void Http2HpackDecoder::settings_table_size_update(uint32_t new_size)
     decode_table.get_dynamic_table().update_size(new_size);
 }
 
-Field Http2HpackDecoder::get_decoded_headers(const uint8_t* const decoded_headers)
+void Http2HpackDecoder::set_decoded_headers(Field& http1_header)
+{
+    assert(decoded_headers);
+    http1_header.set(decoded_headers_size, decoded_headers, true);
+
+    // The headers frame object now owns this buffer
+    decoded_headers = nullptr;
+    decoded_headers_size = 0;
+}
+
+void Http2HpackDecoder::cleanup()
 {
-    return Field(decoded_headers_size, decoded_headers, false);
+    if (decoded_headers)
+    {
+        // get_decoded_headers() was never called because we encountered some error during
+        // processing. We must clean up the buffer.
+        delete[] decoded_headers;
+        decoded_headers = nullptr;
+        decoded_headers_size = 0;
+    }
 }

diff --git a/src/service_inspectors/http2_inspect/http2_hpack.h b/src/service_inspectors/http2_inspect/http2_hpack.h
index 1a0b5f90a41a5b81da145b315da25b220de7d6b2..9d0f8c82ddeee39643536a6ebc89f3cb565ed568 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_hpack.h
+++ b/src/service_inspectors/http2_inspect/http2_hpack.h
@@ -46,7 +46,7 @@ public:
         session_data(flow_data), events(_events), infractions(_infractions), source_id(src_id),
         decode_table(flow_data) { }
     bool decode_headers(const uint8_t* encoded_headers, const uint32_t encoded_headers_length,
-        uint8_t* decoded_headers, Http2StartLine* start_line, bool trailers);
+         Http2StartLine* start_line, bool trailers);
     bool write_decoded_headers(const uint8_t* in_buffer, const uint32_t in_length,
         uint8_t* decoded_header_buffer, uint32_t decoded_header_length, uint32_t& bytes_written);
     bool decode_header_line(const uint8_t* encoded_header_buffer,
@@ -80,15 +80,16 @@ public:
         uint32_t& bytes_written, Field& field);
 
     bool finalize_start_line();
-    const Field* get_start_line();
-    Field get_decoded_headers(const uint8_t* const decoded_headers);
+    void set_decoded_headers(Field&);
     bool are_pseudo_headers_allowed() { return pseudo_headers_allowed; }
     void settings_table_size_update(const uint32_t size);
+    void cleanup();
 
 private:
     Http2StartLine* start_line;
     bool pseudo_headers_allowed;
-    uint32_t decoded_headers_size;
+    uint8_t* decoded_headers = nullptr; // working buffer to store decoded headers
+    uint32_t decoded_headers_size = 0;
     Http2FlowData* session_data;
     Http2EventGen* const events;
     Http2Infractions* const infractions;

diff --git a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
index f5afb6ecb6e3119831979245c0e201b9b03a45b4..89b7841e082c1b98147f7771d3ef57dc0ebf2a95 100644 (file)
--- a/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
+++ b/src/service_inspectors/http2_inspect/http2_push_promise_frame.cc
@@ -67,7 +67,7 @@ Http2PushPromiseFrame::Http2PushPromiseFrame(const uint8_t* header_buffer,
 
     // Decode headers
     if (!hpack_decoder->decode_headers((data.start() + hpack_headers_offset), data.length() -
-        hpack_headers_offset, decoded_headers, start_line_generator, false))
+        hpack_headers_offset, start_line_generator, false))
     {
         if (!(*session_data->infractions[source_id] & INF_TRUNCATED_HEADER_LINE))
         {