fix(nginx): Fix segfault in GetTraceContext (#153)

* Only try to evaluate the request variables if tracing is enabled for the current request

* If a parent request is not traced the map maybe null

* changed test to catch (now fixed) segfault

instrumentation/nginx/src/otel_ngx_module.cpp

@@ -172,6 +172,20 @@ static ngx_http_variable_t otel_ngx_variables[] = {
   ngx_http_null_variable,
 };
 
+static bool IsOtelEnabled(ngx_http_request_t* req) {
+  OtelNgxLocationConf* locConf = GetOtelLocationConf(req);
+  if (locConf->enabled) {
+#if (NGX_PCRE)
+    int ovector[3];
+    return locConf->ignore_paths == nullptr || ngx_regex_exec(locConf->ignore_paths, &req->unparsed_uri, ovector, 0) < 0;
+#else
+    return true;
+#endif
+  } else {
+    return false;
+  }
+}
+
 TraceContext* GetTraceContext(ngx_http_request_t* req) {
   ngx_http_variable_value_t* val = ngx_http_get_indexed_variable(req, otel_ngx_variables[0].index);
 
@@ -201,6 +215,12 @@ nostd::string_view WithoutOtelVarPrefix(ngx_str_t value) {
 
 static ngx_int_t
 OtelGetTraceContextVar(ngx_http_request_t* req, ngx_http_variable_value_t* v, uintptr_t data) {
+  if (!IsOtelEnabled(req)) {
+    v->valid = 0;
+    v->not_found = 1;
+    return NGX_OK;
+  }
+
   TraceContext* traceContext = GetTraceContext(req);
 
   if (traceContext == nullptr || !traceContext->request_span) {
@@ -235,6 +255,12 @@ OtelGetTraceContextVar(ngx_http_request_t* req, ngx_http_variable_value_t* v, ui
 
 static ngx_int_t
 OtelGetTraceId(ngx_http_request_t* req, ngx_http_variable_value_t* v, uintptr_t data) {
+  if (!IsOtelEnabled(req)) {
+    v->valid = 0;
+    v->not_found = 1;
+    return NGX_OK;
+  }
+
   TraceContext* traceContext = GetTraceContext(req);
 
   if (traceContext == nullptr || !traceContext->request_span) {
@@ -284,6 +310,12 @@ OtelGetTraceId(ngx_http_request_t* req, ngx_http_variable_value_t* v, uintptr_t
 
 static ngx_int_t
 OtelGetSpanId(ngx_http_request_t* req, ngx_http_variable_value_t* v, uintptr_t data) {
+  if (!IsOtelEnabled(req)) {
+    v->valid = 0;
+    v->not_found = 1;
+    return NGX_OK;
+  }
+
   TraceContext* traceContext = GetTraceContext(req);
 
   if (traceContext == nullptr || !traceContext->request_span) {
@@ -373,28 +405,14 @@ nostd::string_view GetNgxServerName(const ngx_http_request_t* req) {
   return FromNgxString(cscf->server_name);
 }
 
-static bool IsOtelEnabled(ngx_http_request_t* req) {
-  OtelNgxLocationConf* locConf = GetOtelLocationConf(req);
-  if (locConf->enabled) {
-#if (NGX_PCRE)
-    int ovector[3];
-    return locConf->ignore_paths == nullptr || ngx_regex_exec(locConf->ignore_paths, &req->unparsed_uri, ovector, 0) < 0;
-#else
-    return true;
-#endif
-  } else {
-    return false;
-  }
-}
-
 TraceContext* CreateTraceContext(ngx_http_request_t* req, ngx_http_variable_value_t* val) {
   ngx_pool_cleanup_t* cleanup = ngx_pool_cleanup_add(req->pool, sizeof(TraceContext));
   TraceContext* context = (TraceContext*)cleanup->data;
   new (context) TraceContext(req);
   cleanup->handler = TraceContextCleanup;
 
   std::unordered_map<ngx_http_request_t*, TraceContext*>* map;
-  if (req->parent) {
+  if (req->parent && val->data) {
     // Subrequests will already have the map created so just retrieve it
     map = (std::unordered_map<ngx_http_request_t*, TraceContext*>*)val->data;
   } else {

instrumentation/nginx/test/backend/php/ignored.php

@@ -0,0 +1,9 @@
+<?php
+  $traceparent = $_SERVER["HTTP_TRACEPARENT"];
+  if (preg_match("/00-[0-9a-f]{32}-[0-9a-f]{16}-0[0-1]/", $traceparent ?? "")) {
+    echo("valid  traceparent");
+    throw new Exception("valid traceparent");
+  }
+
+  header("Content-Type: application/json");
+?>

instrumentation/nginx/test/conf/nginx.conf

@@ -5,7 +5,7 @@ events {}
 http {
   opentelemetry_config /conf/otel-nginx.toml;
   opentelemetry_operation_name otel_test;
-  opentelemetry_ignore_paths .*\.js;
+  opentelemetry_ignore_paths ignored.php;
   access_log stderr;
   error_log stderr debug;
 

instrumentation/nginx/test/instrumentation/test/instrumentation_test.exs

@@ -398,7 +398,7 @@ defmodule InstrumentationTest do
 
   test "Accessing a excluded uri produces no span", %{trace_file: trace_file} do
     %HTTPoison.Response{status_code: status, body: body} =
-      HTTPoison.get!("#{@host}/files/file.js")
+      HTTPoison.get!("#{@host}/ignored.php")
 
     assert_raise RuntimeError, "timed out waiting for traces", fn ->
       read_traces(trace_file, 1)