[PATCH] Limit req: r/h and r/d support

[Maxim Dounin]

Hello!

On Fri, Feb 16, 2018 at 03:57:05PM +0100, Bernhard Reutner-Fischer wrote:

> Attached is a patch to add r/h and r/d limit_req support.
> 
> There are at least two feature requests that i found asking for this:
> - https://trac.nginx.org/nginx/ticket/68
> - https://trac.nginx.org/nginx/ticket/1060
> 
> Ok?
> 
> Please consider applying.

Do you use it yourself, or your patch is based on the feature 
requests in question?  If yes, what is your use case?

Some obvious problems with the patch outlined below.

> # HG changeset patch
> # User Bernhard Reutner-Fischer <rep.dot.nop at gmail.com>
> # Date 1518790589 -3600
> #      Fri Feb 16 15:16:29 2018 +0100
> # Node ID 4f5981016b6d3b4b9745f90ddf9521fa7a0d375f
> # Parent  a49af443656f2b65ca5de9d8cad5594f44e18ff7
> Limit req: r/h and r/d support

Style: please add a trailing dot.

> 
> diff -r a49af443656f -r 4f5981016b6d src/http/modules/ngx_http_limit_req_module.c
> --- a/src/http/modules/ngx_http_limit_req_module.c	Thu Feb 08 12:11:30 2018 +0300
> +++ b/src/http/modules/ngx_http_limit_req_module.c	Fri Feb 16 15:16:29 2018 +0100
> @@ -16,7 +16,7 @@
>      u_short                      len;
>      ngx_queue_t                  queue;
>      ngx_msec_t                   last;
> -    /* integer value, 1 corresponds to 0.001 r/s */
> +    /* integer value, 1 corresponds to 0.00001 r/s */
>      ngx_uint_t                   excess;
>      ngx_uint_t                   count;
>      u_char                       data[1];
> @@ -33,7 +33,7 @@
>  typedef struct {
>      ngx_http_limit_req_shctx_t  *sh;
>      ngx_slab_pool_t             *shpool;
> -    /* integer value, 1 corresponds to 0.001 r/s */
> +    /* integer value, 1 corresponds to 0.00001 r/s */
>      ngx_uint_t                   rate;

On 32-bit platforms this means that the maximum supported rate 
would be about ~40kr/s.  And I suspect this can hurt real setups.

[...]

> @@ -213,7 +213,7 @@
>  
>          ngx_log_debug4(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>                         "limit_req[%ui]: %i %ui.%03ui",
> -                       n, rc, excess / 1000, excess % 1000);
> +                       n, rc, excess / 100000, excess % 100000);

The "%ui.%03ui" format needs to be adjusted as well, here and in 
all other places.

[...]

> @@ -398,8 +398,9 @@
>              ngx_queue_insert_head(&ctx->sh->queue, &lr->queue);
>  
>              ms = (ngx_msec_int_t) (now - lr->last);
> +            ms = ngx_abs(ms);
>  
> -            excess = lr->excess - ctx->rate * ngx_abs(ms) / 1000 + 1000;
> +            excess = lr->excess - ctx->rate * ms * 100 / 100000 + 100000;

Just keeping "/ 1000" might be a better option, given it 
corresponds to milliseconds in the "ms" variable.

[...]

> @@ -527,7 +529,7 @@
>              continue;
>          }
>  
> -        delay = excess * 1000 / ctx->rate;
> +        delay = excess * 100000 / ctx->rate;
>  
>          if (delay > max_delay) {
>              max_delay = delay;
> @@ -535,7 +537,7 @@
>              *limit = &limits[n];
>          }
>      }
> -
> +    max_delay /= 100;
>      return max_delay;

It is not clear why calculate delay in different units instead of 
milliseconds as specified by the type used, and then convert it 
back to milliseconds.

[...]

> @@ -825,7 +835,11 @@
>          return NGX_CONF_ERROR;
>      }
>  
> -    ctx->rate = rate * 1000 / scale;
> +    ctx->rate = rate * 100000 / scale;
> +    if (ctx->rate < 1) {
> +        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "invalid rate \"%ui\"", rate);
> +        return NGX_CONF_ERROR;
> +    }

The ctx->rate is unsigned, and the ctx->rate check will only fail 
if ctx->rate is 0.  While this might happen in some cases, it 
certainly won't catch all overflows (which are quite likely in 
real configurations on 32-bit platforms, see above).

Also, the "rate" type is ngx_int_t, so (rate * 100000) will cause 
integer overflow, which is not defined.  It might be a better idea 
to avoid overflows at all.

Also, the "rate" type is ngx_int_t, not ngx_uint_t, so proper 
format would be "%i". 

In general, it might be a better idea to move the check to 
ngx_atoi(), and test something like NGX_MAX_INT_T_VALUE / 100000.  
This will ensure no overflow can happen during calculations below, 
and will allow logging of the original value.  This will further 
limit maximum rate to about 20kr/s on 32-bit platforms though.

Note that I have no good solution for 32-bit platforms and the 
40kr/s (20kr/s) limit.

-- 
Maxim Dounin
http://mdounin.ru/