[PATCH] Avoid unnecessary restriction on nohash http variables

[Alexey Radkov]

Thanks! It looks good.
Cheers, Alexey.

вт, 26 окт. 2021 г., 03:42 Maxim Dounin <mdounin at mdounin.ru>:

> Hello!
>
> On Thu, Aug 19, 2021 at 09:34:39PM +0300, Alexey Radkov wrote:
>
> > # HG changeset patch
> > # User Alexey Radkov <alexey.radkov at gmail.com>
> > # Date 1629395487 -10800
> > #      Thu Aug 19 20:51:27 2021 +0300
> > # Node ID a1065b2252855730ed8e5368c88fe41a7ff5a698
> > # Parent  13d0c1d26d47c203b1874ca1ffdb7a9ba7fd2d77
> > Avoid unnecessary restriction on nohash http variables.
> >
> > When I use variables with long names albeit being tagged as
> > NGX_HTTP_VARIABLE_NOHASH, Nginx says "could not build variables_hash,
> > you should increase variables_hash_bucket_size: 64". It seems that this
> is
> > an unnecessary restriction, as soon as the hash gets only built for
> variables
> > with names[n].key.data == NULL (note that other pieces in ngx_hash_init()
> > where the macro NGX_HASH_ELT_SIZE is used, are always guarded with this
> > condition). This fix puts this same condition into the only unguarded
> piece:
> > when testing against the hash_bucket_size.
> >
> > The issue arises after assignment of key[n].key.data = NULL without
> symmetric
> > assignment of key[n].key.len in ngx_http_variables_init_vars(): after
> this,
> > the key[n].key comes to an inconsistent state. Perhaps this was made
> > intentionally, as hash initialization in other places seems to follow the
> > same pattern (for instance, see how ngx_hash_init() gets called from
> > ngx_http_upstream_hide_headers_hash()).
> >
> > Without this fix, I must put in the config "variables_hash_bucket_size
> 128;"
> > even if the long-named variables are nohash.
> >
> > diff -r 13d0c1d26d47 -r a1065b225285 src/core/ngx_hash.c
> > --- a/src/core/ngx_hash.c     Fri Aug 13 03:57:47 2021 -0400
> > +++ b/src/core/ngx_hash.c     Thu Aug 19 20:51:27 2021 +0300
> > @@ -274,6 +274,9 @@
> >      }
> >
> >      for (n = 0; n < nelts; n++) {
> > +        if (names[n].key.data == NULL) {
> > +            continue;
> > +        }
> >          if (hinit->bucket_size < NGX_HASH_ELT_SIZE(&names[n]) +
> sizeof(void *))
> >          {
> >              ngx_log_error(NGX_LOG_EMERG, hinit->pool->log, 0,
> >
>
> Thanks for spotting this.
>
> Here is a version of the patch slightly cleaned up to better match
> style, please take a look if looks good for you:
>
> # HG changeset patch
> # User Alexey Radkov <alexey.radkov at gmail.com>
> # Date 1629395487 -10800
> #      Thu Aug 19 20:51:27 2021 +0300
> # Node ID 2a7155733855d1c2ea1c1ded8d1a4ba654b533cb
> # Parent  3f0ab7b6cd71eb02b4714278cabcd2db5c79b3a9
> Core: removed unnecessary restriction in hash initialization.
>
> Hash initialization ignores elements with key.data set to NULL.
> Nevertheless, the initial hash bucket size check didn't skip them,
> resulting in unnecessary restrictions on, for example, variables with
> long names and with the NGX_HTTP_VARIABLE_NOHASH flag.
>
> Fix is to update the initial hash bucket size check to skip elements
> with key.data set to NULL, similarly to how it is done in other parts
> of the code.
>
> diff --git a/src/core/ngx_hash.c b/src/core/ngx_hash.c
> --- a/src/core/ngx_hash.c
> +++ b/src/core/ngx_hash.c
> @@ -274,6 +274,10 @@ ngx_hash_init(ngx_hash_init_t *hinit, ng
>      }
>
>      for (n = 0; n < nelts; n++) {
> +        if (names[n].key.data == NULL) {
> +            continue;
> +        }
> +
>          if (hinit->bucket_size < NGX_HASH_ELT_SIZE(&names[n]) +
> sizeof(void *))
>          {
>              ngx_log_error(NGX_LOG_EMERG, hinit->pool->log, 0,
>
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20211026/6d74e036/attachment.htm>