Extra data from upstream and keepalive connections

[Awdhesh Mathpal]

Yes, Content-Length:0 is not the only case. The updated change set looks good to me.

> But this one shouldn't be needed, as this part cannot be reached 
> with u->keepalive set.  If you think it can, please elaborate.

Yes, that part cannot happen currently, as upstream->keepalive is not yet initialized. I added it to be explicit about the intent and to avoid any case, where the condition of upstream->keepalive not being set becomes false in future. 


Awdhesh


----- Original message -----
From: Maxim Dounin <mdounin at mdounin.ru>
To: nginx-devel at nginx.org
Subject: Re: Extra data from upstream and keepalive connections
Date: Monday, October 11, 2021 11:49

Hello!

On Fri, Oct 08, 2021 at 11:19:29AM -0700, Awdhesh Mathpal wrote:

> Hello,
> 
> Proxy module may not disable keepalive connection when upstream sends extra data with Content-Length:0 response header.
> 
> This happens because of an incorrect assumption on the state of the upstream->keepalive flag at https://github.com/nginx/nginx/blame/master/src/http/modules/ngx_http_proxy_module.c#L2336
> 
> When response content-length is 0,  then upstream->keepalive may get initialized to 1 depending on the Connection response header. https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_proxy_module.c#L2002
> 
> To trigger this issue, nginx must be configured as follows:
> -    proxy buffering is disabled
> -    responses are processed by ngx_http_proxy_non_buffered_copy_filter (no nginx caching)
> -    The upstream keepalive directive is enabled
> -    The content-length response header from upstream is 0
> -    Upstream sends a body/extra data
> 
> Under these conditions, the connection will be saved for next request.
> 
> Here is a patch that addresses this:

Thanks for the patch, see below for comments.

> 
> # HG changeset patch
> # User Awdhesh Mathpal <mathpal at amazon.com>
> # Date 1633659791 25200
> #      Thu Oct 07 19:23:11 2021 -0700
> # Node ID ccf2ccd9724f7cff4363e81545b1af97aa881415
> # Parent  ae7c767aa491fa55d3168dfc028a22f43ac8cf89
> proxy: Disable keepalive on extra data
> 
> When an upstream sends Content-Length:0, upstream->keepalive
> is initialized eagerly on the basis of Connection header. This
> can lead to keepalive being enabled on the connection. If in such
> a scenario upstream sends extra data, then the connection should
> not be reused.

The "Content-Length: 0" case is not the only possible scenario 
when this may happen.  A more accurate would be to say "a response 
without body".  It might also make sense to refer to the similar 
code in ngx_http_proxy_copy_filter(), as well as the 83c4622053b0 
(http://hg.nginx.org/nginx/rev/83c4622053b0) where this was 
missed.

Suggested commit log, also fixing minor style issues:

: Proxy: disabled keepalive on extra data in non-buffered mode.
: 
: The u->keepalive flag is initialized early if the response has no body
: (or an empty body), and needs to be reset if there are any extra data,
: similarly to how it is done in ngx_http_proxy_copy_filter().  Missed
: in 83c4622053b0.

> 
> diff -r ae7c767aa491 -r ccf2ccd9724f src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c  Wed Oct 06 18:01:42 2021 +0300
> +++ b/src/http/modules/ngx_http_proxy_module.c  Thu Oct 07 19:23:11 2021 -0700
> @@ -2337,6 +2337,7 @@
>          ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
>                        "upstream sent more data than specified in "
>                        "\"Content-Length\" header");
> +        u->keepalive = 0;
>          return NGX_OK;
>      }
> 

This part looks fine.

> @@ -2370,7 +2371,7 @@
>          ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
>                        "upstream sent more data than specified in "
>                        "\"Content-Length\" header");
> -
> +        u->keepalive = 0;
>          cl->buf->last = cl->buf->pos + u->length;
>          u->length = 0;
> 
> 

But this one shouldn't be needed, as this part cannot be reached 
with u->keepalive set.  If you think it can, please elaborate.

Updated patch with the above commit-log changes and unneeded part 
removed:

# HG changeset patch
# User Awdhesh Mathpal <mathpal at amazon.com>
# Date 1633659791 25200
#      Thu Oct 07 19:23:11 2021 -0700
# Node ID 055b2a8471171dfa16a5696524d6f740b213e660
# Parent  ae7c767aa491fa55d3168dfc028a22f43ac8cf89
Proxy: disabled keepalive on extra data in non-buffered mode.

The u->keepalive flag is initialized early if the response has no body
(or an empty body), and needs to be reset if there are any extra data,
similarly to how it is done in ngx_http_proxy_copy_filter().  Missed
in 83c4622053b0.

diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c
+++ b/src/http/modules/ngx_http_proxy_module.c
@@ -2337,6 +2337,7 @@ ngx_http_proxy_non_buffered_copy_filter(
         ngx_log_error(NGX_LOG_WARN, r->connection->log, 0,
                       "upstream sent more data than specified in "
                       "\"Content-Length\" header");
+        u->keepalive = 0;
         return NGX_OK;
     }
 

Please take a look.

-- 
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel at nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel


-- 
Awdhesh Mathpal