[PATCH] Fixing a segmentation fault when resolver and poll are used together

[Maxim Dounin]

Hello!

On Wed, Jan 23, 2013 at 12:13:41PM +0400, Ruslan Ermilov wrote:

> On Tue, Jan 22, 2013 at 02:56:34PM -0800, agentzh wrote:
> > I've noticed a segmentation fault caught by Valgrind/Memcheck when
> > using ngx_resolver and ngx_poll_module together with Nginx 1.2.6 on
> > Linux x86_64 (and i386 too):
> > 
> >     ==25191== Jump to the invalid address stated on the next line
> >     ==25191==    at 0x0: ???
> >     ==25191==    by 0x42CC67: ngx_event_process_posted (ngx_event_posted.c:40)
> >     ==25191==    by 0x42C7E7: ngx_process_events_and_timers (ngx_event.c:274)
> >     ==25191==    by 0x434BCB: ngx_single_process_cycle (ngx_process_cycle.c:315)
> >     ==25191==    by 0x416981: main (nginx.c:409)
> >     ==25191==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> >     ==25191==
> > 
> > It crashes on the following source line in src/event/ngx_event_posted.c:
> > 
> >     ev->handler(ev);
> > 
> > The minimized config sample is as follows:
> > 
> >     events {
> >         use poll;
> >     }
> > 
> >     http {
> >         server {
> >             listen 8080;
> > 
> >             location /t {
> >                 set $myserver nginx.org;
> >                 proxy_pass http://$myserver/;
> >                 resolver 127.0.0.1;
> >             }
> >         }
> >     }
> > 
> > assuming that there's nothing listening on the local port 53.
> > 
> > Basically, when POLLERR happens ngx_poll_module will generate both a
> > write event and a read event for the current connection but there is
> > no write event handler registered for the UDP connection created in
> > ngx_resolver.c, thus leading to calling a NULL function pointer.
> > 
> > The simplest fix (though maybe not the most reasonable) is to register
> > an empty write event handler in ngx_resolver.c, as shown in the patch
> > attached to this mail (already tested on my side).
> > 
> > Best regards,
> > -agentzh
> 
> First of all, thanks for catching the bug!
> 
> I propose one of these patches instead:
> 
> %%%
> Index: src/event/modules/ngx_poll_module.c
> ===================================================================
> --- src/event/modules/ngx_poll_module.c	(revision 5013)
> +++ src/event/modules/ngx_poll_module.c	(working copy)
> @@ -366,7 +366,13 @@ ngx_poll_process_events(ngx_cycle_t *cycle, ngx_ms
>               * active handler
>               */
>  
> -            revents |= POLLIN|POLLOUT;
> +            if (c->read->active) {
> +                revents |= POLLIN;
> +            }
> +
> +            if (c->write->active) {
> +                revents |= POLLOUT;
> +            }
>          }
>  
>          found = 0;
> %%%
> 
> %%%
> Index: src/event/modules/ngx_poll_module.c
> ===================================================================
> --- src/event/modules/ngx_poll_module.c	(revision 5013)
> +++ src/event/modules/ngx_poll_module.c	(working copy)
> @@ -371,7 +371,7 @@ ngx_poll_process_events(ngx_cycle_t *cycle, ngx_ms
>  
>          found = 0;
>  
> -        if (revents & POLLIN) {
> +        if ((revents & POLLIN) && c->read->active) {
>              found = 1;
>  
>              ev = c->read;
> @@ -388,7 +388,7 @@ ngx_poll_process_events(ngx_cycle_t *cycle, ngx_ms
>              ngx_locked_post_event(ev, queue);
>          }
>  
> -        if (revents & POLLOUT) {
> +        if ((revents & POLLOUT) && c->write->active) {
>              found = 1;
>              ev = c->write;
>  
> %%%
> 
> While the first patch looks more natural to me, the second patch is
> in line with the ngx_epoll_process_events() code.

Second one also consistent with rtsig code (and might also catch 
other strange cases of unexpected events), I vote for it.  Please 
go ahead and commit it.

-- 
Maxim Dounin
http://nginx.com/support.html