Clean up, simplify, and re-work Perl_seed(). Fixes #22968

[scottchiefbaker]

There are a ton of #ifdefs in Perl_seed() which make the code hard to read and understand. This PR should simplify that code considerably by removing and combining #ifdefs. This will also do a much better (and more modern) job of hashing state variables to generate a random seed if we do not use the /dev/urandom code path.

Overall code clean-up, moderization, and simplification.

[scottchiefbaker]

It's worth noting, per my email to p5p, that after this patch is applied we're not using RANDBITS anywhere in code anymore. We were barely using it before this patch, but this removes the last pieces that reference it.

[demerphq]

I wouldn't make this change. I'd prefer to see it as

u = (U32)SEED_C1 * when.tv_sec + (U32)SEED_C4 * when.tv_usec;

That is id avoid SEED_C2 (3 is prime but its tiny) in favour of SEED_C4. Consider:

$ perl -le'printf "%032b\n", 1000003'
00000000000011110100001001000011
$ perl -le'printf "%032b\n", 73819'
00000000000000010010000001011011

The purpose of multiplying by a prime is to cause single bit changes to result in multi-bit changes. Simply summing the raw time data will not have this affect. Also remember that multiplication by a prime (actually any odd number) is a reversible operation, meaning that it is collision free.

[scottchiefbaker]

The goal on this line isn't to make a good random number with lots of entropy, it's to calculate milliseconds.

The previous constant multipliers were a hack because we didn't have hashing in core. Now that we do, conceptually things are a lot simpler: gather some semi-random state variables and hash them to generate better entropy.

The hashing will handle all the bit spreading better than any simple multiplication ever did.

[demerphq]

I think you are missing the point, before we were multiplying before we added. Now you are adding and then doing the ptr hash. Its not the same thing.

perl -le'printf "%10d - %032b\n", ($_)x2 for 1*73819,2*1000003,1*73819+2*1000003
> '
     73819 - 00000000000000010010000001011011
   2000006 - 00000000000111101000010010000110
   2073825 - 00000000000111111010010011100001

If you want use the ptr_hash() on each element of when. Or multiply them by different primes. Whatever. Just dont add them first and then do ptr_hash().

[scottchiefbaker]

I'm purposely not trying to do the same thing here.

This particular line calculates milliseconds of uptime, that's all. This happens to require multiplication but I am not using it to generate entropy like we were previously.

Previously we were trying to generate entropy by multiplying a bunch of state variables together. In this PR I'm hashing a bunch of variables together to generate entropy. Hashing should provide better entropy and bit spreading than multiplication.

[bulk88]

Perl's GTOD() portability macro, has no guarantees on update frequency or granularity. Just b/c the milli/micro/ns portion/precision is there as a data type, Means nothing if the low half is never updated by Perl's OS specific port code, or by that OS'es libc.

GTOD() has stung (bugged broken) WinPerl and WinPerl CPAN eco system (NYTProf/p5ps Time::HiRES) many times in history. Specific reasons are Metaconfig picking ancient 1 SECOND fallbacks, Or Window kernels 15 ms rule, for updating/polling wall time from the HW wall time clock. 15 ms can be changed to 1 ms if process has semi-root privilege, but its very un-user friendly to ever turn that setting. Battery life/IPC sluggishness reasons.

Instead of GTOD, you probably want RDTSC/QueryPerformanceCounter/or the unix analogs in Time::HiRes'es src code.

See also Mingw's static linked into user binds code for the Win32/MSVCs stack cookie sentinal feature

mingw-w64/mingw-w64@e6ac7e4#diff-de3c44bfb45c9896cfe8a93c1e3b87001659d644876201678f07044edc8e714dL62

[scottchiefbaker]

I purposely chose four different state variables so that a failure in any one (or two?) should not affect the randomness we generate from the hash of all the variables. I venture a guess that at least 80% of Perl installations these days have /dev/urandom and will never hit the GTOD code path anyway.

Even without using time of any kind we still use PID, a stack pointer, and the local variable pointer which should be psuedo-random enough to generate a simple seed.

If we didn't have /dev/urandom and GTOD was broke, AND the local pointer was compromised we might start to have a problem.

Blead today uses these same state variables (and has for 20+ years now), but used multiplication to generate entropy instead of a hash. In other words, if it hasn't been broken for the last 20 years, nothing is changing in this implementation.

[scottchiefbaker]

As one small follow-up point. This code has one purpose: generating a single 32bit psuedo-random number. That number should only be used as a seed for a real PRNG. As long as this number is mostly-sorta-kinda good then the PRNG will handle generating "good" random numbers.

[demerphq]

I am sorry, i do not feel comfortable with this part of your patch. Especially given @bulk88 's comments. I like the general direction you want to go, but I feel like this patch could do better, and I feel like adding the components of the time together like that without any kind of difference magnification could have negative consequences, so I could not in good conscience approve the patch with this change.

All I know is that historically using time for initializing RNG's has been problematic, and id feel more more confident we weren't doing any harm if we were a bit more aggressive about magnifying differences in the time data before we mixed it together for the final initialization.

For instance I would feel much better if the code in question was reformulated as:

 UV uptime = ptr_hash(when.tv_sec) + ptr_hash(when.tv_usec);

Multiplying by different primes is also an option.

If some other committer, especially one who uses Win32 or one of the other affected platforms (VMS maybe?) felt differently then that would be their call and on their conscience. As I wont be affected by this patch, and as I feel uncomfortable about it, I wont be approving it in its current state.

Honestly I do not understand the vehemence of your objection to being more careful with this data. It doesn't make sense to me. Being more aggressive about it can do no harm, and might do some good, so why object so strongly?

[scottchiefbaker]

I guess I'm misunderstanding the goal of changing things as you requested.

We're mixing up a bunch of state variables to generate a PRNG seed that is neither predictable or repeatable. The hashing should be doing all the bit mixing of the variables, but you're talking about doing some pre-mixing of the variables by doing arbitrary multiplication. I don't think multiplying the variables before we mix them will make things any more secure or random. The "difference magnification" in this case is handled by the ptr_hash(uptime) on line 4739 after we have calculated milliseconds.

It's like we are making a milkshake and one of the steps is pre-mixing the milk, chocolate, and ice cream with a fork in a bowl before you put it in the blender. It's just another step that would be done better by the blender anyway.

It won't hurt anything, but it seems unnecessary to me. It's not a hill I want to die on though, so I'm happy to capitulate and make the change you suggested. For the sake of moving things forward I will implement:

 UV uptime = ptr_hash(when.tv_sec) + ptr_hash(when.tv_usec);

in the PR if this is something you're will to buy off on and we can move forward with getting this landed.