Issue 18756: os.urandom() fails under high load (original) (raw)
Created on 2013-08-16 15:21 by christian.heimes, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (47)
Author: Christian Heimes (christian.heimes) * 
Date: 2013-08-16 15:21
I have seen complains from e.g. Tarek that os.urandom() fails under high load: https://twitter.com/tarek_ziade/status/362281268215418880
The problem is caused by file descriptor limits. os.urandom() opens /dev/urandom for every call. How about os.urandom() uses a persistent file descriptor? That should eliminate the error. It may alsos speed up os.urandom() because a persistent FD gets rid of open() and close() syscalls.
- open /dev/urandom on first call of os.urandom() and store the fd in a static var
- invalidate, close fd on read() -> EINVAL; open /dev/urandom again
- close fd when interpreter shuts down
Author: STINNER Victor (vstinner) *
Date: 2013-08-16 15:30
I have seen complains from e.g. Tarek that os.urandom() fails under high load: https://twitter.com/tarek_ziade/status/362281268215418880
dev_urandom_python() should handle ENFILE and ENOENT differently to raise a different exception. Or it should always call PyErr_SetFromErrno(PyExc_OSError); ?
Can tarek tell us more about its usecases: is he directly calling os.urandom() or does he use the random module? How many threads?
How about os.urandom() uses a persistent file descriptor?
os.urandom() is called at Python startup to generate a "secret key" for random hash. If the file descriptor is never closed, the next file descriptor will be 4 instead of the expect 3.
Always keeping an internal file descriptor open may have unexpected effects like leaking a file descriptor to a child process... (see the PEP 446, not implemented yet).
I'm ok to keep a fd open if the user controls the lifetime of the object (lifetime of the fd). For example, I expect that rng = SystemRandom() opens /dev/urandom when the object is created, and closes it when the object is destroyed.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 15:40
I don't think that's bug in os.urandom(). If os.urandom() doesn't fail, something else will fail soon after. OTOH, the error is clearly misleading. The NotImplementedError should only be raised for certain errnos (such as ENOENT, ENODEV, ENXIO and EACCES), not all of them.
Author: Jesús Cea Avión (jcea) *
Date: 2013-08-16 15:59
I agree with Antoine. Exhausting the FDs is not the problem, the problem is the misleading error.
Author: Tarek Ziadé (tarek) *
Date: 2013-08-16 16:19
If os.urandom() doesn't fail, something else will fail soon after.
the random pool can be exhausted, but this is not "soon after" I think. In Linux and Mac OS X, ulimit -n defaults to 512 and 256.
It's very easy to reach that limit if you write a web app that uses this API.
I agree with Antoine. Exhausting the FDs is not the problem,
Do you suggest that we should not use os.urandom on high load ?
Opening an FD on every call sounds under optimal, I am not seeing any drawback not to try to optimize that API.
Author: Tarek Ziadé (tarek) *
Date: 2013-08-16 16:22
Can tarek tell us more about its usecases: is he directly calling os.urandom() or does he use the random module? How many threads?
I was using ws4py inside greenlets. ws4py uses os.urandom() to generate some keys. So one single thread, many greenlets.
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 16:24
If os.urandom() doesn't fail, something else will fail soon after.
the random pool can be exhausted, but this is not "soon after" I think. In Linux and Mac OS X, ulimit -n defaults to 512 and 256.
I don't think he's referring to the entropy pool, but to RLIMIT_NOFILE. You'll likely hit EMFILE sooner or later, e.g. on socket(), open()...
It's very easy to reach that limit if you write a web app that uses this API.
I agree with Antoine. Exhausting the FDs is not the problem,
Do you suggest that we should not use os.urandom on high load ?
What does high load mean? If you mean many concurrent threads, then you should probably go for the random module, no?
Opening an FD on every call sounds under optimal, I am not seeing any drawback not to try to optimize that API.
Well, first we'll have to make the code thread-safe, if we want to keep a persistent FD open. Which means we'll have to add a lock, which is likely to reduce concurrency, and overall throughput.
Author: Christian Heimes (christian.heimes) *
Date: 2013-08-16 16:26
Tarek Ziadé added the comment:
If os.urandom() doesn't fail, something else will fail soon after.
the random pool can be exhausted, but this is not "soon after" I think. In Linux and Mac OS X, ulimit -n defaults to 512 and 256.
It's highly unlikely that you are every going to exhaust the CPRNG to a point were it is no longer cryptographically secure. Thomas Ptacek pointed me to http://security.stackexchange.com/a/3939 yesterday.
I agree with Antoine. Exhausting the FDs is not the problem,
Do you suggest that we should not use os.urandom on high load ?
Opening an FD on every call sounds under optimal, I am not seeing any drawback not to try to optimize that API.
The drawback is a slightly more complicated implementation that has to deal with invalid FDs.
Author: Tarek Ziadé (tarek) *
Date: 2013-08-16 16:27
What does high load mean?
a web app with a few hundreds concurrent requests.
If you mean many concurrent threads, then you should probably go for the random module, no?
I use greenlets. But, I don't know - are you suggesting os.urandom() should be marked in the documentation as "DOES NOT SCALE" and I should use another API ? Which one ?
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 16:33
2013/8/16, Tarek Ziadé <report@bugs.python.org>:
I use greenlets. But, I don't know - are you suggesting os.urandom() should be marked in the documentation as "DOES NOT SCALE" and I should use another API ? Which one ?
Well, even with greenlets, I assume you're using at least one FD (socket) per client, no? So you can get EMFILE on socket() just as on os.urandom(). The only difference is that sockets are long-lived, whereas os.urandom() only opens a FD for a couple ms. So os.urandom() isn't your biggest problem here. I'd suggest you to just open '/dev/urandom' once, and then make all your threads/green-threads read from it. IMO os.urandom() is a really poor API ;-)
Author: Tarek Ziadé (tarek) *
Date: 2013-08-16 16:38
Well, even with greenlets, I assume you're using at least one FD (socket) per client, no? So you can get EMFILE on socket() just as on os.urandom().
I do many calls on urandom() so that's the FD bottleneck.
So os.urandom() isn't your biggest problem here.
Of course it is. But it looks like you know better without having looked at the code. :)
I'd suggest you to just open '/dev/urandom' once, and then make all your threads/green-threads read from it.
Let me know how to do this without being able to prevent the API to close the FD everytime.
IMO os.urandom() is a really poor API ;-)
Then we should improve it or deprecate it.
Author: Christian Heimes (christian.heimes) *
Date: 2013-08-16 16:43
Am 16.08.2013 18:24, schrieb Charles-François Natali:
Well, first we'll have to make the code thread-safe, if we want to keep a persistent FD open. Which means we'll have to add a lock, which is likely to reduce concurrency, and overall throughput.
Why locking? /dev/urandom is a pseudo char device. You can have multiple readers on the same fd without any locking. Did you know that Java keeps one persistent fd to /dev/urandom?
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 16:43
Well, even with greenlets, I assume you're using at least one FD (socket) per client, no? So you can get EMFILE on socket() just as on os.urandom().
I do many calls on urandom() so that's the FD bottleneck.
Unless you're doing many calls in parallel it's unlikely to be a bottleneck. At worse you can write your own /dev/urandom reading code, with a shared fd amongst all your threads / greenlets.
os.urandom() is a convenience function, it doesn't have to be extremely optimized.
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 16:47
I do many calls on urandom() so that's the FD bottleneck.
So os.urandom() isn't your biggest problem here.
Of course it is. But it looks like you know better without having looked at the code. :)
So please explain me :-) os.urandom() can only be called by one thread/greenlet at a time. So I assumed you're using a per-client thread/greenlet, and so a per-client socket. So, you have O(N) open sockets, which are long-lived. OTOH, you can only have so many threads inside os.urandom() at a time, since it's short lived, and the FD is closed as soon as urandom() returns. So I would assume that you have asymptotically at least as many open sockets than FDs open to os.urandom.
I'd suggest you to just open '/dev/urandom' once, and then make all your threads/green-threads read from it.
Let me know how to do this without being able to prevent the API to close the FD everytime.
Simply open('/dev/urandom', 'rb').
IMO os.urandom() is a really poor API ;-)
Then we should improve it or deprecate it.
I don't think it can be fixed. I think Christian's working on a PEP for random number generators, which would probably make it easier, although I din't have a look at it.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 16:48
Am 16.08.2013 18:24, schrieb Charles-François Natali:
Well, first we'll have to make the code thread-safe, if we want to keep a persistent FD open. Which means we'll have to add a lock, which is likely to reduce concurrency, and overall throughput.
Why locking? /dev/urandom is a pseudo char device. You can have multiple readers on the same fd without any locking.
You must put a lock around the open() call, though, to avoid calling it several times and losing an fd.
Author: Christian Heimes (christian.heimes) *
Date: 2013-08-16 16:54
Am 16.08.2013 18:47, schrieb Charles-François Natali:
I don't think it can be fixed. I think Christian's working on a PEP for random number generators, which would probably make it easier, although I din't have a look at it.
In the light of the recent Android issue with PRNGs [1] I don't think that Python should roll out its own CPRNG. I'd rather use the operation system's CPRNG or OpenSSL's CPRNG. After all we aren't crypto experts. I'd rather point my finger to OpenSSL than take the blame for a faulty CPRNG.
[1] http://bitcoin.org/en/alert/2013-08-11-android
Author: Tarek Ziadé (tarek) *
Date: 2013-08-16 16:54
Unless you're doing many calls in parallel it's unlikely to be a bottleneck.
That's what we're saying since message 1. Antoine, allo quoi! :)
os.urandom() is a convenience function, it doesn't have to be extremely optimized
I suggest that you tell it the documentation then, and explain that it does not scale and people should write their own thing.
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 16:56
Why locking? /dev/urandom is a pseudo char device. You can have multiple readers on the same fd without any locking.
You must put a lock around the open() call, though, to avoid calling it several times and losing an fd.
Exactly (unless the FD is open during the module initialization, instead of using lazy-open upon first os.urandom() call).
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 16:58
In the light of the recent Android issue with PRNGs [1] I don't think that Python should roll out its own CPRNG. I'd rather use the operation system's CPRNG or OpenSSL's CPRNG. After all we aren't crypto experts. I'd rather point my finger to OpenSSL than take the blame for a faulty CPRNG.
Yeah, sure. But it would be nice to have an API similar to the random module (i.e. a Random ABC, which could have several implementations, among which an /dev/urandom backed one). The underlying FD lifetime would be tied to the Random object lifetime, and we couldn't have to open/close it at each call.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 17:06
Attaching a patch to make error reporting better.
Author: Tarek Ziadé (tarek) *
Date: 2013-08-16 17:08
So please explain me :-).
it sounded like you did not really want any explanation
os.urandom() can only be called by one thread/greenlet at a time.
do you mean that we cannot have two parallel calls of that function ? e.g. two opened FD at the same time ?
So I would assume that you have asymptotically at least as many open sockets than FDs open to os.urandom.
a web socket application that spawns one socket per connection, then uses a lib that calls many times os.urandom(), will generate most of its FDs on os urandom
but since you said that os.urandom() should not be used in the first place - that's what I will keep in mind
Author: Donald Stufft (dstufft) *
Date: 2013-08-16 17:13
Just to be explicit, open("/dev/urandom") only works on POSIX platforms while os.usrandom should work on any supported platform that has an OS level source of randomness. So advocating for simply using open() is probably a bad idea unless the target OS is only POSIX.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 17:14
Good point, Donald. os.urandom() is the only (simple) way to access the Windows randomness pool.
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 17:29
Attaching a patch to make error reporting better.
Why didn't you include ENODEV? Apparently it can be reported in some corner cases, e.g. in this patch: http://lfs-matrix.net/patches/downloads/linux/linux-2.6.14.2-pseudo_random-1.patch
Otherwise, wouldn't self.addCleanup be simpler than the large try/finally block in the test (but it's not available on 2.7)?
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 17:32
Why didn't you include ENODEV? Apparently it can be reported in some corner cases, e.g. in this patch: http://lfs-matrix.net/patches/downloads/linux/linux-2.6.14.2-pseudo_random-1.patch
That isn't mentioned in the POSIX open() spec: http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
However ENODEV still seems to be a standard errno constant, so why not: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
Otherwise, wouldn't self.addCleanup be simpler than the large try/finally block in the test (but it's not available on 2.7)?
The problem is if some code tries to create a fd before the cleanup callback is called. With a try/finally block we're guaranteed not to have such a problem.
Author: Donald Stufft (dstufft) *
Date: 2013-08-16 17:33
Looking at random.SystemRandom it appears it would suffer from the same FD exhaustion problem.
So as of right now afaik none of the sources of cryptographically secure random in the python stdlib offer a way to open a persistent FD. The primary question on my mind is if os.urandom can't be modified to maintain a persistent FD can Python offer a urandom class that will maintain a persistent FD?
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 17:37
So as of right now afaik none of the sources of cryptographically secure random in the python stdlib offer a way to open a persistent FD. The primary question on my mind is if os.urandom can't be modified to maintain a persistent FD can Python offer a urandom class that will maintain a persistent FD?
Well, if we want to offer such a facility, let's bundle it in os.urandom(). It would be suboptimal to have two slightly different implementations of the same thing.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 17:50
Attached patch to make os.urandom's fd persistent.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 17:53
Updated error handling patch testing for ENODEV.
Author: Charles-François Natali (neologix) *
Date: 2013-08-16 18:42
Updated error handling patch testing for ENODEV.
LGTM, you can apply to 2.7 and 3.x (I just hope all those errnos are available on every POSIX platform ;-).
Author: Roundup Robot (python-dev) 
Date: 2013-08-16 18:52
New changeset 193bcc12575d by Antoine Pitrou in branch '3.3': Issue #18756: Improve error reporting in os.urandom() when the failure is due to something else than /dev/urandom not existing. http://hg.python.org/cpython/rev/193bcc12575d
New changeset fe949918616c by Antoine Pitrou in branch 'default': Issue #18756: Improve error reporting in os.urandom() when the failure is due to something else than /dev/urandom not existing. http://hg.python.org/cpython/rev/fe949918616c
Author: Roundup Robot (python-dev)
Date: 2013-08-16 18:54
New changeset ec296a36156b by Antoine Pitrou in branch '2.7': Issue #18756: Improve error reporting in os.urandom() when the failure is due to something else than /dev/urandom not existing. http://hg.python.org/cpython/rev/ec296a36156b
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 18:54
Ok, committed. We're left with the persistent fd patch for 3.4.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 19:09
Updated patch for persistent fd.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-16 20:37
Updated patch after Christian's comments.
Author: Christian Heimes (christian.heimes) *
Date: 2013-08-16 20:45
LGTM
Author: STINNER Victor (vstinner) *
Date: 2013-08-17 07:46
Tarek: try to use ssl.RAND_bytes(), it is secure, fast and don't use a file descriptor.
IMO if something can be improved, it is in the random.SystemRandom() class: it can keep the FD open. Does the class have a method to generate random bytes?
Author: Donald Stufft (dstufft) *
Date: 2013-08-17 08:17
haypo: It's been suggested by a number of security professionals that using the OpenSSL random (or really any random) instead of urandom is likely to be a smarter idea. The likelyhood that urandom is broken is far less than any other source of random. This can be seen in the recent issues on the Android platform. This is not to say that there's a reason to believe that OpenSSL is broken currently, but that the chances are higher for it to be than /dev/urandom. An example of when this happened was http://www.debian.org/security/2008/dsa-1571.
There's no reason to believe that OpenSSL is wrong right now, but the chances of OpenSSL being wrong are greater than the chances of /dev/urandom being
There's been a few threads on twitter about it in light of the Android SecureRandom issue (don't need to read these, just here for reference): - https://twitter.com/tqbf/status/368089082800246784 - https://twitter.com/tqbf/status/367793231808843777 - https://twitter.com/tqbf/status/368089362333827072
I don't think it actually matters if os.urandom or random.SystemRandom is the preferred interface that keeps the FD open but I do believe there should be one implementation that will use the OS source of random and maintain a persistent FD.
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-23 20:04
Ok, you're gonna laugh, the simplified patch has a complication (not theoretical, it would trip test_cmd_line). Attaching patch.
Author: Charles-François Natali (neologix) *
Date: 2013-08-24 17:49
New changeset fe949918616c by Antoine Pitrou in branch 'default': Issue #18756: Improve error reporting in os.urandom() when the failure is due to something else than /dev/urandom not existing. http://hg.python.org/cpython/rev/fe949918616c
Antoine, this changeset broke Tiger buildbots badly: """
ERROR: test_urandom_failure (test.test_os.URandomTests)
Traceback (most recent call last): File "/Users/db3l/buildarea/3.x.bolen-tiger/build/Lib/test/test_os.py", line 1033, in test_urandom_failure ValueError: not allowed to raise maximum limit """
http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/6826/steps/test/logs/stdio
""" 1030 # We restore the old limit as soon as possible. If doing it 1031 # using addCleanup(), code running in between would fail 1032 # creating any file descriptor. 1033 resource.setrlimit(resource.RLIMIT_NOFILE, (soft_limit, hard_limit)) """
The code trying to reset RLIMIT_NOFILE to its previous value fails, and as a consequence, all subsequent FDs creation fail (since the soft limit it 1)...
It looks like Tiger getrlimit() return a nonsensical value (maybe -1), which means that you can't do setrlimit(getrlimit()), or yet another OS X bug (TM). See e.g. http://www.couchbase.com/issues/browse/MB-3064
I'd suggest two things:
- skip it on Tiger (using support.requires_mac_vers() decorator)
- run the test in a subprocess (using the test in your latest patch would be fine), to be more robust against this
Author: Charles-François Natali (neologix) *
Date: 2013-08-24 18:10
Or more precisely, just run the test in a subprocess. That should fix the OS X failure if we don't restore the RLIMIT_NOFILE limits, and will make the test more robust (but you can't reuse the new test, since it won't work with lazy-opening).
Author: Roundup Robot (python-dev)
Date: 2013-08-24 18:54
New changeset b9e62929460e by Antoine Pitrou in branch '3.3': Issue #18756: make test_urandom_failure more robust by executing its code in a subprocess http://hg.python.org/cpython/rev/b9e62929460e
New changeset 68ff013b194c by Antoine Pitrou in branch 'default': Issue #18756: make test_urandom_failure more robust by executing its code in a subprocess http://hg.python.org/cpython/rev/68ff013b194c
New changeset 869df611c138 by Antoine Pitrou in branch '2.7': Issue #18756: make test_urandom_failure more robust by executing its code in a subprocess http://hg.python.org/cpython/rev/869df611c138
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-24 18:55
Ok, the tiger should feel better now :-)
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-24 19:32
So, to come back to the original topic, is everyone sold on the idea of caching the urandom fd lazily?
Author: Donald Stufft (dstufft) *
Date: 2013-08-24 19:34
Lazily opening urandom and holding it open sounds like a sane thing to do to me +1
Author: Roundup Robot (python-dev)
Date: 2013-08-30 22:26
New changeset acc7439b1406 by Antoine Pitrou in branch 'default': Issue #18756: os.urandom() now uses a lazily-opened persistent file descriptor, so as to avoid using many file descriptors when run in parallel from multiple threads. http://hg.python.org/cpython/rev/acc7439b1406
Author: Antoine Pitrou (pitrou) *
Date: 2013-08-30 22:27
Ok, I've committed the patch for the lazy opening approach.
History
Date
User
Action
Args
2022-04-11 14:57:49
admin
set
github: 62956
2013-09-20 12:32:44
Adam.Bielański
set
nosy: + Adam.Bielański
2013-08-30 22:27:37
pitrou
set
status: open -> closed
resolution: fixed
messages: +
stage: patch review -> resolved
2013-08-30 22:26:49
python-dev
set
messages: +
2013-08-24 19:34:22
dstufft
set
messages: +
2013-08-24 19:32:47
pitrou
set
messages: +
2013-08-24 18:55:54
pitrou
set
messages: +
2013-08-24 18:54:58
python-dev
set
messages: +
2013-08-24 18:10:53
neologix
set
messages: +
2013-08-24 17:49:35
neologix
set
messages: +
2013-08-23 20:04:28
pitrou
set
files: + persistent_urandom_fd4.patch
messages: +
2013-08-17 08:17:12
dstufft
set
messages: +
2013-08-17 07:46:46
vstinner
set
messages: +
2013-08-16 20:45:44
christian.heimes
set
messages: +
2013-08-16 20:37:46
pitrou
set
files: + persistent_urandom_fd3.patch
messages: +
2013-08-16 19:09:38
pitrou
set
files: + persistent_urandom_fd2.patch
messages: +
2013-08-16 18:54:59
pitrou
set
stage: needs patch -> patch review
messages: +
versions: - Python 2.7, Python 3.3
2013-08-16 18:54:18
python-dev
set
messages: +
2013-08-16 18:52:05
python-dev
set
nosy: + python-dev
messages: +
2013-08-16 18:42:48
neologix
set
messages: +
2013-08-16 17:53:56
pitrou
set
files: + urandom_error2.patch
messages: +
2013-08-16 17:50:16
pitrou
set
files: + persistent_urandom_fd.patch
type: behavior -> resource usage
messages: +
2013-08-16 17:37:38
pitrou
set
messages: +
2013-08-16 17:33:44
dstufft
set
messages: +
2013-08-16 17:32:28
pitrou
set
messages: +
2013-08-16 17:29:08
neologix
set
messages: +
2013-08-16 17:21:54
alex
set
nosy: + alex
2013-08-16 17:14:27
pitrou
set
messages: +
2013-08-16 17:13:08
dstufft
set
nosy: + dstufft
messages: +
2013-08-16 17:08:58
tarek
set
messages: +
2013-08-16 17:06:34
pitrou
set
files: + urandom_error.patch
keywords: + patch
messages: +
2013-08-16 16:58:47
neologix
set
messages: +
2013-08-16 16:56:54
neologix
set
messages: +
2013-08-16 16:54:41
tarek
set
messages: +
2013-08-16 16:54:32
christian.heimes
set
messages: +
2013-08-16 16:48:31
pitrou
set
messages: +
2013-08-16 16:47:14
neologix
set
messages: +
2013-08-16 16:43:45
pitrou
set
messages: +
2013-08-16 16:43:31
christian.heimes
set
messages: +
2013-08-16 16:38:02
tarek
set
messages: +
2013-08-16 16:33:19
neologix
set
messages: +
2013-08-16 16:27:03
tarek
set
messages: +
2013-08-16 16:26:19
christian.heimes
set
messages: +
2013-08-16 16:24:23
neologix
set
messages: +
2013-08-16 16:22:54
tarek
set
messages: +
2013-08-16 16:19:12
tarek
set
messages: +
2013-08-16 15:59:17
jcea
set
nosy: + jcea
messages: +
2013-08-16 15:40:13
pitrou
set
nosy: + pitrou, neologix
messages: +
2013-08-16 15:30:57
vstinner
set
messages: +
2013-08-16 15:21:38
christian.heimes
create