Hello!

On Thu, Jun 07, 2012 at 05:36:36PM +0800, agentzh wrote:

> Hello!
>
> I observed consistent segmentation faults when the Nginx worker
> process exits due to HUP signal reload. The Nginx server has both
> resolver and ngx_poll_module configured. The backtrace looks like
> this:
>
> Process terminating with default action of signal 11 (SIGSEGV)
> Access not within mapped region at address 0x50
> at 0x437AF8: ngx_poll_del_event (ngx_poll_module.c:208)
> by 0x4220C0: ngx_close_connection (ngx_connection.c:872)
> by 0x4267D1: ngx_resolver_cleanup (ngx_resolver.c:223)
> by 0x4176D0: ngx_destroy_pool (ngx_palloc.c:55)
> by 0x43317C: ngx_worker_process_exit (ngx_process_cycle.c:1059)
> by 0x43327D: ngx_worker_process_cycle (ngx_process_cycle.c:800)
> by 0x431AAE: ngx_spawn_process (ngx_process.c:198)
> by 0x4328C9: ngx_start_worker_processes (ngx_process_cycle.c:365)
> by 0x4342ED: ngx_master_process_cycle (ngx_process_cycle.c:250)
> by 0x416653: main (nginx.c:410)
>
> At this point of failure, ngx_cycle->files was a NULL pointer (due to
> the assignment of ngx_exit_cycle to ngx_cycle in
> ngx_worker_process_exit) but ngx_poll_del_event tragically accessed
> this array by an index, which was an invalid read.
>
> Below attaches a patch for nginx 1.0.15 that fixes this segfault. It
> can also be cleanly applied to at least nginx 1.2.1.
>
> Comments will be highly appreciated as usual :)
>
> Thanks!
> -agentzh
>
> --- nginx-1.0.15/src/event/modules/ngx_poll_module.c 2012-02-06
> 04:02:59.000000000 +0800
> +++ nginx-1.0.15-patched/src/event/modules/ngx_poll_module.c 2012-06-07
> 17:22:43.538168219 +0800
> @@ -205,19 +205,21 @@
>
> event_list[ev->index] = event_list[nevents];
>
> - c = ngx_cycle->files[event_list[nevents].fd];
> + if (ngx_cycle->files) {
> + c = ngx_cycle->files[event_list[nevents].fd];

[...]

While the patch obviously fixes the segfault, I don't really like
it as it leaves incorrect event index set for a copied event.
It's unlikely this will cause any problems on exit, but it's still
incorrect.

Something like this should be better way to solve the problem:

diff --git a/src/os/unix/ngx_process_cycle.c b/src/os/unix/ngx_process_cycle.c
--- a/src/os/unix/ngx_process_cycle.c
+++ b/src/os/unix/ngx_process_cycle.c
@@ -711,6 +711,8 @@ ngx_master_process_exit(ngx_cycle_t *cyc
ngx_exit_log.file = &ngx_exit_log_file;

ngx_exit_cycle.log = &ngx_exit_log;
+ ngx_exit_cycle.files = ngx_cycle->files;
+ ngx_exit_cycle.files_n = ngx_cycle->files_n;
ngx_cycle = &ngx_exit_cycle;

ngx_destroy_pool(cycle->pool);
@@ -1054,6 +1056,8 @@ ngx_worker_process_exit(ngx_cycle_t *cyc
ngx_exit_log.file = &ngx_exit_log_file;

ngx_exit_cycle.log = &ngx_exit_log;
+ ngx_exit_cycle.files = ngx_cycle->files;
+ ngx_exit_cycle.files_n = ngx_cycle->files_n;
ngx_cycle = &ngx_exit_cycle;

ngx_destroy_pool(cycle->pool);


Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel