[PATCH] Fixing an obvious segfault in ngx_http_upstream

[Maxim Dounin]

Hello!

On Fri, May 14, 2010 at 01:31:23PM +0400, Igor Sysoev wrote:

> On Wed, May 05, 2010 at 01:18:53PM +0400, Maxim Dounin wrote:
> 
> > Hello!
> > 
> > On Wed, May 05, 2010 at 11:38:41AM +0400, Igor Sysoev wrote:
> > 
> > > On Tue, May 04, 2010 at 10:38:20AM +0800, agentzh wrote:
> > > 
> > > > On Tue, May 4, 2010 at 3:11 AM, Igor Sysoev <igor at sysoev.ru> wrote:
> > > > >
> > > > > You should test u->cleanup before *u->cleanup = NULL.
> > > > > This code has appeared in 0.8.33:
> > > > >
> > > > 
> > > > Hi, Igor,
> > > > 
> > > > It is *YOU* who didn't test u->cleanup before *u->cleanup in
> > > > ngx_http_upstream_create ;)
> > > > 
> > > > Please read my patch more carefully. To emphasize, in
> > > > ngx_http_upstream_create, the ngx_http_upstream_cleanup call first
> > > > clears u->cleanup but you later set *u->cleanup to NULL, which leads
> > > > to segfault.
> > > > 
> > > > There's no code written by myself, all in your nginx core ;)
> > > > 
> > > > I don't see how it is relevant to your fastcgi fixes in 0.8.33. This
> > > > bug appeared at least in nginx 0.8.29 :)
> > > 
> > > Yes, thank you, this is my fault.
> > > Strangely, I did not see segfaults on my production servers.
> > 
> > I believe this codepath can't be triggered in official nginx.
> 
> It may trigger if you use something like this:
> 
>     location / {
>         proxy/fastcgi/memcached_pass
>         error_page  404 502 503 = @fallback;
>     }
> 
>     location @fallback {
>         proxy/fastcgi/memcached_pass
>     }

How?  Every code path in upstream module I could see (including 
early errors and ngx_http_upstream_intercept_errors()) calls 
ngx_http_upstream_finalize_request() which will clear u->cleanup.

The only idea I have is something like error 400 due to client closed 
connection after POST with proxy_ignore_client_abort on; directed 
to proxied location, but this is due to bug in 
proxy_ignore_client_abort implementation.

> > Additionally, it looks like r->main->count++; there will result in 
> > socket leak (if it will be triggered).
> 
> Where ?

Never mind, I was wrong here.  Problem may only happen if 
ngx_http_finalize_request() wasn't called at all before/after 
creating new upstream request (and this will be actual bug).

Maxim Dounin