[PATCH 2 of 2] Win32: extended ngx_random range to 0x7fffffff

[Maxim Dounin]

Hello!

On Sat, Dec 09, 2023 at 08:42:11AM +0000, J Carter wrote:

> On Sat, 09 Dec 2023 07:46:10 +0000
> J Carter <jordanc.carter at outlook.com> wrote:
> 
> > # HG changeset patch
> > # User J Carter <jordanc.carter at outlook.com>
> > # Date 1702101635 0
> > #      Sat Dec 09 06:00:35 2023 +0000
> > # Node ID 1a77698f82d2580aa8b8f62ce89b4dbb6d678c5d
> > # Parent  d9275e982a7188a1ea7855295ffa93362ea9830f
> > Win32: extended ngx_random range to 0x7fffffff
> > 
> > rand() is used on win32. RAND_MAX is implementation defined. win32's is
> > 0x7fff.
> > 
> > Existing uses of ngx_random rely upon 0x7fffffff range provided by
> > posix implementations of random().
> > 
> > diff -r d9275e982a71 -r 1a77698f82d2 src/os/win32/ngx_win32_config.h
> > --- a/src/os/win32/ngx_win32_config.h	Sat Dec 09 05:09:07 2023 +0000
> > +++ b/src/os/win32/ngx_win32_config.h	Sat Dec 09 06:00:35 2023 +0000
> > @@ -280,7 +280,9 @@
> >  
> >  #define NGX_HAVE_GETADDRINFO         1
> >  
> > -#define ngx_random               rand
> > +#define ngx_random                                                             \
> > +    ((rand() << 16) | (rand() << 1) | (rand() >> 14))
> > +
> >  #define ngx_debug_init()
> >  
> >  
> 
> ^ my mistake - copying error..
> 
> # HG changeset patch
> # User J Carter <jordanc.carter at outlook.com>
> # Date 1702111094 0
> #      Sat Dec 09 08:38:14 2023 +0000
> # Node ID 10ef59a412a330872fc6d46de64f42e32e997b3a
> # Parent  d9275e982a7188a1ea7855295ffa93362ea9830f
> Win32: extended ngx_random range to 0x7fffffff

Nitpicking:

Win32: extended ngx_random() range to 0x7fffffff.

> 
> rand() is used on win32. RAND_MAX is implementation defined. win32's is
> 0x7fff.
> 
> Existing uses of ngx_random rely upon 0x7fffffff range provided by
> posix implementations of random().

Thanks for catching this.

As far as I can see, the only module which actually relies on the 
range is the random index module.  Relying on the ngx_random() 
range generally looks wrong to me, and we might want to change the 
code to don't.  OTOH, it's the only way to get a completely 
uniform distribution, and that's what the module tries to do.  As 
such, it might be good enough to preserve it as is, at least till 
further changes to ngx_random().

Either way, wider range for ngx_random() should be beneficial in 
other places.

> 
> diff -r d9275e982a71 -r 10ef59a412a3 src/os/win32/ngx_win32_config.h
> --- a/src/os/win32/ngx_win32_config.h	Sat Dec 09 05:09:07 2023 +0000
> +++ b/src/os/win32/ngx_win32_config.h	Sat Dec 09 08:38:14 2023 +0000
> @@ -280,7 +280,9 @@
>  
>  #define NGX_HAVE_GETADDRINFO         1
>  
> -#define ngx_random               rand
> +#define ngx_random()                                                             \

Nitpicking: the "\" character should be at the 79th column (in 
some files at 78th).  This ensures that a diff won't wrap on a 
standard 80-column terminal.

> +    ((rand() << 16) | (rand() << 1) | (rand() >> 14))
> +
>  #define ngx_debug_init()

Using "|" for random numbers looks utterly wrong to me, even if 
ORed values are guaranteed to not interfere.

I would rather use "^", and avoid dependency on the particular 
value of RAND_MAX (other than POSIX-required minimum of 32767) by 
using something like

   0x7fffffff & ((rand() << 16) ^ (rand() << 8) ^ rand())

with proper typecasts.

Something like this should work:

diff --git a/src/os/win32/ngx_win32_config.h b/src/os/win32/ngx_win32_config.h
--- a/src/os/win32/ngx_win32_config.h
+++ b/src/os/win32/ngx_win32_config.h
@@ -280,7 +280,11 @@ typedef int                 sig_atomic_t
 
 #define NGX_HAVE_GETADDRINFO         1
 
-#define ngx_random               rand
+#define ngx_random()                                                          \
+    ((long) (0x7fffffff & ( ((uint32_t) rand() << 16)                         \
+                          ^ ((uint32_t) rand() << 8)                          \
+                          ^ ((uint32_t) rand()) )))
+
 #define ngx_debug_init()
 
 

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