fix: bypass context propagation if headers are not initialized (#213)

When processing HTTP/0.9 requests, the HTTP headers data structure
is not initialized because this version of HTTP does not support headers.
This leads to a crash when the code attempts to add context propagation
information to an uninitialized list of headers.

We have implemented a check to bypass context propagation if the headers
are not initialized.

Resolves #212

Changes:
  - move headers logic into `common` folder.
  - add integration test with HTTP/0.9.

Author: dmehala

CMakeLists.txt

@@ -138,6 +138,7 @@ target_sources(ngx_http_datadog_objs
   PRIVATE
     src/common/variable.cpp
     src/common/directives.cpp
+    src/common/headers.cpp
     src/array_util.cpp
     src/datadog_conf.cpp
     src/datadog_conf_handler.cpp

src/common/headers.cpp

@@ -0,0 +1,68 @@
+#include "headers.h"
+
+#include "string_util.h"
+
+namespace datadog::common {
+
+ngx_table_elt_t *search_header(ngx_list_t &headers, std::string_view key) {
+  ngx_list_part_t *part = &headers.part;
+  auto *h = static_cast<ngx_table_elt_t *>(part->elts);
+
+  for (std::size_t i = 0;; i++) {
+    if (i >= part->nelts) {
+      if (part->next == nullptr) {
+        break;
+      }
+
+      part = part->next;
+      h = static_cast<ngx_table_elt_t *>(part->elts);
+      i = 0;
+    }
+
+    if (key.size() != h[i].key.len ||
+        ngx_strncasecmp((u_char *)key.data(), h[i].key.data, key.size()) != 0) {
+      continue;
+    }
+
+    return &h[i];
+  }
+
+  return nullptr;
+}
+
+bool add_header(ngx_pool_t &pool, ngx_list_t &headers, std::string_view key,
+                std::string_view value) {
+  if (headers.last == nullptr) {
+    // Certainly a bad request (4xx). No need to add HTTP headers.
+    return false;
+  }
+
+  ngx_table_elt_t *h = static_cast<ngx_table_elt_t *>(ngx_list_push(&headers));
+  if (h == nullptr) {
+    return false;
+  }
+
+  const auto key_size = key.size();
+
+  // This trick tells ngx_http_header_module to reflect the header value
+  // in the actual response. Otherwise the header will be ignored and client
+  // will never see it. To date the value must be just non zero.
+  // Source:
+  // <https://web.archive.org/web/20240409072840/https://www.nginx.com/resources/wiki/start/topics/examples/headers_management/>
+  h->hash = 1;
+
+  // HTTP proxy module expects the header to has a lowercased key value
+  // Instead of allocating twice the same key, `h->key` and `h->lowcase_key`
+  // use the same data.
+  h->key.len = key_size;
+  h->key.data = (u_char *)ngx_pnalloc(&pool, sizeof(char) * key_size);
+  for (std::size_t i = 0; i < key_size; ++i) {
+    h->key.data[i] = nginx::to_lower(key[i]);
+  }
+  h->lowcase_key = h->key.data;
+
+  h->value = nginx::to_ngx_str(&pool, value);
+  return true;
+}
+
+}  // namespace datadog::common

src/common/headers.h

@@ -0,0 +1,53 @@
+#pragma once
+
+extern "C" {
+#include <ngx_core.h>
+#include <ngx_hash.h>
+}
+
+#include <string_view>
+
+namespace datadog::common {
+
+/// Searches through an NGINX header list to find a header with a matching key.
+///
+/// @param headers
+///     A reference to an NGINX-style list (`ngx_list_t`) containing
+///     `ngx_table_elt_t` elements, typically representing HTTP headers.
+///
+/// @param key
+///     A string view representing the name of the header to search for.
+///     The comparison is typically case-insensitive.
+///
+/// @return
+///     A pointer to the matching `ngx_table_elt_t` header element if found,
+///     or `nullptr` if no header with the given key exists in the list.
+////
+ngx_table_elt_t *search_header(ngx_list_t &headers, std::string_view key);
+
+/// Adds a new HTTP header to an NGINX-style header list.
+///
+/// @param pool
+///     A reference to the NGINX memory pool (`ngx_pool_t`) used for allocating
+///     memory for the new header and its key/value strings. This pool must
+///     remain valid for the lifetime of the header.
+///
+/// @param headers
+///     A reference to an `ngx_list_t` representing the list of HTTP headers
+///     (`ngx_table_elt_t` elements) to which the new header will be appended.
+///
+/// @param key
+///     A string view representing the header name (e.g., "Content-Type").
+///     This will be copied into the NGINX pool memory before insertion.
+///
+/// @param value
+///     A string view representing the header value (e.g., "application/json").
+///     This will also be copied into the NGINX pool memory.
+///
+/// @return
+///     `true` if the header was successfully added to the list;
+///     `false` if memory allocation failed or the list could not be updated.
+bool add_header(ngx_pool_t &pool, ngx_list_t &headers, std::string_view key,
+                std::string_view value);
+
+}  // namespace datadog::common

src/ngx_header_writer.h

@@ -2,6 +2,7 @@
 
 #include <datadog/dict_writer.h>
 
+#include "common/headers.h"
 #include "string_util.h"
 
 extern "C" {
@@ -23,66 +24,14 @@ class NgxHeaderWriter : public datadog::tracing::DictWriter {
       : request_(request), pool_(request_->pool) {}
 
   void set(std::string_view key, std::string_view value) override {
-    const auto key_size = key.size();
-
-    ngx_table_elt_t *h = search_header(key);
+    ngx_table_elt_t *h =
+        common::search_header(request_->headers_in.headers, key);
     if (h != nullptr) {
       h->value = to_ngx_str(pool_, value);
     } else {
-      h = static_cast<ngx_table_elt_t *>(
-          ngx_list_push(&request_->headers_in.headers));
-      if (h == nullptr) {
-        return;
-      }
-
-      // This trick tells ngx_http_header_module to reflect the header value
-      // in the actual response. Otherwise the header will be ignored and client
-      // will never see it. To date the value must be just non zero.
-      // Source:
-      // <https://web.archive.org/web/20240409072840/https://www.nginx.com/resources/wiki/start/topics/examples/headers_management/>
-      h->hash = 1;
-
-      // HTTP proxy module expects the header to has a lowercased key value
-      // Instead of allocating twice the same key, `h->key` and `h->lowcase_key`
-      // use the same data.
-      h->key.len = key_size;
-      h->key.data = (u_char *)ngx_pnalloc(pool_, sizeof(char) * key_size);
-      for (std::size_t i = 0; i < key_size; ++i) {
-        h->key.data[i] = to_lower(key[i]);
-      }
-      h->lowcase_key = h->key.data;
-
-      h->value = to_ngx_str(pool_, value);
+      common::add_header(*pool_, request_->headers_in.headers, key, value);
     }
   }
-
- private:
-  ngx_table_elt_t *search_header(std::string_view key) {
-    ngx_list_part_t *part = &request_->headers_in.headers.part;
-    auto *h = static_cast<ngx_table_elt_t *>(part->elts);
-
-    for (std::size_t i = 0;; i++) {
-      if (i >= part->nelts) {
-        if (part->next == nullptr) {
-          break;
-        }
-
-        part = part->next;
-        h = static_cast<ngx_table_elt_t *>(part->elts);
-        i = 0;
-      }
-
-      if (key.size() != h[i].key.len ||
-          ngx_strncasecmp((u_char *)key.data(), h[i].key.data, key.size()) !=
-              0) {
-        continue;
-      }
-
-      return &h[i];
-    }
-
-    return nullptr;
-  }
 };
 
 }  // namespace nginx

src/rum/injection.cpp

@@ -8,6 +8,7 @@ extern "C" {
 
 #include <cassert>
 
+#include "common/headers.h"
 #include "datadog_conf.h"
 #include "ngx_http_datadog_module.h"
 #include "string_util.h"
@@ -18,32 +19,6 @@ namespace nginx {
 namespace rum {
 namespace {
 
-ngx_table_elt_t *search_header(ngx_list_t &headers, std::string_view key) {
-  ngx_list_part_t *part = &headers.part;
-  auto *h = static_cast<ngx_table_elt_t *>(part->elts);
-
-  for (std::size_t i = 0;; i++) {
-    if (i >= part->nelts) {
-      if (part->next == nullptr) {
-        break;
-      }
-
-      part = part->next;
-      h = static_cast<ngx_table_elt_t *>(part->elts);
-      i = 0;
-    }
-
-    if (key.size() != h[i].key.len ||
-        ngx_strcasecmp((u_char *)key.data(), h[i].key.data) != 0) {
-      continue;
-    }
-
-    return &h[i];
-  }
-
-  return nullptr;
-}
-
 bool is_html_content(ngx_str_t *content_type) {
   assert(content_type != nullptr);
   std::string_view content_type_sv = to_string_view(*content_type);
@@ -62,20 +37,14 @@ InjectionHandler::~InjectionHandler() {
 }
 
 ngx_int_t InjectionHandler::on_rewrite_handler(ngx_http_request_t *r) {
-  ngx_table_elt_t *h =
-      static_cast<ngx_table_elt_t *>(ngx_list_push(&r->headers_in.headers));
-  if (h == nullptr) {
+  if (!common::add_header(r->pool, r->headers_in.headers,
+                          "x-datadog-rum-injection-pending", "1")) {
     ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
                   "RUM SDK injection failed: unable to add "
                   "x-datadog-rum-injection-pending HTTP header");
     return NGX_ERROR;
   }
 
-  ngx_str_set(&h->key, "x-datadog-rum-injection-pending");
-  ngx_str_set(&h->value, "1");
-  h->lowcase_key = h->key.data;
-  h->hash = 1;
-
   return NGX_DECLINED;
 }
 
@@ -88,8 +57,8 @@ ngx_int_t InjectionHandler::on_header_filter(
     return next_header_filter(r);
   }
 
-  if (auto injected_header =
-          search_header(r->headers_in.headers, "x-datadog-rum-injected");
+  if (auto injected_header = common::search_header(r->headers_in.headers,
+                                                   "x-datadog-rum-injected");
       injected_header != nullptr) {
     if (nginx::to_string_view(injected_header->value) == "1") {
       ngx_log_error(NGX_LOG_INFO, r->connection->log, 0,

test/cases/auto_propagation/conf/http_auto.conf

@@ -13,6 +13,10 @@ http {
 
         proxy_set_header server-block-header not-hidden-by-autoinjection;
 
+        location / {
+            return 200 "Hello, World!";
+        }
+
         location /http {
             proxy_pass http://http:8080;
         }

test/cases/auto_propagation/test_http.py

@@ -69,6 +69,19 @@ def test_without_module(self):
         return self.run_test("./conf/http_without_module.conf",
                              should_propagate=False)
 
+    def test_illformated_request(self):
+        # From: <https://github.com/DataDog/nginx-datadog/issues/212>
+        conf_path = Path(__file__).parent / "./conf/http_auto.conf"
+        conf_text = conf_path.read_text()
+        status, log_lines = self.orch.nginx_replace_config(
+            conf_text, conf_path.name)
+        self.assertEqual(status, 0, log_lines)
+
+        status, response = self.orch.send_nginx_raw_http_request(
+            request_line="GET /\r\n")
+        self.assertEqual(0, status)
+        self.assertEqual("Hello, World!", response)
+
     def run_test(self, conf_relative_path, should_propagate):
         conf_path = Path(__file__).parent / conf_relative_path
         conf_text = conf_path.read_text()

test/cases/orchestration.py

@@ -600,6 +600,30 @@ def send_nginx_websocket_request(
         )
         return result.returncode, result.stdout
 
+    def send_nginx_raw_http_request(self, port=80, request_line=""):
+        """Send the request line to nginx, and return the resulting HTTP
+        status code and response body as a tuple `(status, body)`.
+        """
+        command = docker_compose_command(
+            "exec",
+            "-T",
+            "--",
+            "client",
+            "nc",
+            "nginx",
+            str(port),
+        )
+
+        result = subprocess.run(
+            command,
+            input=request_line,
+            stdout=subprocess.PIPE,
+            env=child_env(),
+            encoding="utf8",
+            check=True,
+        )
+        return result.returncode, result.stdout
+
     def send_nginx_http_request(
         self,
         path,