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

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

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.

I agree with Antoine. Exhausting the FDs is not the problem, the problem is the misleading error.

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

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

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

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

> 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 ?

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

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

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?

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

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

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

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

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

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

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

Attaching a patch to make error reporting better.

> 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

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.

Good point, Donald. os.urandom() is the only (simple) way to access the Windows randomness pool.

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

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

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?

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

Attached patch to make os.urandom's fd persistent.

Updated error handling patch testing for ENODEV.

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

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

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

Ok, committed. We're left with the persistent fd patch for 3.4.

Updated patch for persistent fd.

Updated patch after Christian's comments.

LGTM

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?

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.

Ok, you're gonna laugh, the simplified patch has a complication (not theoretical, it would trip test_cmd_line). Attaching patch.

> 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

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

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

Ok, the tiger should feel better now :-)

So, to come back to the original topic, is everyone sold on the idea of caching the urandom fd lazily?

Lazily opening urandom and holding it open sounds like a sane thing to do to me +1

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

Ok, I've committed the patch for the lazy opening approach.