Cache the file descriptor for /dev/urandom to avoid needing to reopen it.

Cache the file descriptor for /dev/urandom to avoid needing to reopen it for every call.

Patch by Mike Mammarella.

BUG=none
TEST=none

[Mike Mammarella [gmail], 2009-06-22 22:23:57]

Once the renderers are sandboxed on Linux, they won't be able to reopen
/dev/urandom. So cache it the first time it's opened. Later, this will have to
be done prior to enforcing the sandbox on renderers.

[Mark Mentovai, 2009-06-23 03:28:53]

+dean

[Dean McNamee, 2009-06-23 14:59:25]

http://codereview.chromium.org/144010/diff/1/2
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/144010/diff/1/2#newcode31
Line 31: uint64 number;
I would use a lazy_instance to avoid race conditions and threading issue.  This
code is designed to run across platforms.

Basically it will involve you making a small wrapper class:

namespace {

class URandomFd() {
 public:
  URandomFd() {
    fd_ = open("/dev/urandom", O_RDONLY);
    CHECK(fd_ >= 0);
  }
  ~URandomFd() {
    close(fd_);
  }
  int fd() { return fd_; }
 private:
  int fd_;
};

base::LazyInstance<URandomFd> g_urandom_fd(base::LINKER_INITIALIZED);

}  // namespace

You would then access it like

int urandom_fd = g_urandom_fd.Pointer()->fd();

[Mike Mammarella [gmail], 2009-06-23 19:46:03]

Do you really think that's necessary here though? It involves additional
synchronization and a pointer dereference on every call, while the only downside
to a race is a one-time leaked file descriptor. But then, this method isn't
actually called super-frequently anyway, so the additional overhead may not be
important.

It seems to me that the code would be both simpler and more efficient without
LazyInstance, but if you feel strongly I could use it.

On 2009/06/23 14:59:25, Dean McNamee wrote:

> http://codereview.chromium.org/144010/diff/1/2
> File base/rand_util_posix.cc (right):
> 
> http://codereview.chromium.org/144010/diff/1/2#newcode31
> Line 31: uint64 number;
> I would use a lazy_instance to avoid race conditions and threading issue. 
This
> code is designed to run across platforms.
> 
> Basically it will involve you making a small wrapper class:
> 
> namespace {
> 
> class URandomFd() {
>  public:
>   URandomFd() {
>     fd_ = open("/dev/urandom", O_RDONLY);
>     CHECK(fd_ >= 0);
>   }
>   ~URandomFd() {
>     close(fd_);
>   }
>   int fd() { return fd_; }
>  private:
>   int fd_;
> };
> 
> base::LazyInstance<URandomFd> g_urandom_fd(base::LINKER_INITIALIZED);
> 
> }  // namespace
> 
> You would then access it like
> 
> int urandom_fd = g_urandom_fd.Pointer()->fd();

[Paweł Hajdan Jr., 2009-06-23 19:50:38]

+agl, who also works on sandboxing I think

This is not the only way to allow access to /dev/urandom fd before getting
sandboxed. We can create a dedicated call (no-op on Win) which would have to be
called before rand_util is used, and would open the fd.

I'm not sure what way is better, but just posting an idea.

[Mike Mammarella [gmail], 2009-06-23 19:55:36]

Actually it was agl who suggested this from among the possible approaches. The
sandbox does need to call RandUint64() before imposing the sandbox actually, to
make sure the descriptor is cached in case no calls have yet been made.

On 2009/06/23 19:50:38, Paweł Hajdan Jr. wrote:
> +agl, who also works on sandboxing I think
> 
> This is not the only way to allow access to /dev/urandom fd before getting
> sandboxed. We can create a dedicated call (no-op on Win) which would have to
be
> called before rand_util is used, and would open the fd.
> 
> I'm not sure what way is better, but just posting an idea.

[Dean McNamee, 2009-06-24 11:41:34]

Yes, LazyInstance is the right thing to do here.  I promise you that the extra
atomic instruction and pointer fetch will not be noticed over the context switch
of calling read().

Thanks

On 2009/06/23 19:55:36, Mike Mammarella wrote:
> Actually it was agl who suggested this from among the possible approaches. The
> sandbox does need to call RandUint64() before imposing the sandbox actually,
to
> make sure the descriptor is cached in case no calls have yet been made.
> 
> On 2009/06/23 19:50:38, Paweł Hajdan Jr. wrote:
> > +agl, who also works on sandboxing I think
> > 
> > This is not the only way to allow access to /dev/urandom fd before getting
> > sandboxed. We can create a dedicated call (no-op on Win) which would have to
> be
> > called before rand_util is used, and would open the fd.
> > 
> > I'm not sure what way is better, but just posting an idea.

[Mike Mammarella [gmail], 2009-06-24 17:34:34]

Updated the patch to use LazyInstance.

[Mark Mentovai, 2009-06-24 17:55:15]

http://codereview.chromium.org/144010/diff/2005/2006
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/144010/diff/2005/2006#newcode9
Line 9: #include <errno.h>
Sort alphabetically - errno.h before fcntl.h.

http://codereview.chromium.org/144010/diff/2005/2006#newcode32
Line 32: inline int fd() { return fd_; }
I'd call this "inline int fd() const" because it doesn't touch the object.

http://codereview.chromium.org/144010/diff/2005/2006#newcode37
Line 37: LazyInstance<URandomFd> g_urandom_fd(LINKER_INITIALIZED);
We only use this in this file, right?  It should be marked static or be in an
anonymous namespace to keep it out of the global symbol table.

http://codereview.chromium.org/144010/diff/2005/2006#newcode42
Line 42: int urandom_fd = g_urandom_fd.Pointer()->fd();
...or, Dean, could we just put the lazy instance right here in this function as
a static?

[Paweł Hajdan Jr., 2009-06-24 17:55:33]

Few nits, but after fixing them LGTM (please wait for Dean's comments). There
should probably be empty lines between methods and before private.

http://codereview.chromium.org/144010/diff/2005/2006
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/144010/diff/2005/2006#newcode21
Line 21: 
nit: should this empty line be here?

http://codereview.chromium.org/144010/diff/2005/2006#newcode27
Line 27: LOG(FATAL) << "Cannot open /dev/urandom: " << errno;
nit: wouldn't CHECK(fd_ >= 0) be clearer? You can << output to it too.

http://codereview.chromium.org/144010/diff/2005/2006#newcode32
Line 32: inline int fd() { return fd_; }
nit: this method can be made const

[Mike Mammarella [gmail], 2009-06-25 01:12:21]

http://codereview.chromium.org/144010/diff/2005/2006
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/144010/diff/2005/2006#newcode9
Line 9: #include <errno.h>
On 2009/06/24 17:55:15, Mark Mentovai wrote:
> Sort alphabetically - errno.h before fcntl.h.

Done.

http://codereview.chromium.org/144010/diff/2005/2006#newcode21
Line 21: 
On 2009/06/24 17:55:33, Paweł Hajdan Jr. wrote:
> nit: should this empty line be here?

Removed.

http://codereview.chromium.org/144010/diff/2005/2006#newcode27
Line 27: LOG(FATAL) << "Cannot open /dev/urandom: " << errno;
On 2009/06/24 17:55:33, Paweł Hajdan Jr. wrote:
> nit: wouldn't CHECK(fd_ >= 0) be clearer? You can << output to it too.

Done.

http://codereview.chromium.org/144010/diff/2005/2006#newcode32
Line 32: inline int fd() { return fd_; }
On 2009/06/24 17:55:15, Mark Mentovai wrote:
> I'd call this "inline int fd() const" because it doesn't touch the object.

Done.

http://codereview.chromium.org/144010/diff/2005/2006#newcode32
Line 32: inline int fd() { return fd_; }
On 2009/06/24 17:55:15, Mark Mentovai wrote:
> I'd call this "inline int fd() const" because it doesn't touch the object.

Done.

http://codereview.chromium.org/144010/diff/2005/2006#newcode37
Line 37: LazyInstance<URandomFd> g_urandom_fd(LINKER_INITIALIZED);
On 2009/06/24 17:55:15, Mark Mentovai wrote:
> We only use this in this file, right?  It should be marked static or be in an
> anonymous namespace to keep it out of the global symbol table.

Done.

http://codereview.chromium.org/144010/diff/2005/2006#newcode42
Line 42: int urandom_fd = g_urandom_fd.Pointer()->fd();
On 2009/06/24 17:55:15, Mark Mentovai wrote:
> ...or, Dean, could we just put the lazy instance right here in this function
as
> a static?

Used anonymous namespace instead.

[Dean McNamee, 2009-06-25 10:08:11]

The static is good in the anonymous namespace, we don't usually and probably
shouldn't put LINKER_INITIALIZED statics within functions, since the behavior of
those are more complicated than simple global .data variables.

So yeah, LGTM with all the nits everyone else has said.

http://codereview.chromium.org/144010/diff/2009/1006
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/144010/diff/2009/1006#newcode32
Line 32: inline int fd() const { return fd_; }
no inline keyword.

[Dean McNamee, 2009-06-25 11:11:25]

Hi,

I was going to commit this and realized you're not in the AUTHORS.

Can you make sure you've signed the CLA, and add yourself to the AUTHORS file? 
See

http://dev.chromium.org/developers/contributing-code

Thanks

On 2009/06/25 10:08:11, Dean McNamee wrote:
> The static is good in the anonymous namespace, we don't usually and probably
> shouldn't put LINKER_INITIALIZED statics within functions, since the behavior
of
> those are more complicated than simple global .data variables.
> 
> So yeah, LGTM with all the nits everyone else has said.
> 
> http://codereview.chromium.org/144010/diff/2009/1006
> File base/rand_util_posix.cc (right):
> 
> http://codereview.chromium.org/144010/diff/2009/1006#newcode32
> Line 32: inline int fd() const { return fd_; }
> no inline keyword.

[Mike Mammarella [gmail], 2009-06-26 00:28:44]

http://codereview.chromium.org/144010/diff/2009/1006
File base/rand_util_posix.cc (right):

http://codereview.chromium.org/144010/diff/2009/1006#newcode32
Line 32: inline int fd() const { return fd_; }
On 2009/06/25 10:08:11, Dean McNamee wrote:
> no inline keyword.

Done.