Hello - and missing verification of configuration format / very tiny mem leak in limit_req config

[Maxim Dounin]

Hello!

On Sat, Aug 23, 2014 at 07:03:32PM +0100, Martin Frb wrote:

> Hi,
> 
> My name is Martin. I am new to this list (obvious), and new to nginx as
> well.
> I hope this is the right place for the feedback I have, if not please
> correct me. Well the below may be for the bugtracker. Except: It is yet to
> be confirmed as bug, I was unable to find a way to register. (I do not like
> the concept of open id / one service = one login = one password)
> 
> Here goes:
> 
> Looking through the code I came across:
> src\http\modules\ngx_http_limit_req_module.c  line 816
> In function ngx_http_limit_req_zone
> 
> This parses the arguments to the config directive:   limit_req_zone
> 
> If the line in the config has more than one entry starting with "$" then in
> line  816 the previous value of cfg is lost
>     for (i = 1; i < cf->args->nelts; i++) {  // line 752
> ...
>         if (value[i].data[0] == '$') {  // line 811
> ...
>             ctx = ngx_pcalloc(cf->pool, sizeof(ngx_http_limit_req_ctx_t));
> 
> Of course normally this does not matter, since nginx will exit if the config
> cannot be parsed (and thus free all memory)

In either case, it's not a memory leak.  All allocations will be 
freed on next configuration reload.  While the memory is wasted, 
it's not leaked.

> Yet the below line is accepted by nginx.
>   limit_req_zone  $nginx_version  $binary_remote_addr zone=addr_foo:20m  ;
> 
> In this case, reading the config, a single small block of memory is leaked.
> 
> This should probably be fixed by giving an error that this config is
> malformed.
> 
> Btw, it also takes
>   limit_req_zone  $nginx_version   zone=addr_foo:20m zone=addr_foo:20m  ;
> 
> Anything, that has 3 parameters.  Any parameter can be repeated.

This applies to almost all directives which take named parameters: 
nginx doesn't try to check for duplicates, but silently uses last 
value specified.

Additional checks can be added to make parsing more strict, though 
general consensus seems to be that it isn't worth the effort.

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