[PATCH] Avoid unnecessary restriction on nohash http variables

[Maxim Dounin]

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/