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

[Sergey Kandaurov]

> On 16 Dec 2023, at 06:57, 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.
> 
>> 
>> 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()
> 
> 

Nitpicking: you might want to re-align the "^" operator to the first
symbol of the left-hand operand (similar to NGX_CONF_TAKE1234, or
even NGX_UNIX_ADDRSTRLEN).  Other than that, it looks good.

-- 
Sergey Kandaurov