Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3.6 on Windows doesn't seed Random() well enough #73271

Closed
nedbat opened this issue Dec 27, 2016 · 10 comments
Closed

Python 3.6 on Windows doesn't seed Random() well enough #73271

nedbat opened this issue Dec 27, 2016 · 10 comments
Assignees
Labels
3.7 (EOL) end of life

Comments

@nedbat
Copy link
Member

nedbat commented Dec 27, 2016

BPO 29085
Nosy @brettcannon, @rhettinger, @pfmoore, @vstinner, @nedbat, @tjguk, @zware, @eryksun, @zooba, @dstufft, @ammaraskar, @altendky

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = 'https://github.com/rhettinger'
closed_at = <Date 2016-12-29.04:03:32.185>
created_at = <Date 2016-12-27.17:39:24.375>
labels = ['3.7']
title = "Python 3.6 on Windows doesn't seed Random() well enough"
updated_at = <Date 2017-01-09.10:08:17.263>
user = 'https://github.com/nedbat'

bugs.python.org fields:

activity = <Date 2017-01-09.10:08:17.263>
actor = 'vstinner'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2016-12-29.04:03:32.185>
closer = 'python-dev'
components = []
creation = <Date 2016-12-27.17:39:24.375>
creator = 'nedbat'
dependencies = []
files = []
hgrepos = []
issue_num = 29085
keywords = ['3.6regression']
message_count = 10.0
messages = ['284118', '284120', '284121', '284122', '284123', '284127', '284192', '284194', '284225', '285031']
nosy_count = 13.0
nosy_names = ['brett.cannon', 'rhettinger', 'paul.moore', 'vstinner', 'nedbat', 'tim.golden', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower', 'dstufft', 'ammar2', 'altendky']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue29085'
versions = ['Python 3.6', 'Python 3.7']

@nedbat
Copy link
Member Author

nedbat commented Dec 27, 2016

Creating two Random() instances in quick succession produces the same sequence, but only on Windows on Python 3.6. On 3.5 or earlier, or on Mac/Linux, the randomization is good.

Python 3.6.0 (v3.6.0:41df79263a11, Dec 23 2016, 07:18:10) [MSC v.1900 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import random; print(random.Random().randint(1, 999999), random.Random().randint(1, 999999))
903885 903885
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(2)])
996947 56476
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(2)])
793282 793282
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(2)])
519702 519702
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(2)])
230678 230678
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(3)])
474701 474701 474701
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(3)])
890942 890942 890942
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(3)])
997495 997495 997495
>>> import random; print(*[random.Random().randint(1, 999999) for _ in range(5)])
27803 27803 27803 27803 919401
>>>

I would expect each of these runs to produce unique numbers, with no duplicates.

@nedbat
Copy link
Member Author

nedbat commented Dec 27, 2016

Adding a time.sleep(0.001) (or maybe even just 0) between the calls prevents the problem.

@dstufft
Copy link
Member

dstufft commented Dec 27, 2016

It's presumably failing to read nonblocking random from CrpytGen and falling back to seeding using time and pid.

@ammaraskar
Copy link
Member

Can recreate under

Python 3.6.0 (v3.6.0:41df79263a11, Dec 23 2016, 08:06:12) [MSC v.1900 64 bit (AMD64)] on win32

and on latest master.

@ammaraskar ammaraskar added the 3.7 (EOL) end of life label Dec 27, 2016
@altendky
Copy link
Mannequin

altendky mannequin commented Dec 27, 2016

time.sleep(0) and time.sleep(0.0) acted the same for me and both exhibited matching 'random' values in some cases. (win10, python3.6) I also printed time.time() with each 'random' value and it wasn't a perfect match but matching times tended to go with matching 'random' values.

@eryksun
Copy link
Contributor

eryksun commented Dec 27, 2016

It's presumably failing to read nonblocking random from CrpytGen
and falling back to seeding using time and pid.

Yes and no. CryptGenRandom is not failing, but it is nonetheless calling random_seed_time_pid:

    >>> r = random.Random()
    Breakpoint 0 hit
    python36_d!random_seed:
    00000000`5e7277c0 4889542410      mov     qword ptr [rsp+10h],rdx
                                        ss:00000095`053ef0e8=0000000000000000
    0:000> g
    Breakpoint 1 hit
    python36_d!random_seed_time_pid:
    00000000`5e728910 48894c2408      mov     qword ptr [rsp+8],rcx
                                        ss:00000095`053ef040=000002319b6f3b88
    0:000> kc 3
    Call Site
    python36_d!random_seed_time_pid
    python36_d!random_seed
    python36_d!random_new

It looks like there's a bug in the new implementation of random_seed():

     if (arg == NULL || arg == Py_None) {
        if (random_seed_urandom(self) >= 0) {
            PyErr_Clear();

            /* Reading system entropy failed, fall back on the worst entropy:
               use the current time and process identifier. */
            random_seed_time_pid(self);
        }
        Py_RETURN_NONE;
    }

If random_seed_urandom fails, it returns -1, so this should be testing if the result is less than 0.

@brettcannon
Copy link
Member

Does this affect the secrets module at all?

@eryksun
Copy link
Contributor

eryksun commented Dec 28, 2016

The secrets module uses SystemRandom, which overrides random() and getrandbits() to use os.urandom, so it's not affected.

@python-dev
Copy link
Mannequin

python-dev mannequin commented Dec 29, 2016

New changeset 0a55e039d25f by Benjamin Peterson in branch '3.6':
fix error check, so that Random.seed actually uses OS randomness (closes bpo-29085)
https://hg.python.org/cpython/rev/0a55e039d25f

New changeset fc3eab44765f by Benjamin Peterson in branch 'default':
merge 3.6 (bpo-29085)
https://hg.python.org/cpython/rev/fc3eab44765f

@python-dev python-dev mannequin closed this as completed Dec 29, 2016
@vstinner
Copy link
Member

vstinner commented Jan 9, 2017

Oh, sorry, I introduced the bug in Python 3.6 with the PEP-524 (os.urandom() now blocks on Linux). Too bad that there is no simple way to write an unit test for that.

... but only on Windows on Python 3.6

With the bug, or when the fix when _PyOS_URandomNonblock() fails, Random.seed() uses:

  • Reading system entropy failed, fall back on the worst entropy:
    use the current time and process identifier. */
    random_seed_time_pid(self);

It's just that on Windows, the system clock has a resolution around 15 ms, whereas it has a resolution better than 1 us on Linux. So it's just that calling Random.seed() usually takes longer than the resolution of the system clock on Linux :-) Not really that the bug is specific to Windows.

Thanks for the fix Benjamin!

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.7 (EOL) end of life
Projects
None yet
Development

No branches or pull requests

7 participants