code review 4170056: crypto/rand: Added read buffer to speed up requests for...

crypto/rand: Added read buffer to speed up requests for small amounts of bytes.

[albert.strasheim, 2011-02-17 15:28:53]

Hello agl1, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[agl1, 2011-02-17 15:34:44]

LGTM

[rsc, 2011-02-17 15:39:38]

HOLD ON

[rsc, 2011-02-17 15:42:17]

bufio.NewReader will try to fill its buffer
when you call Read the first time, which
will mean it will grab something like 8K of
random data.  That's a lot of randomness
to steal from the system.  Does it matter?

It might be better to call NewReaderSize
with a smallish buffer size: big enough to
amortize the reads but small enough not
to drain the randomness from the system.
What number is that?  128, 256, 512?
Or does it not matter at all with /dev/urandom?

What is the motivating situation here,
just out of curiosity?

[agl1, 2011-02-17 16:20:50]

On Thu, Feb 17, 2011 at 10:42 AM, Russ Cox <rsc@golang.org> wrote:
> Or does it not matter at all with /dev/urandom?

It doesn't really matter with /dev/urandom. It doesn't drain entropy,
it just generates pseudo-random data from a state that is randomly
updated.

The only concern would be that the kernel takes slightly longer to
generate a buffer's worth of pseudo-random data than to generate a
small amount. But the fixed overhead of the cross into kernel land and
pulling the PRNG state into cache means that there's not a lot in it.


AGL

[rsc, 2011-02-17 16:30:23]

LGTM

[agl1, 2011-02-17 20:14:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=dea835a8ff3e ***

crypto/rand: Added read buffer to speed up requests for small amounts of bytes.

R=agl1, rsc
CC=golang-dev
http://codereview.appspot.com/4170056

Committer: Adam Langley <agl@golang.org>

[fw, 2011-02-17 21:33:42]

* Adam Langley:

> On Thu, Feb 17, 2011 at 10:42 AM, Russ Cox <rsc@golang.org> wrote:
>> Or does it not matter at all with /dev/urandom?
>
> It doesn't really matter with /dev/urandom. It doesn't drain entropy,
> it just generates pseudo-random data from a state that is randomly
> updated.

On many systems, it drains entropy from the pool shared with
/dev/random, causing future reads from /dev/random to block.

Before virtualization, the common wisdom was that applications should
use 32 or so bytes from /dev/urandom to seed their own generator
(which should have proper fork and thread protection).  With
widespread virtualization, I'm no longer sure what to recommend.

[agl1, 2011-02-17 21:37:56]

On Thu, Feb 17, 2011 at 4:33 PM, Florian Weimer <fw@deneb.enyo.de> wrote:
> On many systems, it drains entropy from the pool shared with
> /dev/random, causing future reads from /dev/random to block.

Which systems? I don't object to having a local PRNG if it's required,
but I don't want to implement workarounds without good cause.


AGL

[fw, 2011-02-17 21:42:53]

* Adam Langley:

> On Thu, Feb 17, 2011 at 4:33 PM, Florian Weimer <fw@deneb.enyo.de> wrote:
>> On many systems, it drains entropy from the pool shared with
>> /dev/random, causing future reads from /dev/random to block.
>
> Which systems?

Older Linux-based systems, probably from the 2.6.18 time-frame.  The
pools weren't separate.  I'm a bit surprised that this seems to have
been fixed.

> I don't object to having a local PRNG if it's required,
> but I don't want to implement workarounds without good cause.

Understood.  It's actually quite tricky, and only possible if you
don't care that the generator might yield the same bit stream after VM
snapshotting.