bpo-40286: Add randbytes() method to random.Random by vstinner · Pull Request #19527 · python/cpython · GitHub

Add random.randbytes() function and random.Random.randbytes()
method to generate random bytes.

Modify secrets.token_bytes() to use SystemRandom.randbytes()
rather than calling directly os.urandom().

Rename also genrand_int32() to genrand_uint32(), since it returns an
unsigned 32-bit integer, not a signed integer.

https://bugs.python.org/issue40286

TODO: handle properly inheritance. Currently, random.Random is kind of weird. The base class directly implements Mersenne Twister, it's not an abstract class. Maybe a getrandbytes() implementation using getrandbits() should be implemented for subclasses?

cc @pitrou @serhiy-storchaka

I'm not sure if my Mersenne Twister implementation is correct. Not sure if it should handle endianness differently for example.

I think your MT implementation is fine. I don't think we guarantee identical results on different architectures. Otherwise you'll need to choose conventional endianness indeed. @mdickinson

I'm not sure if we should really care about subclassing here other than the classes bundled with Python. Otherwise you'll need to do a bit of plumbing in __init_subclass__.

Allow n = 0.
The result should be deterministic and do not depend on endianess. Ideally it should be identical to getrandbits(n*8).to_bytes(n, 'little') (or 'big').
See also Raymod's suggestion about the name.

Would be nice to add the following tests:

Call seek() with some constant and compare the result of getrandbytes() with a constant.
Compare the results of several sequential calls of getrandbits() and getrandbytes() for identical Random objects (or after seek() with the same seed).

I would write

Suggested change

      
                    size_t chunk = Py_MIN(n, 4);
          
                    memcpy(ptr, &word, chunk);
          
                    ptr += chunk;
          
                    n -= chunk;
          
                    if (n < 4) {
          
                        memcpy(ptr, &word, n);
          
                        break;
          
                    }
          
                    memcpy(ptr, &word, 4);
          
                    ptr += 4;
          
                    n -= 4;

memcpy() and incrementing/decrementing with constant size may be faster.

Also do not forget about endianess.

I would write (...)

Done.

Also do not forget about endianess.

I don't know how to handle the endianness. Should use htonl() to convert word to big endian?

Yes, you can use htonl() or swap bytes manually, as it is done in many places in CPython for performace. Search WORDS_BIGENDIAN, PY_BIG_ENDIAN and PY_LITTLE_ENDIAN. Perhaps the most efficient way is:

x = ...
x = ((x & 0x00FF00FFu) << 8) | ((x >> 8) & 0x00FF00FFu);
x = ((x & 0x0000FFFFu) << 16) | ((x >> 16) & 0x0000FFFFu));

I created https://bugs.python.org/issue40302 "Add _Py_bswap32() function to pyport.h".

I updated my PR to rename the method to randbytes().

Allow n = 0.

Done.

I do not think it is needed. It may confuse the user if the degenerate case be explicitly mentioned for one and only one method.

It's kind of surprising that a function documented as "generate random bytes" returns a deterministic (non random) result, so I consider that it's worth it to document it.

... explicitly mentioned for one and only one method.

What do you mean? Currently, only randbytes() has this convenience behavior no? I asked Antoine to add a similar sentence for his getrandbits(0) PR.

randrange(1, 2), uniform(10, 10) and choice([7]) all return deterministic result. I think that most if not all of random functions can return deterministic result for some arguments.

Alright, I remove this sentence.

Maybe I should wait until https://bugs.python.org/issue40302 "Add _Py_bswap32() function to pyport.h" is solved, rather than using htonl(). I'm not 100% sure that htonl() uses uint32_t on all platforms. It may use "long" type on some platforms and long may be larger than uint32_t.

htonl() should use 32-bit integers. The name is historical. But it is better to wait for _Py_bswap32(). AFAIK htonl() is not so optimized on all platforms.

If genrand_uint32() produces numbers 0x01234567 and 0x89abcdef, then getrandbits(6*8) will return 0x89ab01234567, but randbytes(6) will return bytes.fromhex('0123456789ab'). It combines bits in different order.

If genrand_uint32() produces numbers 0x01234567 and 0x89abcdef, then getrandbits(6*8) will return 0x89ab01234567, but randbytes(6) will return bytes.fromhex('0123456789ab'). It combines bits in different order.

Is it a question? Are you asking to change the result?

FWIW, I don't think there is a problem if two different methods produce different results.

It would be nice if there is a simple relation between getrandbits() and randbytes(). This would help to implement a fallback for older Python versions.

It would be nice if there is a simple relation between getrandbits() and randbytes(). This would help to implement a fallback for older Python versions.

Here you have:

import random

def randbytes(gen, n):
    data = bytearray(n)
    i = 0
    while i < n:
        chunk = 4
        word = gen.getrandbits(32)
        word = word.to_bytes(4, 'big')
        chunk = min(n, 4)
        data[i:i+chunk] = word[:chunk]
        i += chunk
    return bytes(data)

gen = random.Random()
gen.seed(8675309)
assert randbytes(gen, 16) == b'f\xf9\xa836\xd0\xa4\xf4\x82\x9f\x8f\x19\xf0eo\x02'

The last line (test) is copied from the unit test of this PR: Python 3.7 with this script and Python 3.9 with my PR produces the same bytes.

I rebased my PR to fix a merge conflict, I fixed the NEWS entry and added more tests to address Serhiy's latest review.

I think that the following implementation would be simpler:

def randbytes(gen, n):
    return gen.getrandbits(n * 8).to_bytes(n, 'little')

It is actually the most efficient code that is used now in cases when you need randbytes().

I prefer to design randbytes() to be efficient and reliable, rather than trying to be compatible with various recipes to generate bytes currently used in the wild.

Serhiy:

I think that the following implementation would be simpler: (...)

People already worked around the lack of randbytes() and have their own implementation, see: https://bugs.python.org/issue40286 for various recipes.

Antoine:

FWIW, I don't think there is a problem if two different methods produce different results.

Yeah, I don't think that we should bother with compatibility with users creative ways to generate random bytes.

For example, some people use sha1(str(random.getrandbits(8*20))).digest() which gives a completely different result than randbyte() of this PR.

Some other people prefer an "explicit" bytes(random.randint(0, 255) for i in range(size)) which consumes 4x more entropy than my proposed implementation: each Python randint() call Mersenne Twister genrand_int32() which consumes 32-bits at once, even if only 8 bits are used.

Add random.randbytes() function and random.Random.randbytes() method to generate random bytes. Modify secrets.token_bytes() to use SystemRandom.randbytes() rather than calling directly os.urandom(). Rename also genrand_int32() to genrand_uint32(), since it returns an unsigned 32-bit integer, not a signed integer. The _random module is now built with Py_BUILD_CORE_MODULE defined.

I rebased my PR to get https://bugs.python.org/issue40302 _Py_bswap32() and squashed commits. I replaced the htohl() call with:

#if PY_LITTLE_ENDIAN
        /* Convert to big endian */
        word = _Py_bswap32(word);
#endif

Thanks for the review @serhiy-storchaka and @pitrou!

* master: (1985 commits) bpo-40179: Fix translation of #elif in Argument Clinic (pythonGH-19364) bpo-35967: Skip test with `uname -p` on Android (pythonGH-19577) bpo-40257: Improve help for the typing module (pythonGH-19546) Fix two typos in multiprocessing (pythonGH-19571) bpo-40286: Use random.randbytes() in tests (pythonGH-19575) bpo-40286: Makes simpler the relation between randbytes() and getrandbits() (pythonGH-19574) bpo-39894: Route calls from pathlib.Path.samefile() to os.stat() via the path accessor (pythonGH-18836) bpo-39897: Remove needless `Path(self.parent)` call, which makes `is_mount()` misbehave in `Path` subclasses. (pythonGH-18839) bpo-40282: Allow random.getrandbits(0) (pythonGH-19539) bpo-40302: UTF-32 encoder SWAB4() macro use a|b rather than a+b (pythonGH-19572) bpo-40302: Replace PY_INT64_T with int64_t (pythonGH-19573) bpo-40286: Add randbytes() method to random.Random (pythonGH-19527) bpo-39901: Move `pathlib.Path.owner()` and `group()` implementations into the path accessor. (pythonGH-18844) bpo-40300: Allow empty logging.Formatter.default_msec_format. (pythonGH-19551) bpo-40302: Add pycore_byteswap.h header file (pythonGH-19552) bpo-40287: Fix SpooledTemporaryFile.seek() return value (pythonGH-19540) Minor modernization and readability improvement to the tokenizer example (pythonGH-19558) bpo-40294: Fix _asyncio when module is loaded/unloaded multiple times (pythonGH-19542) Fix parameter names in assertIn() docs (pythonGH-18829) bpo-39793: use the same domain on make_msgid tests (python#18698) ...

vstinner requested a review from rhettinger as a code owner April 14, 2020 22:38

the-knights-who-say-ni added the CLA signed label Apr 14, 2020

bedevere-bot added the awaiting core review label Apr 14, 2020

pitrou reviewed Apr 15, 2020

View reviewed changes

Comment thread Lib/random.py Outdated

Comment thread Modules/_randommodule.c Outdated

Comment thread Modules/_randommodule.c Outdated

serhiy-storchaka reviewed Apr 15, 2020

View reviewed changes

vstinner changed the title bpo-40286: Add getrandbytes() method to random.Random bpo-40286: Add randbytes() method to random.Random Apr 15, 2020

serhiy-storchaka reviewed Apr 15, 2020

View reviewed changes

serhiy-storchaka reviewed Apr 16, 2020

View reviewed changes

Comment thread Lib/test/test_random.py Outdated

Comment thread Lib/test/test_random.py Outdated

Comment thread Misc/NEWS.d/next/Library/2020-04-15-00-39-25.bpo-40286.ai80FA.rst Outdated

serhiy-storchaka approved these changes Apr 17, 2020

View reviewed changes

Comment thread Lib/random.py Outdated

bedevere-bot added awaiting merge and removed awaiting core review labels Apr 17, 2020

vstinner merged commit 9f5fe79 into python:master Apr 17, 2020

bedevere-bot removed the awaiting merge label Apr 17, 2020

vstinner deleted the getrandbytes branch April 17, 2020 17:05

bpo-40286: Add randbytes() method to random.Random by vstinner · Pull Request #19527 · python/cpython · GitHub

Repository navigation

bpo-40286: Add randbytes() method to random.Random - #19527

bpo-40286: Add randbytes() method to random.Random#19527vstinner merged 1 commit intopython:masterpython/cpython:masterfrom vstinner:getrandbytesCopy head branch name to clipboard

Conversation

vstinner commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Apr 14, 2020

Uh oh!

vstinner commented Apr 14, 2020

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner commented Apr 15, 2020

Uh oh!

vstinner commented Apr 15, 2020

Uh oh!

serhiy-storchaka Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

vstinner Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Apr 16, 2020

Uh oh!

serhiy-storchaka commented Apr 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Apr 16, 2020

Uh oh!

vstinner commented Apr 17, 2020

Uh oh!

pitrou commented Apr 17, 2020

Uh oh!

serhiy-storchaka commented Apr 17, 2020

Uh oh!

vstinner commented Apr 17, 2020

Uh oh!

vstinner commented Apr 17, 2020

Uh oh!

serhiy-storchaka commented Apr 17, 2020

Uh oh!

vstinner commented Apr 17, 2020

Uh oh!

Uh oh!

vstinner commented Apr 17, 2020

Uh oh!

vstinner commented Apr 17, 2020

Uh oh!

bpo-40286: Add randbytes() method to random.Random#19527
vstinner merged 1 commit into
python:masterfrom
vstinner:getrandbytes

vstinner commented Apr 14, 2020 •
edited
Loading

vstinner Apr 17, 2020 •
edited
Loading