[Bug libstdc++/57920] New: [c++11] Linux: std::random_device reads too much from /dev/urandom

[Bug libstdc++/57920] New: [c++11] Linux: std::random_device reads too much from /dev/urandom

[f.heckenbach@fh-soft.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

            Bug ID: 57920
           Summary: [c++11] Linux: std::random_device reads too much from
                    /dev/urandom
           Product: gcc
           Version: 4.7.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: f.heckenbach@fh-soft.de

As the test below shows, a single invocation of
std::random_device::operator() reads 4k from /dev/urandom, which is
rather wasteful of the entropy collected in the random device pool.

Of course, in theory, reading 4k and using just 4 bytes of it will
only decrease the entropy by 4 bytes, not 4k, but the kernel can't
know that. When you read 4k from /dev/urandom, it has to assume it
will be used, so it will reduce the entropy by 4k (which is
typically all it has). This means that a subsequent read from
/dev/random (by the same or another process) will block, often
unnecessarily because actually enough entropy was available. This is
particularly annoying since std::random_device is often just used to
seed a PRNG which needs just a few random bytes.

So while buffered reading is almost always a good thing, I contend
it's not in this case. I'd suggest to read unbuffered by default,
which may entail using read() instead of fread().

% cat test.cpp
#include <random>

int main ()
{
  std::random_device rd;
  rd ();
}
% g++-4.7 -std=c++11 test.cpp
% strace ./a.out
[...]
open("/dev/urandom", O_RDONLY)          = 3
fstat64(3, {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 9), ...}) = 0
ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbfffec50) = -1 EINVAL (Invalid
argument)
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0xb7fdf000
read(3, [...], 4096) = 4096
close(3)                                = 0

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

--- Comment #1 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Note that in 4.8.x and mainline for modern x86 and x86_64 targets we don't use
/dev/urandom at all, we use __x86_rdrand. In general, the idea is that more
targets should use hardware support for random numbers and /dev/urandom become
just a fall back. I'm not sure changing fread to read it's worth the trouble,
and the change, since we are not talking about a regression, would not go in
4.7.x branch anyway. Are you on x86 / x86_64 or something else?

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

--- Comment #2 from Paolo Carlini <paolo.carlini at oracle dot com> ---
I mean we use __builtin_ia32_rdrand32_step ;)

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2013-07-21
           Assignee|unassigned at gcc dot gnu.org      |paolo.carlini at oracle dot com
     Ever confirmed|0                           |1

--- Comment #3 from Paolo Carlini <paolo.carlini at oracle dot com> ---
I'm going to attach a patchlet which does the trick (fread -> read) for me.
Note I'm on purpose disabling the use of __builtin_ia32_rdrand32_step on my
x86_64 machine, the undef would not be in the committed patch of course.

It would be great if you could test the change on your machines (in 4.7.x just
change random.h, no need to rebuild) and confirm that everything is fine.

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

--- Comment #4 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Created attachment 30534
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=30534&action=edit
Draft mainline patch

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[f.heckenbach@fh-soft.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

--- Comment #5 from Frank Heckenbach <f.heckenbach@fh-soft.de> ---
(In reply to Paolo Carlini from comment #1)
> Note that in 4.8.x and mainline for modern x86 and x86_64 targets we don't
> use /dev/urandom at all, we use __x86_rdrand. In general, the idea is that
> more targets should use hardware support for random numbers and /dev/urandom
> become just a fall back. I'm not sure changing fread to read it's worth the
> trouble, and the change, since we are not talking about a regression, would
> not go in 4.7.x branch anyway. Are you on x86 / x86_64 or something else?

I use an AMD (Thuban) in 32 bit mode. This processor core is ~3 years old
and AFAICS it doesn't support rdrand. So even if newer AMDs do supports it
(which I don't know), I guess it's fair to say that for some more years
there will be processors around which don't, and in this case I assume
gcc falls back to /dev/urandom.

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[f.heckenbach@fh-soft.de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

--- Comment #6 from Frank Heckenbach <f.heckenbach@fh-soft.de> ---
(In reply to Paolo Carlini from comment #3)
> I'm going to attach a patchlet which does the trick (fread -> read) for me.
> Note I'm on purpose disabling the use of __builtin_ia32_rdrand32_step on my
> x86_64 machine, the undef would not be in the committed patch of course.
> 
> It would be great if you could test the change on your machines (in 4.7.x
> just change random.h, no need to rebuild) and confirm that everything is
> fine.

I did the equivalent change to /usr/include/c++/4.7/bits/random.h and it works
for me, thanks.

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

--- Comment #7 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Agreed, let's just commit the improvement. If/when you become aware of ways to
extend the use of builtins to other CPUs / targets, please let me know, thanks.

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |4.9.0

--- Comment #8 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Done.

[Bug libstdc++/57920] [c++11] Linux: std::random_device reads too much from /dev/urandom

[gnu at binarywings dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57920

Florian Philipp <gnu at binarywings dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gnu at binarywings dot net

--- Comment #9 from Florian Philipp <gnu at binarywings dot net> ---
Created attachment 32110
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=32110&action=edit
Patch without POSIX I/O

I find the original patch as applied to mainline unnecessarily complicated. You
can achieve the same just with std I/O by deactivating buffering with
std::setbuf. Basically a one line change when applied to the original version.