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

Afacb3e6f1c16e4887f7827505104ddd?d=identicon&s=25 Martin Frb (Guest)
on 2014-08-23 20:04
(Received via mailing list)
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)

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.


Best Regards
Martin
A8108a0961c6087c43cda32c8616dcba?d=identicon&s=25 Maxim Dounin (Guest)
on 2014-08-25 18:28
(Received via mailing list)
Hello!

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

>
>         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.

>
> 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/
Please log in before posting. Registration is free and takes only a minute.
Existing account

NEW: Do you have a Google/GoogleMail, Yahoo or Facebook account? No registration required!
Log in with Google account | Log in with Yahoo account | Log in with Facebook account
No account? Register here.