Move MMDB auto-reloading to log phase

Previous approach could lead to SIGSEGV while logging requests
with log format containing variables produed by geoip2 module.

ngx_http_geoip2_module.c

@@ -44,8 +44,6 @@ typedef struct {
 } ngx_http_geoip2_metadata_t;
 
 
-static ngx_int_t ngx_http_geoip2_reload(ngx_http_geoip2_db_t *database,
-    ngx_log_t *log);
 static ngx_int_t ngx_http_geoip2_variable(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data);
 static ngx_int_t ngx_http_geoip2_metadata(ngx_http_request_t *r,
@@ -67,6 +65,7 @@ static char *ngx_http_geoip2_proxy(ngx_conf_t *cf, ngx_command_t *cmd,
 static ngx_int_t ngx_http_geoip2_cidr_value(ngx_conf_t *cf, ngx_str_t *net,
     ngx_cidr_t *cidr);
 static void ngx_http_geoip2_cleanup(void *data);
+static ngx_int_t ngx_http_geoip2_init(ngx_conf_t *cf);
 
 
 #define FORMAT(fmt, ...) do {                           \
@@ -107,7 +106,7 @@ static ngx_command_t  ngx_http_geoip2_commands[] = {
 
 static ngx_http_module_t  ngx_http_geoip2_module_ctx = {
     NULL,                                  /* preconfiguration */
-    NULL,                                  /* preconfiguration */
+    ngx_http_geoip2_init,                  /* postconfiguration */
 
     ngx_http_geoip2_create_conf,           /* create main configuration */
     ngx_http_geoip2_init_conf,             /* init main configuration */
@@ -136,41 +135,6 @@ ngx_module_t  ngx_http_geoip2_module = {
 };
 
 
-static ngx_int_t
-ngx_http_geoip2_reload(ngx_http_geoip2_db_t *database, ngx_log_t *log)
-{
-    struct stat  attr;
-    MMDB_s       tmpdb;
-    int          status;
-
-    if (database->check_interval > 0
-            && database->last_check + database->check_interval <= ngx_time()) {
-        database->last_check = ngx_time();
-        stat(database->mmdb.filename, &attr);
-
-        if (attr.st_mtime > database->last_change) {
-            status = MMDB_open(database->mmdb.filename, MMDB_MODE_MMAP, &tmpdb);
-
-            if (status != MMDB_SUCCESS) {
-                ngx_log_error(NGX_LOG_ERR, log, 0,
-                                "MMDB_open(\"%s\") failed to reload - %s",
-                                database->mmdb.filename, MMDB_strerror(status));
-                return NGX_ERROR;
-            }
-
-            database->last_change = attr.st_mtime;
-            MMDB_close(&database->mmdb);
-            database->mmdb = tmpdb;
-
-            ngx_log_error(NGX_LOG_INFO, log, 0, "Reload MMDB \"%s\"",
-                                tmpdb.filename);
-        }
-    }
-
-    return NGX_OK;
-}
-
-
 static ngx_int_t
 ngx_http_geoip2_variable(ngx_http_request_t *r, ngx_http_variable_value_t *v,
     uintptr_t data)
@@ -191,8 +155,6 @@ ngx_http_geoip2_variable(ngx_http_request_t *r, ngx_http_variable_value_t *v,
     unsigned long address;
 #endif
 
-    ngx_http_geoip2_reload(database, r->connection->log);
-
     if (geoip2->source.value.len > 0) {
          if (ngx_http_complex_value(r, &geoip2->source, &val) != NGX_OK) {
              goto not_found;
@@ -338,8 +300,6 @@ ngx_http_geoip2_metadata(ngx_http_request_t *r, ngx_http_variable_value_t *v,
     ngx_http_geoip2_db_t        *database = metadata->database;
     u_char                      *p;
 
-    ngx_http_geoip2_reload(database, r->connection->log);
-
     if (ngx_strncmp(metadata->metavalue.data, "build_epoch", 11) == 0) {
         FORMAT("%uL", database->mmdb.metadata.build_epoch);
     } else if (ngx_strncmp(metadata->metavalue.data, "last_check", 10) == 0) {
@@ -735,3 +695,94 @@ ngx_http_geoip2_cleanup(void *data)
         ngx_array_destroy(gcf->databases);
     }
 }
+
+
+static ngx_int_t
+ngx_http_geoip2_log_handler(ngx_http_request_t *r)
+{
+    int                      status;
+    MMDB_s                   tmpdb;
+    ngx_uint_t               i;
+    ngx_file_info_t          fi;
+    ngx_http_geoip2_db_t    *database;
+    ngx_http_geoip2_conf_t  *gcf;
+
+    ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+                   "geoip2 http log handler");
+
+    gcf = ngx_http_get_module_main_conf(r, ngx_http_geoip2_module);
+
+    if (gcf->databases == NULL) {
+        return NGX_OK;
+    }
+
+    database = gcf->databases->elts;
+
+    for (i = 0; i < gcf->databases->nelts; i++) {
+        if (database[i].check_interval == 0) {
+            continue;
+        }
+
+        if ((database[i].last_check + database[i].check_interval)
+            > ngx_time())
+        {
+            continue;
+        }
+
+        database[i].last_check = ngx_time();
+
+        if (ngx_file_info(database[i].mmdb.filename, &fi) == NGX_FILE_ERROR) {
+            ngx_log_error(NGX_LOG_EMERG, r->connection->log, ngx_errno,
+                          ngx_file_info_n " \"%s\" failed",
+                          database[i].mmdb.filename);
+
+            continue;
+        }
+
+        if (ngx_file_mtime(&fi) <= database[i].last_change) {
+            continue;
+        }
+
+        /* do the reload */
+
+        ngx_memzero(&tmpdb, sizeof(MMDB_s));
+        status = MMDB_open(database[i].mmdb.filename, MMDB_MODE_MMAP, &tmpdb);
+
+        if (status != MMDB_SUCCESS) {
+            ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
+                          "MMDB_open(\"%s\") failed to reload - %s",
+                          database[i].mmdb.filename, MMDB_strerror(status));
+
+            continue;
+        }
+
+        database[i].last_change = ngx_file_mtime(&fi);
+        MMDB_close(&database[i].mmdb);
+        database[i].mmdb = tmpdb;
+
+        ngx_log_error(NGX_LOG_INFO, r->connection->log, 0,
+                      "Reload MMDB \"%s\"",
+                      database[i].mmdb.filename);
+    }
+
+    return NGX_OK;
+}
+
+
+static ngx_int_t
+ngx_http_geoip2_init(ngx_conf_t *cf)
+{
+    ngx_http_handler_pt        *h;
+    ngx_http_core_main_conf_t  *cmcf;
+
+    cmcf = ngx_http_conf_get_module_main_conf(cf, ngx_http_core_module);
+
+    h = ngx_array_push(&cmcf->phases[NGX_HTTP_LOG_PHASE].handlers);
+    if (h == NULL) {
+        return NGX_ERROR;
+    }
+
+    *h = ngx_http_geoip2_log_handler;
+
+    return NGX_OK;
+}

ngx_stream_geoip2_module.c

@@ -43,8 +43,6 @@ typedef struct {
 } ngx_stream_geoip2_metadata_t;
 
 
-static ngx_int_t ngx_stream_geoip2_reload(ngx_stream_geoip2_db_t *database,
-    ngx_log_t *log);
 static ngx_int_t ngx_stream_geoip2_variable(ngx_stream_session_t *s,
     ngx_stream_variable_value_t *v, uintptr_t data);
 static ngx_int_t ngx_stream_geoip2_metadata(ngx_stream_session_t *s,
@@ -63,6 +61,7 @@ static char *ngx_stream_geoip2_add_variable_geodata(ngx_conf_t *cf,
 static char *ngx_stream_geoip2_add_variable_metadata(ngx_conf_t *cf,
     ngx_stream_geoip2_db_t *database);
 static void ngx_stream_geoip2_cleanup(void *data);
+static ngx_int_t ngx_stream_geoip2_init(ngx_conf_t *cf);
 
 
 #define FORMAT(fmt, ...) do {                               \
@@ -89,7 +88,7 @@ static ngx_command_t  ngx_stream_geoip2_commands[] = {
 
 static ngx_stream_module_t  ngx_stream_geoip2_module_ctx = {
     NULL,                                  /* preconfiguration */
-    NULL,                                  /* preconfiguration */
+    ngx_stream_geoip2_init,                /* postconfiguration */
 
     ngx_stream_geoip2_create_conf,         /* create main configuration */
     NULL,                                  /* init main configuration */
@@ -115,41 +114,6 @@ ngx_module_t  ngx_stream_geoip2_module = {
 };
 
 
-static ngx_int_t
-ngx_stream_geoip2_reload(ngx_stream_geoip2_db_t *database, ngx_log_t *log)
-{
-    struct stat  attr;
-    MMDB_s       tmpdb;
-    int          status;
-
-    if (database->check_interval > 0
-            && database->last_check + database->check_interval <= ngx_time()) {
-        database->last_check = ngx_time();
-        stat(database->mmdb.filename, &attr);
-
-        if (attr.st_mtime > database->last_change) {
-            status = MMDB_open(database->mmdb.filename, MMDB_MODE_MMAP, &tmpdb);
-
-            if (status != MMDB_SUCCESS) {
-                ngx_log_error(NGX_LOG_ERR, log, 0,
-                                "MMDB_open(\"%s\") failed to reload - %s",
-                                database->mmdb.filename, MMDB_strerror(status));
-                return NGX_ERROR;
-            }
-
-            database->last_change = attr.st_mtime;
-            MMDB_close(&database->mmdb);
-            database->mmdb = tmpdb;
-
-            ngx_log_error(NGX_LOG_INFO, log, 0, "Reload MMDB \"%s\"",
-                                tmpdb.filename);
-        }
-    }
-
-    return NGX_OK;
-}
-
-
 static ngx_int_t
 ngx_stream_geoip2_variable(ngx_stream_session_t *s, ngx_stream_variable_value_t *v,
     uintptr_t data)
@@ -168,8 +132,6 @@ ngx_stream_geoip2_variable(ngx_stream_session_t *s, ngx_stream_variable_value_t
     unsigned long address;
 #endif
 
-    ngx_stream_geoip2_reload(database, s->connection->log);
-
     if (geoip2->source.value.len > 0) {
          if (ngx_stream_complex_value(s, &geoip2->source, &val) != NGX_OK) {
              goto not_found;
@@ -309,8 +271,6 @@ ngx_stream_geoip2_metadata(ngx_stream_session_t *s, ngx_stream_variable_value_t
     ngx_stream_geoip2_db_t        *database = metadata->database;
     u_char                        *p;
 
-    ngx_stream_geoip2_reload(database, s->connection->log);
-
     if (ngx_strncmp(metadata->metavalue.data, "build_epoch", 11) == 0) {
         FORMAT("%uL", database->mmdb.metadata.build_epoch);
     } else if (ngx_strncmp(metadata->metavalue.data, "last_check", 10) == 0) {
@@ -636,3 +596,94 @@ ngx_stream_geoip2_cleanup(void *data)
         ngx_array_destroy(gcf->databases);
     }
 }
+
+
+static ngx_int_t
+ngx_stream_geoip2_log_handler(ngx_stream_session_t *s)
+{
+    int                        status;
+    MMDB_s                     tmpdb;
+    ngx_uint_t                 i;
+    ngx_file_info_t            fi;
+    ngx_stream_geoip2_db_t    *database;
+    ngx_stream_geoip2_conf_t  *gcf;
+
+    ngx_log_debug0(NGX_LOG_DEBUG_STREAM, s->connection->log, 0,
+                   "geoip2 stream log handler");
+
+    gcf = ngx_stream_get_module_main_conf(s, ngx_stream_geoip2_module);
+
+    if (gcf->databases == NULL) {
+        return NGX_OK;
+    }
+
+    database = gcf->databases->elts;
+
+    for (i = 0; i < gcf->databases->nelts; i++) {
+        if (database[i].check_interval == 0) {
+            continue;
+        }
+
+        if ((database[i].last_check + database[i].check_interval)
+            > ngx_time())
+        {
+            continue;
+        }
+
+        database[i].last_check = ngx_time();
+
+        if (ngx_file_info(database[i].mmdb.filename, &fi) == NGX_FILE_ERROR) {
+            ngx_log_error(NGX_LOG_EMERG, s->connection->log, ngx_errno,
+                          ngx_file_info_n " \"%s\" failed",
+                          database[i].mmdb.filename);
+
+            continue;
+        }
+
+        if (ngx_file_mtime(&fi) <= database[i].last_change) {
+            continue;
+        }
+
+        /* do the reload */
+
+        ngx_memzero(&tmpdb, sizeof(MMDB_s));
+        status = MMDB_open(database[i].mmdb.filename, MMDB_MODE_MMAP, &tmpdb);
+
+        if (status != MMDB_SUCCESS) {
+            ngx_log_error(NGX_LOG_ERR, s->connection->log, 0,
+                          "MMDB_open(\"%s\") failed to reload - %s",
+                          database[i].mmdb.filename, MMDB_strerror(status));
+
+            continue;
+        }
+
+        database[i].last_change = ngx_file_mtime(&fi);
+        MMDB_close(&database[i].mmdb);
+        database[i].mmdb = tmpdb;
+
+        ngx_log_error(NGX_LOG_INFO, s->connection->log, 0,
+                      "Reload MMDB \"%s\"",
+                      database[i].mmdb.filename);
+    }
+
+    return NGX_OK;
+}
+
+
+static ngx_int_t
+ngx_stream_geoip2_init(ngx_conf_t *cf)
+{
+    ngx_stream_handler_pt        *h;
+    ngx_stream_core_main_conf_t  *cmcf;
+
+    cmcf = ngx_stream_conf_get_module_main_conf(cf, ngx_stream_core_module);
+
+    h = ngx_array_push(&cmcf->phases[NGX_STREAM_LOG_PHASE].handlers);
+    if (h == NULL) {
+        return NGX_ERROR;
+    }
+
+    *h = ngx_stream_geoip2_log_handler;
+
+    return NGX_OK;
+}