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

#13704: Random number generator in Python core

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 11 months ago by lists
Modified:
7 years, 11 months ago
Reviewers:
martin, pitrou, greg
CC:
gvanrossum, loewis, barry, Georg, rhettinger, jcea, AntoinePitrou, christian.heimes, Benjamin Peterson, Arfrever, alex, dmalcolm, rhettinger
Visibility:
Public.

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

Messages

Total messages: 4
loewis
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 ...
7 years, 11 months ago #1
christian.heimes
On 2012/01/03 23:52:35, loewis wrote: > Since using a RNG becomes essentially mandatory with this ...
7 years, 11 months ago #2
AntoinePitrou
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 ...
7 years, 11 months ago #3
gregory.p.smith
7 years, 11 months 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.

http://bugs.python.org/review/13704/diff/3928/12967
File Objects/object.c (right):

http://bugs.python.org/review/13704/diff/3928/12967#newcode762
Objects/object.c:762: x = Py_RndHashSeed + ((Py_uhash_t) *p << 7);
Use ^ instead of +.

http://bugs.python.org/review/13704/diff/3928/12968
File Objects/unicodeobject.c (right):

http://bugs.python.org/review/13704/diff/3928/12968#newcode11110
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].

http://bugs.python.org/review/13704/diff/3928/12969
File Python/hash.c (right):

http://bugs.python.org/review/13704/diff/3928/12969#newcode4
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
behavior.

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).

http://bugs.python.org/review/13704/diff/3928/12969#newcode24
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
determine.

http://bugs.python.org/review/13704/diff/3928/12971
File Python/random.c (right):

http://bugs.python.org/review/13704/diff/3928/12971#newcode15
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.

http://bugs.python.org/review/13704/diff/3928/12971#newcode145
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.

http://bugs.python.org/review/13704/diff/3928/12971#newcode146
Python/random.c:146: error_errno(PyExc_OSError, DEV_URANDOM, "Can't open
/dev/urandom");
On 2012/01/04 18:54:01, AntoinePitrou wrote:
> If DEV_URANDOM can be overriden, the error message should reuse it, not
hardcode
> 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+