Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

#13704: Random number generator in Python core

Can't Edit
Can't Publish+Mail
Start Review
8 years ago by lists
8 years ago
martin, pitrou, greg
gvanrossum, loewis, barry, Georg, rhettinger, jcea, AntoinePitrou, christian.heimes, Benjamin Peterson, Arfrever, alex, dmalcolm, rhettinger

Patch Set 1 #

Total comments: 15
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Include/pydebug.h View 1 chunk +1 line, -0 lines 0 comments Download
Include/pyhash.h View 1 chunk +7 lines, -0 lines 0 comments Download
Include/pyrandom.h View 1 chunk +18 lines, -0 lines 4 comments Download
Include/Python.h View 1 chunk +1 line, -0 lines 0 comments Download
Include/pythonrun.h View 1 chunk +0 lines, -1 line 0 comments Download
Lib/test/test_sys.py View 1 chunk +1 line, -1 line 0 comments Download
Makefile.pre.in View 3 chunks +4 lines, -0 lines 0 comments Download
Modules/main.c View 2 chunks +5 lines, -0 lines 0 comments Download
Modules/posixmodule.c View 2 chunks +2 lines, -48 lines 0 comments Download
Modules/_randommodule.c View 7 chunks +19 lines, -193 lines 0 comments Download
Objects/object.c View 1 chunk +1 line, -1 line 1 comment Download
Objects/unicodeobject.c View 1 chunk +1 line, -1 line 1 comment Download
Python/hash.c View 1 chunk +39 lines, -0 lines 2 comments Download
Python/pythonrun.c View 3 chunks +9 lines, -0 lines 0 comments Download
Python/random.c View 1 chunk +344 lines, -0 lines 7 comments Download
Python/sysmodule.c View 3 chunks +4 lines, -2 lines 0 comments Download


Total messages: 4
http://bugs.python.org/review/13704/diff/3928/12971 File Python/random.c (right): http://bugs.python.org/review/13704/diff/3928/12971#newcode71 Python/random.c:71: if (hCryptProv == 0) { Since using a RNG ...
8 years ago #1
On 2012/01/03 23:52:35, loewis wrote: > Since using a RNG becomes essentially mandatory with this ...
8 years ago #2
http://bugs.python.org/review/13704/diff/3928/12960 File Include/pyrandom.h (right): http://bugs.python.org/review/13704/diff/3928/12960#newcode4 Include/pyrandom.h:4: int PyOS_URandom(unsigned char *buf, Py_ssize_t len); I don't think ...
8 years ago #3
8 years ago #4
Overall I like the approach you are taking in this change better than the
earlier work seen in issue13703.  I've added comments.

Strictly speaking, this change encompasses more than just what you have
described in this issue and still tries to address 13703.  I'd commit the random
number code refactoring to provide a _PyOS_URandom API as its own change and
leave the hash changes in a separate change that builds on and uses that API for
its seed.

File Objects/object.c (right):

Objects/object.c:762: x = Py_RndHashSeed + ((Py_uhash_t) *p << 7);
Use ^ instead of +.

File Objects/unicodeobject.c (right):

Objects/unicodeobject.c:11110: x = Py_RndHashSeed + ((Py_uhash_t)*P << 7); \
Use ^ instead of +.  Consistent with my suggestion in object.c [or in 3.2 and
earlier in bytesobject.c].

File Python/hash.c (right):

Python/hash.c:4: Py_hash_t Py_RndHashSeed = -1;
This should be a Py_uhash_t and the default value should be 0 so that the
behavior if it is never set to anything else is the same as today's non-seeded

Instead of being a global, should this be a member of PyInterpreterState and be
initialized from pystate.c's PyInterpreterState_New()?  For 3.3 I think that
makes sense.  A process with multiple embedded Python interpreters would have
different hash seeds for each of them.

For backports it will have to be a global to avoid changing the
PyInterpreterState definition (API).

Python/hash.c:24: _Py_MT_GenRand_Init(&state, seed);
Include the pid, system uptime and any other easy to get environmental info into
the seed.  No sane OS is going to use this code path...

But it should be better than simply time if at all possible as the time an
interpreter serving something on a network started up is often easy to

File Python/random.c (right):

Python/random.c:15: if (Py_RndHashSeed == -1) {           \
On 2012/01/04 18:54:01, AntoinePitrou wrote:
> These macros obscure the code. Why do you want to force a Py_FatalError?
> Also, why is Py_RndHashSeed at all mentioned in a file that only deals with
> random numbers, not hashes?

Agreed, don't force a fatal error.  -1 is a perfectly valid random seed value
and code in this file shouldn't even know about Py_RndHashSeed at all.

What I think you were trying to do is get clever with the error routines to
decide if they could even call PyErr_Set* or not.  If you want to determine
that, do it some other way than via a magic hash seed value.

Python/random.c:145: if ((fd = open(DEV_URANDOM, O_RDONLY)) == -1) {
On 2012/01/04 18:54:01, AntoinePitrou wrote:
> You may want to release the GIL when doing I/O.

Agreed... But be careful!  This is going to be called both before and after the
GIL has been initialized.  It needs to test if the GIL is even available before
attempting to use release or acquire it.

Python/random.c:146: error_errno(PyExc_OSError, DEV_URANDOM, "Can't open
On 2012/01/04 18:54:01, AntoinePitrou wrote:
> If DEV_URANDOM can be overriden, the error message should reuse it, not
> the default value.

agreed.  implicit string concatenation is your friend here (and below).
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+