[PATCH 2 of 2] Win32: extended ngx_random range to 0x7fffffff
Sergey Kandaurov
pluknet at nginx.com
Fri Dec 22 13:52:30 UTC 2023
On Thu, Dec 21, 2023 at 07:14:40PM +0300, Maxim Dounin wrote:
> Hello!
>
> On Thu, Dec 21, 2023 at 05:37:02PM +0400, Sergey Kandaurov wrote:
>
> > > 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.
>
> It's intentionally aligned this way to simplify reading. Such
> style is occasionally used in complex macro definitions, see
> ngx_mp4_get_32value() or ngx_proxy_protocol_parse_uint32() for
> some examples.
>
> Just for completeness, below is the updated patch.
>
> # HG changeset patch
> # User J Carter <jordanc.carter at outlook.com>
> # Date 1702111094 0
> # Sat Dec 09 08:38:14 2023 +0000
> # Node ID 92923ac5ea2a395774b28460f07d0fd2e1a2de24
> # Parent cc16989c6d61385027c1ebfd43929f8369fa5f62
> Win32: extended ngx_random() range to 0x7fffffff.
>
> rand() is used on win32. RAND_MAX is implementation defined. win32's is
> 0x7fff.
I'd unwrap this line to look more pleasant, it fits into 80 columns.
>
> Existing uses of ngx_random() rely upon 0x7fffffff range provided by
> POSIX implementations of random().
>
> diff -r cc16989c6d61 -r 92923ac5ea2a src/os/win32/ngx_win32_config.h
> --- a/src/os/win32/ngx_win32_config.h Sat Dec 16 03:40:01 2023 +0400
> +++ b/src/os/win32/ngx_win32_config.h Sat Dec 09 08:38:14 2023 +0000
> @@ -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) \
If I'm not mistaken, indentation is now 5 spaces.
Otherwise, looks good.
> + ^ ((uint32_t) rand() << 8) \
> + ^ ((uint32_t) rand()) )))
> +
> #define ngx_debug_init()
>
>
>
More information about the nginx-devel
mailing list