[PATCH] nginx does not close the connection for 412 responses under extreme conditions

[Maxim Dounin]

Hello!

On Sat, May 12, 2012 at 09:12:46PM +0400, Maxim Dounin wrote:

[...]

> I'll take a closer look at the patch later to make sure it doesn't 
> break various image filter use cases (likely no, but as I already 
> said filter finalization is at best fragile) and to see if it's 
> also possible to avoid response truncation in case of error_page 
> used after filter finalization.

Production use of 1.3.0 revealed that it indeed breaks filter 
finalization in config like this

        location /image/ {
            error_page 415 = /zero;
            image_filter crop $arg_w $arg_h;
            proxy_pass http://127.0.0.1:8080/1m?;
            proxy_store /tmp/store/$uri;
        }

        location /zero {
            return 204;
        }

In case of 415 from image_filter request is finalized in rewrite 
module, and

1) due to filter_finalize check r->write_event_handler 
remains set to ngx_http_core_run_phases();

2) due to proxy_store NGX_ERROR is ignored by upstream, and 
reading from upstream continues.

Then on write event reported for a client 
ngx_http_core_run_phases() is triggered again, 
ngx_http_finalize_request() is called (incorrecly) causing another 
ngx_http_finalize_connection() due to filter_finalize set.  This 
eventually results in segfault once request is finalized from 
upstream module.

Michael Monashov, who reported the issue, currently testing the 
following patch (against 1.3.0):

--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -1933,8 +1933,6 @@ ngx_http_finalize_request(ngx_http_reque
 
     if (rc == NGX_OK && r->filter_finalize) {
         c->error = 1;
-        ngx_http_finalize_connection(r);
-        return;
     }
 
     if (rc == NGX_DECLINED) {

It should resolve the original problem with socket leak but let 
things handle correctly the above scenario as well.  Could you 
please test if it works for you?

Maxim Dounin