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

[J Carter]

Hello, 

Thanks the review and feedback.

On Sat, 16 Dec 2023 05:57:28 +0300
Maxim Dounin <mdounin at mdounin.ru> wrote:

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

Ah thanks, will include the full stop in future.

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

Yes, that was the obvious one - I suspect upstream_random balancers
might act strangely in extreme cases (I'm not entirely sure though).

Other uses I'm not sure at all, as those are more domain specific 
(like resolver).

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

Yes the alternative would seem to be #ifdefs for win32 vs unix, or 
adding in an 'ngx_random_max', or writing (or borrowing) a posix
random implementation into nginx's codebase.

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

Thanks, will note for the future.

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

Yep, makes sense.

> 
> 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()
>  
>  
> 
Looks good to me.