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/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
