From 26399de025de1fd33c3b2fce0522438db33bc12d Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Wed, 10 Oct 2018 13:00:25 +0300 Subject: [PATCH 1/3] 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 | 135 +++++++++++++++++++++++++------------ ngx_stream_geoip2_module.c | 135 +++++++++++++++++++++++++------------ 2 files changed, 186 insertions(+), 84 deletions(-) diff --git a/ngx_http_geoip2_module.c b/ngx_http_geoip2_module.c index e3e3cb7..0d76e34 100644 --- a/ngx_http_geoip2_module.c +++ b/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; +} diff --git a/ngx_stream_geoip2_module.c b/ngx_stream_geoip2_module.c index f988e6a..dfd94ad 100644 --- a/ngx_stream_geoip2_module.c +++ b/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; +} From 80659b1bee6f54d3063478a094208db5cc5d9ca9 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Thu, 11 Oct 2018 16:59:37 +0300 Subject: [PATCH 2/3] Store string variables from libmaxmind in request pool Using direct references may cause SIGSEGV on database reload (when auto_reload parameter is enabled). --- ngx_http_geoip2_module.c | 12 ++++++++++-- ngx_stream_geoip2_module.c | 12 ++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/ngx_http_geoip2_module.c b/ngx_http_geoip2_module.c index 0d76e34..d94bd42 100644 --- a/ngx_http_geoip2_module.c +++ b/ngx_http_geoip2_module.c @@ -226,12 +226,20 @@ ngx_http_geoip2_variable(ngx_http_request_t *r, ngx_http_variable_value_t *v, FORMAT("%d", entry_data.boolean); break; case MMDB_DATA_TYPE_UTF8_STRING: - v->data = (u_char *) entry_data.utf8_string; v->len = entry_data.data_size; + v->data = ngx_pnalloc(r->pool, v->len); + if (v->data == NULL) { + return NGX_ERROR; + } + ngx_memcpy(v->data, (u_char *) entry_data.utf8_string, v->len); break; case MMDB_DATA_TYPE_BYTES: - v->data = (u_char *) entry_data.bytes; v->len = entry_data.data_size; + v->data = ngx_pnalloc(r->pool, v->len); + if (v->data == NULL) { + return NGX_ERROR; + } + ngx_memcpy(v->data, (u_char *) entry_data.bytes, v->len); break; case MMDB_DATA_TYPE_FLOAT: FORMAT("%.5f", entry_data.float_value); diff --git a/ngx_stream_geoip2_module.c b/ngx_stream_geoip2_module.c index dfd94ad..ad97ac6 100644 --- a/ngx_stream_geoip2_module.c +++ b/ngx_stream_geoip2_module.c @@ -195,12 +195,20 @@ ngx_stream_geoip2_variable(ngx_stream_session_t *s, ngx_stream_variable_value_t FORMAT("%d", entry_data.boolean); break; case MMDB_DATA_TYPE_UTF8_STRING: - v->data = (u_char *) entry_data.utf8_string; v->len = entry_data.data_size; + v->data = ngx_pnalloc(r->pool, v->len); + if (v->data == NULL) { + return NGX_ERROR; + } + ngx_memcpy(v->data, (u_char *) entry_data.utf8_string, v->len); break; case MMDB_DATA_TYPE_BYTES: - v->data = (u_char *) entry_data.bytes; v->len = entry_data.data_size; + v->data = ngx_pnalloc(r->pool, v->len); + if (v->data == NULL) { + return NGX_ERROR; + } + ngx_memcpy(v->data, (u_char *) entry_data.bytes, v->len); break; case MMDB_DATA_TYPE_FLOAT: FORMAT("%.5f", entry_data.float_value); From 9db8253eec68eb65de853424b2d4c9efdb5ca2d2 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Thu, 11 Oct 2018 19:22:52 +0300 Subject: [PATCH 3/3] Fixed build of stream module --- ngx_stream_geoip2_module.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ngx_stream_geoip2_module.c b/ngx_stream_geoip2_module.c index ad97ac6..650dcfa 100644 --- a/ngx_stream_geoip2_module.c +++ b/ngx_stream_geoip2_module.c @@ -196,7 +196,7 @@ ngx_stream_geoip2_variable(ngx_stream_session_t *s, ngx_stream_variable_value_t break; case MMDB_DATA_TYPE_UTF8_STRING: v->len = entry_data.data_size; - v->data = ngx_pnalloc(r->pool, v->len); + v->data = ngx_pnalloc(s->connection->pool, v->len); if (v->data == NULL) { return NGX_ERROR; } @@ -204,7 +204,7 @@ ngx_stream_geoip2_variable(ngx_stream_session_t *s, ngx_stream_variable_value_t break; case MMDB_DATA_TYPE_BYTES: v->len = entry_data.data_size; - v->data = ngx_pnalloc(r->pool, v->len); + v->data = ngx_pnalloc(s->connection->pool, v->len); if (v->data == NULL) { return NGX_ERROR; }