Merge branch 'issue-15' into 'master'

cache and clean the VOMS AC only once per connection

Closes #15

See merge request storm2/ngx_http_voms_module!10

src/ngx_http_voms_module.cpp

@@ -22,6 +22,7 @@ extern "C" {
 #include <cassert>
 #include <chrono>
 #include <iostream>
+#include <map>
 #include <memory>
 #include <numeric>
 #include <string>
@@ -63,8 +64,6 @@ ngx_module_t ngx_http_voms_module = {
     NGX_MODULE_V1_PADDING  //
 };
 
-static std::unique_ptr<vomsdata> vomsdata_ptr;
-
 static ngx_int_t generic_getter(  //
     ngx_http_request_t* r,
     ngx_http_variable_value_t* v,
@@ -344,52 +343,62 @@ static MaybeVomsAc retrieve_voms_ac_from_proxy(ngx_http_request_t* r)
     return boost::none;
   }
 
-  if (!vomsdata_ptr) {
-    vomsdata_ptr.reset(new vomsdata);
-  }
-  auto ok =
-      vomsdata_ptr->Retrieve(client_cert.get(), client_chain, RECURSE_CHAIN);
+  vomsdata vd;
+
+  auto ok = vd.Retrieve(client_cert.get(), client_chain, RECURSE_CHAIN);
   if (!ok) {
-    auto msg = vomsdata_ptr->ErrorMessage().c_str();
+    auto msg = vd.ErrorMessage().c_str();
     ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "%s", msg);
     return boost::none;
   }
 
-  if (vomsdata_ptr->data.empty()) {
+  if (vd.data.empty()) {
     ngx_log_error(NGX_LOG_DEBUG, r->connection->log, 0, "no ACs in proxy");
     return boost::none;
   }
 
-  return vomsdata_ptr->data.front();
+  return vd.data.front();
 }
 
+// note that an entry in the cache is always created if within a connection a
+// request is made to one of the voms_* variables. If the presented
+// credential is a normal certificate or a proxy without attribute certificates,
+// the unique_ptr points to a MaybeVomsAc that is empty (in the sense of an
+// std::optional)
+static std::map<ngx_connection_t*, std::unique_ptr<MaybeVomsAc>> ac_cache;
+
 static void clean_voms_ac(void* data)
 {
-  auto r = static_cast<ngx_http_request_t*>(data);
-  ngx_log_error(NGX_LOG_DEBUG, r->connection->log, 0, "%s", __func__);
-
-  auto p = static_cast<MaybeVomsAc*>(
-      ngx_http_get_module_ctx(r, ngx_http_voms_module));
-  delete p;
+  auto c = static_cast<ngx_connection_t*>(data);
+  ngx_log_error(NGX_LOG_DEBUG, c->log, 0, "%s", __func__);
+  auto n = ac_cache.erase(c);
+  // we erase from the cache exactly once per connection
+  assert(n == 1);
 }
 
-static void cache_voms_ac(ngx_http_request_t* r, MaybeVomsAc* ac)
+static void cache_voms_ac(ngx_http_request_t* r,
+                          std::unique_ptr<MaybeVomsAc> acp)
 {
-  ngx_http_set_ctx(r, ac, ngx_http_voms_module);
-  auto cln = ngx_http_cleanup_add(r, 0);
+  ngx_log_error(NGX_LOG_DEBUG, r->connection->log, 0, "%s", __func__);
+  auto c = r->connection;
+  auto cln = ngx_pool_cleanup_add(c->pool, 0);
   if (cln) {
+    auto r = ac_cache.insert({c, std::move(acp)});
+    // we insert into the cache exactly once per connection
+    assert(r.second);
     cln->handler = clean_voms_ac;
-    cln->data = r;
+    cln->data = c;
   } else {
     ngx_log_error(
-        NGX_LOG_ERR, r->connection->log, 0, "ngx_http_cleanup_add() failed");
+        NGX_LOG_ERR, r->connection->log, 0, "ngx_pool_cleanup_add() failed");
   }
 }
 
 static MaybeVomsAc* get_voms_ac_from_cache(ngx_http_request_t* r)
 {
-  return static_cast<MaybeVomsAc*>(
-      ngx_http_get_module_ctx(r, ngx_http_voms_module));
+  ngx_log_error(NGX_LOG_DEBUG, r->connection->log, 0, "%s", __func__);
+  auto it = ac_cache.find(r->connection);
+  return it != ac_cache.end() ? it->second.get() : nullptr;
 }
 
 static MaybeVomsAc const& get_voms_ac(ngx_http_request_t* r)
@@ -399,8 +408,9 @@ static MaybeVomsAc const& get_voms_ac(ngx_http_request_t* r)
   MaybeVomsAc* acp = get_voms_ac_from_cache(r);
 
   if (!acp) {
-    acp = new MaybeVomsAc(retrieve_voms_ac_from_proxy(r));
-    cache_voms_ac(r, acp);
+    auto p = std::make_unique<MaybeVomsAc>(retrieve_voms_ac_from_proxy(r));
+    acp = p.get();
+    cache_voms_ac(r, std::move(p));
   }
 
   return *acp;

valgrind.suppress

+{
+   <insert_a_suppression_name_here>
+   Memcheck:Leak
+   match-leak-kinds: definite
+   fun:malloc
+   fun:CRYPTO_malloc
+   fun:SSL_SESSION_new
+   fun:ssl_get_new_session
+   fun:ssl23_connect
+   fun:ngx_ssl_handshake
+   fun:ngx_http_upstream_ssl_init_connection
+   fun:ngx_http_upstream_send_request_handler
+   fun:ngx_http_upstream_handler
+   fun:ngx_epoll_process_events
+   fun:ngx_process_events_and_timers
+   fun:ngx_single_process_cycle
+   fun:main
+}
+{
+   <insert_a_suppression_name_here>
+   Memcheck:Leak
+   match-leak-kinds: definite
+   fun:malloc
+   fun:ngx_alloc
+   fun:ngx_set_environment
+   fun:ngx_single_process_cycle
+   fun:main
+}
+{
+   <insert_a_suppression_name_here>
+   Memcheck:Free
+   fun:free
+   fun:__libc_freeres
+   fun:_vgnU_freeres
+   fun:__run_exit_handlers
+   fun:exit
+   fun:ngx_master_process_exit
+   fun:ngx_single_process_cycle
+   fun:main
+}
+{
+   <insert_a_suppression_name_here>
+   Memcheck:Param
+   epoll_ctl(event)
+   fun:epoll_ctl
+   fun:ngx_epoll_test_rdhup
+   fun:ngx_epoll_init
+   fun:ngx_event_process_init
+   fun:ngx_single_process_cycle
+   fun:main
+}
+{
+   <insert_a_suppression_name_here>
+   Memcheck:Param
+   epoll_ctl(event)
+   fun:epoll_ctl
+   fun:ngx_epoll_test_rdhup
+   fun:ngx_epoll_init
+   fun:ngx_event_process_init
+   fun:ngx_single_process_cycle
+   fun:main
+}