Re: Segmentation fault в DAV модуле

[Maxim Dounin]

Hello!

On Tue, Nov 20, 2012 at 05:29:58PM +0300, Dmitry Petrov wrote:

> Hello.
> 
> Finally I was able to make a test that reproduces bug 100% of time.
> Here it is (nginx sources included): http://rghost.net/41694908
> You need to:
> 1. tar zxf nginx-webdav-segfault.tar.gz
> 2. cd dav-bug/nginx-1.3.8
> 3. ./cf — this will configure nginx with needed modules and /tmp as prefix
> 4. cd ..
> 5. In one console start nginx via 'make r' command. It will automatically
> use dav.conf which will start nginx on 0.0.0.0:8080
> 6. Attach to nginx process: gdb --pid `pgrep -n nginx`
> 7. Execute make — this will launch test trying to connect to
> localhost:8080, upload some files etc. Make sure that 8181 port is free
> before running test case - it create dummy "HTTP server" that simply
> ignores all requests. I wasn't able to get this bug without proxy_pass'es
> between webdav PUT's.
> 8. Monitor console where you've started gdb - you will get SIGSEGV here
> The symptoms of the crash are the same as I described earlier:
> r->request_body is not NULL, set from some kind of previous request.
> r->request_body->temp_file is NULL.
> 
> This test case just replaying dump recorded from Iphone HTTP session using
> TCP_CORK to achive exactly the same packets send (with scrambled payload
> ofc).
> 
> Hope this helps.

Thanks, I was able to reproduce the problem here using you test 
code.  It indeed looks like a problem related to extra write 
events, much like one recently reported here:

http://serverfault.com/questions/449195/nginx-webdav-server-with-auth-request

Please try the following patch:

--- a/src/http/ngx_http_request_body.c
+++ b/src/http/ngx_http_request_body.c
@@ -174,6 +174,7 @@ ngx_http_read_client_request_body(ngx_ht
             rb->to_write = rb->bufs;
 
             r->read_event_handler = ngx_http_read_client_request_body_handler;
+            r->write_event_handler = ngx_http_request_empty_handler;
 
             return ngx_http_do_read_client_request_body(r);
         }
@@ -234,6 +235,7 @@ ngx_http_read_client_request_body(ngx_ht
     }
 
     r->read_event_handler = ngx_http_read_client_request_body_handler;
+    r->write_event_handler = ngx_http_request_empty_handler;
 
     return ngx_http_do_read_client_request_body(r);
 }

> On Tue, Nov 20, 2012 at 1:21 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> > Hello!
> >
> > On Tue, Nov 20, 2012 at 12:05:07PM +0300, Dmitry Petrov wrote:
> >
> > > On Mon, Nov 19, 2012 at 7:40 PM, Arnaud GRANAL <serphen at gmail.com>
> > wrote:
> > >
> > > > I guess it was fixed in a recent commit by Maxim:
> > > > [..]
> > > > diff --git a/src/http/modules/ngx_http_dav_module.c
> > > > b/src/http/modules/ngx_http_dav_module.c
> > > > --- a/src/http/modules/ngx_http_dav_module.c
> > > > +++ b/src/http/modules/ngx_http_dav_module.c
> > > > @@ -209,6 +209,11 @@ ngx_http_dav_put_handler(ngx_http_reques
> > > >      ngx_ext_rename_file_t     ext;
> > > >      ngx_http_dav_loc_conf_t  *dlcf;
> > > >
> > > > +    if (r->request_body == NULL || r->request_body->temp_file ==
> > NULL) {
> > > > +        ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
> > > > +        return;
> > > > +    }
> > > > +
> > > >      ngx_http_map_uri_to_path(r, &path, &root, 0);
> > > >
> > > >      path.len--;
> > > >
> > > The problem is that I got this behavior on sequential valid PUT requests.
> > > After handling first request, nginx even doesn't try to read body of next
> > > request. It just calls ngx_http_dav_put_handler with r->response_body
> > from
> >
> > It looks like this is a generic request body handling problem,
> > which leads to incorrect body processing if an extra write event
> > happens during request body reading.
> >
> > The patch quoted will fix resulting segfault in dav module (which
> > may also happen in other conditions), but the underlying problem
> > also needs to be fixed.  If you have a test case to reproduce the
> > problem it might be helpful.
> >
> > > previous request but with r->response_body->temp_file set to NULL. With
> > > extra check for temp_file not being NULL nginx actually reads second
> > > request body and handles it properly.
> >
> > The extra check you've added isn't correct as
> > r->response_body->temp_file may be NULL for valid reasons, e.g.
> > request body reading is done by a normal module like proxy, which
> > doesn't require request body to be in file.
> >
> > --
> > Maxim Dounin
> > http://nginx.com/support.html
> >
> > _______________________________________________
> > nginx-devel mailing list
> > nginx-devel at nginx.org
> > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> >
> 
> 
> 
> -- 
> Regards,
> Dmitry

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


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