[PATCH] Limit req, Limit conn: The "off" parameter of the 'limit_conn' and 'limit_req' directives

[Maxim Dounin]

Hello!

On Sun, Feb 07, 2016 at 03:32:42PM +0600, Pavel V. Rochnyack wrote:

> # HG changeset patch
> # User Pavel V. Rochnyack <pavel2000 at ngs.ru>
> # Date 1454835814 -21600
> # Node ID f7caa68ea80358a035ab766bdb62676522ec529f
> # Parent  3577c021f21eb4de6d09c1d624ba77ee9ee1eb6d
> Limit req, Limit conn: The "off" parameter of the 'limit_conn' and 'limit_req' directives.
> 
> Added support for inherited "limit_conn" and "limit_req" directives disable.

Please re-read http://nginx.org/en/docs/contributing_changes.html.  
In particular:

: maximum text width is 80 characters
...
: The commit message should have a single-line synopsis followed 
: by verbose description after an empty line. It is desirable that 
: the first line is no longer than 67 symbols.

In this particular case I would also recommend to split the patch 
into two separate patches: one for limit req, and another one for 
limit conn.

> diff -r 3577c021f21e -r f7caa68ea803 src/http/modules/ngx_http_limit_conn_module.c
> --- a/src/http/modules/ngx_http_limit_conn_module.c	Fri Feb 05 21:48:25 2016 +0300
> +++ b/src/http/modules/ngx_http_limit_conn_module.c	Sun Feb 07 15:03:34 2016 +0600
> @@ -40,6 +40,7 @@

Please add

[diff]
showfunc=1

to your ~/.hgrc file, this generally simplifies review process.

>      ngx_array_t                limits;
>      ngx_uint_t                 log_level;
>      ngx_uint_t                 status_code;
> +    ngx_uint_t                 off;     /* unsigned  off:1 */
>  } ngx_http_limit_conn_conf_t;

Indentation of the "/* unsigned  off:1 */" looks wrong.

>  
>  
> @@ -82,7 +83,7 @@
>        NULL },
>  
>      { ngx_string("limit_conn"),
> -      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE12,
>        ngx_http_limit_conn,
>        NGX_HTTP_LOC_CONF_OFFSET,
>        0,
> @@ -160,6 +161,10 @@
>      lccf = ngx_http_get_module_loc_conf(r, ngx_http_limit_conn_module);
>      limits = lccf->limits.elts;
>  
> +    if (lccf->off) {
> +        return NGX_DECLINED;
> +    }
> +
>      for (i = 0; i < lccf->limits.nelts; i++) {
>          ctx = limits[i].shm_zone->data;
>  
> @@ -466,6 +471,7 @@
>  
>      conf->log_level = NGX_CONF_UNSET_UINT;
>      conf->status_code = NGX_CONF_UNSET_UINT;
> +    conf->off = NGX_CONF_UNSET_UINT;
>  
>      return conf;
>  }
> @@ -478,6 +484,7 @@
>      ngx_http_limit_conn_conf_t *conf = child;
>  
>      if (conf->limits.elts == NULL) {
> +        ngx_conf_merge_uint_value(conf->off, prev->off, 0);
>          conf->limits = prev->limits;
>      }
>  

See no reasons why limits from previous level should be meged if 
"off" is used on the current level.  It's just unneeded work, 
which also add complexity in other places (like a need to check 
off at runtime, and merge off values from previous levels).

Better approach would be to don't inherit limits if "off" is set 
on the current level.

> @@ -603,6 +610,13 @@
>  
>      value = cf->args->elts;
>  
> +    lccf->off = 0;
> +
> +    if (ngx_strcmp(value[1].data, "off") == 0 && cf->args->nelts == 2) {
> +        lccf->off = 1;
> +        return NGX_CONF_OK;
> +    }
> +
>      shm_zone = ngx_shared_memory_add(cf, &value[1], 0,
>                                       &ngx_http_limit_conn_module);
>      if (shm_zone == NULL) {

This variant of the code doesn't check "off" if additional 
arguments are used.  Limit req's version however does (and 
complains if number of arguments is incorrect).  Reasons for the 
difference are not clear.

With this code something like "limit_conn foo" will try to access 
value[2] unconditionally, potentially resulting in segmentation 
fault during configuration parsing.

This code silently allows "limit_conn off" to be used multiple 
times, as well as with other limit_conn directives.  This leads to 
a situation where:

   limit_conn off;
   limit_conn foo 10;

will not use "limit_conn foo 10" configured without any 
indication, which looks incorrect and confusing.  It should either 
complain, or use limits configured.

> diff -r 3577c021f21e -r f7caa68ea803 src/http/modules/ngx_http_limit_req_module.c
> --- a/src/http/modules/ngx_http_limit_req_module.c	Fri Feb 05 21:48:25 2016 +0300
> +++ b/src/http/modules/ngx_http_limit_req_module.c	Sun Feb 07 15:03:34 2016 +0600
> @@ -53,6 +53,7 @@
>      ngx_uint_t                   limit_log_level;
>      ngx_uint_t                   delay_log_level;
>      ngx_uint_t                   status_code;
> +    ngx_uint_t                   off;     /* unsigned  off:1 */
>  } ngx_http_limit_req_conf_t;
>  
>  
> @@ -172,6 +173,10 @@
>      lrcf = ngx_http_get_module_loc_conf(r, ngx_http_limit_req_module);
>      limits = lrcf->limits.elts;
>  
> +    if (lrcf->off) {
> +        return NGX_DECLINED;
> +    }
> +
>      excess = 0;
>  
>      rc = NGX_DECLINED;
> @@ -698,6 +703,7 @@
>  
>      conf->limit_log_level = NGX_CONF_UNSET_UINT;
>      conf->status_code = NGX_CONF_UNSET_UINT;
> +    conf->off = NGX_CONF_UNSET_UINT;
>  
>      return conf;
>  }
> @@ -710,6 +716,7 @@
>      ngx_http_limit_req_conf_t *conf = child;
>  
>      if (conf->limits.elts == NULL) {
> +        ngx_conf_merge_uint_value(conf->off, prev->off, 0);
>          conf->limits = prev->limits;
>      }
>  
> @@ -870,6 +877,19 @@
>  
>      value = cf->args->elts;
>  
> +    lrcf->off = 0;
> +
> +    if (ngx_strcmp(value[1].data, "off") == 0) {
> +        lrcf->off = 1;
> +        if (cf->args->nelts == 2) {
> +            return NGX_CONF_OK;
> +        }
> +
> +        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                           "invalid parameter \"%V\"", &value[2]);
> +        return NGX_CONF_ERROR;
> +    }
> +
>      shm_zone = NULL;
>      burst = 0;
>      nodelay = 0;

See comments for the limit_conn part of the patch, most of them 
apply here as well.

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