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

[Ruslan Ermilov]

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.