opentracing-contrib / nginx-opentracing

NGINX plugin for OpenTracing
Apache License 2.0
500 stars 121 forks source link

Possible null dereference in opentracing_variable.cpp #669

Open suhovv opened 3 months ago

suhovv commented 3 months ago

File: opentracing_variable.cpp line: 108, 115

To avoid possible nullptr dereference, you should add a check for the result of the ngx_http_add_variable function call. If the function returns nullptr, you should handle this situation correctly, for example, write an error message to the log and return the corresponding error code from the add_variables function.

For the line opentracing_binary_context_var->get_handler = expand_opentracing_binary_context_variable; the same principles of error checking and handling apply as in the case of the opentracing_context_var variable

Example of correction:

ngx_int_t add_variables(ngx_conf_t* cf) noexcept {
  // add opentracing_context
  auto opentracing_context = to_ngx_str(opentracing_context_variable_name);
  auto opentracing_context_var = ngx_http_add_variable(
      cf, &opentracing_context,
      NGX_HTTP_VAR_NOCACHEABLE | NGX_HTTP_VAR_NOHASH | NGX_HTTP_VAR_PREFIX);

  // check nullptr
  if (opentracing_context_var == nullptr) {
    ngx_log_error(NGX_LOG_ERR, cf->log, 0, "Failed to add variable: %V", &opentracing_context);
    return NGX_ERROR;
  }

  opentracing_context_var->get_handler = expand_opentracing_context_variable;
  opentracing_context_var->data = 0;

  // add opentracing_binary_context
  auto opentracing_binary_context =
      to_ngx_str(opentracing_binary_context_variable_name);
  auto opentracing_binary_context_var = ngx_http_add_variable(
      cf, &opentracing_binary_context, NGX_HTTP_VAR_NOCACHEABLE);

  // check nullptr
  if (opentracing_binary_context_var == nullptr) {
    ngx_log_error(NGX_LOG_ERR, cf->log, 0, "Failed to add variable: %V", &opentracing_binary_context);
    return NGX_ERROR;
  }

  opentracing_binary_context_var->get_handler =
      expand_opentracing_binary_context_variable;
  opentracing_binary_context_var->data = 0;

  return NGX_OK;
}
miry commented 3 months ago

@snisorig Thank you for reporting. Do you think you can prepare a PR with tests to cover that problem?

suhovv commented 3 months ago

@miry Sorry, but I have little experience in this.