Closing upstream keepalive connections in an invalid state

[Maxim Dounin]

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;
 };
 

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