[PATCH] Fixing segfaults in ngx_poll_del_event at worker exit

[Maxim Dounin]

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