Closing upstream keepalive connections in an invalid state

[Maxim Dounin]

Hello!

On Mon, Nov 30, 2015 at 09:09:46PM +0300, Maxim Dounin wrote:

> Hello!
> 
> On Mon, Nov 30, 2015 at 03:20:17PM +0000, Chris Branch wrote:
> 
> > There was a thread on the nginx mailing list last week, 
> > regarding upstream keepalive connections being placed in an 
> > invalid state due to a partially-transmitted request body. With 
> > regard to that discussion, I’m submitting two patches for your 
> > review.
> > 
> > The first adds a test case to nginx-tests demonstrating the 
> > problem as of nginx 1.9.7. Most of the change involves extending 
> > the mock origin to consume a request body, and verify the method 
> > transmitted. Currently, nginx will reuse the upstream connection 
> > for a subsequent request and (from the point of view of an 
> > upstream client) insert some or all of a request line and 
> > headers into the previous request's body. The result is 
> > typically a 400 Bad Request error due to a malformed request.
> 
> A test case for this was already committed by Sergey Kandaurov a 
> couple of days ago:
> 
> http://hg.nginx.org/nginx-tests/rev/2f292082c8a0
> 
> > The second patch fixes this bug using the method suggested by 
> > Maxim, i.e. close the upstream connection when a response is 
> > received before the request body is completely sent. This is the 
> > behaviour suggested in RFC 2616 section 8.2.2. The relevant Trac 
> > issue is #669.
> 
> The patch looks incomplete for me.  It doesn't seem to handle the 
> "next upstream" case.  And the condition used looks wrong, too, as 
> it doesn't take into account what nginx actually tried to send.
> 
> (And please also take a look at this article: 
> http://nginx.org/en/docs/contributing_changes.html)
> 
> Quick and dirty patch to address this is as follows.  Though I 
> can't say I like it.
> 
> diff --git a/src/http/modules/ngx_http_upstream_keepalive_module.c b/src/http/modules/ngx_http_upstream_keepalive_module.c
> --- a/src/http/modules/ngx_http_upstream_keepalive_module.c
> +++ b/src/http/modules/ngx_http_upstream_keepalive_module.c
> @@ -302,6 +302,10 @@ ngx_http_upstream_free_keepalive_peer(ng
>          goto invalid;
>      }
>  
> +    if (!u->request_body_sent) {
> +        goto invalid;
> +    }
> +
>      if (ngx_terminate || ngx_exiting) {
>          goto invalid;
>      }
> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c
> +++ b/src/http/ngx_http_upstream.c
> @@ -1818,6 +1818,8 @@ ngx_http_upstream_send_request(ngx_http_
>  
>      /* rc == NGX_OK */
>  
> +    u->request_body_sent = 1;
> +
>      if (c->write->timer_set) {
>          ngx_del_timer(c->write);
>      }
> diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h
> --- a/src/http/ngx_http_upstream.h
> +++ b/src/http/ngx_http_upstream.h
> @@ -370,6 +370,7 @@ struct ngx_http_upstream_s {
>      unsigned                         upgrade:1;
>  
>      unsigned                         request_sent:1;
> +    unsigned                         request_body_sent:1;
>      unsigned                         header_sent:1;
>  };
>  

And an additional part to properly reset the flag on next 
upstream:

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -1434,6 +1434,7 @@ ngx_http_upstream_connect(ngx_http_reque
     }
 
     u->request_sent = 0;
+    u->request_body_sent = 0;
 
     if (rc == NGX_AGAIN) {
         ngx_add_timer(c->write, u->conf->connect_timeout);


-- 
Maxim Dounin
http://nginx.org/