Share the zygote's fopen overrides with nacl_helper.

Share the zygote's fopen overrides with nacl_helper.

This will allow ChromeOS's version of NSS to initialize inside of nacl_helper
without killing the process, which in turn allows validation caching to be
enabled on ChromeOS.

BUG= https://code.google.com/p/chromium/issues/detail?id=134538
TEST= none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146498

[Sergey Ulanov, 2012-07-10 22:42:04]

looks mostly good, I just some style nits

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
File content/zygote/zygote_libc_override_linux.cc (right):

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.cc:34: // being the motivating example
- it needs to read /etc/localtime). We need to
this comment is about localtime(), so it should be kept in zygote_main_linux.cc

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
File content/zygote/zygote_libc_override_linux.h (right):

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:5: #ifndef
CONTENT_ZYGOTE_ZYGOTE_LIBC_OVERRIDE_LINUX_H_
not sure if this is the best location for this file. Might be better to put it
in content/common and remove zygote from the name (it's not zygote specific
now).

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:13: extern bool
g_am_zygote_or_renderer;
instead of exposing this variable it's better to have a function that would set
this flag (e.g. InitUrandomLibcOverrides()) to make sure we don't reset it flag
to false by accident.

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:15: #endif
// !defined (CHROMIUM_SELINUX)

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:17: }
// namespace content

[jam, 2012-07-10 23:57:36]

the checkdeps errors are because chrome can't reach into files inside content.

since this is a shared static function, and it doesn't depend on anything else
in content, would it be possible to put this in base or sandbox?

[jam, 2012-07-10 23:59:40]

On 2012/07/10 23:57:36, John Abd-El-Malek wrote:
> the checkdeps errors are because chrome can't reach into files inside content.
> 
> since this is a shared static function, and it doesn't depend on anything else
> in content, would it be possible to put this in base or sandbox?

if that's not a good fit, you can define the function inside
content/public/common/sandbox_init.h (we try to minimize number of headers in
content/public

[Nick Bray (chromium), 2012-07-11 00:06:11]

jam: thanks for the feedback, I'll need to poke around for a bit and figure out
what location has the strongest affinity.

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
File content/zygote/zygote_libc_override_linux.cc (right):

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.cc:34: // being the motivating example
- it needs to read /etc/localtime). We need to
On 2012/07/10 22:42:04, sergeyu wrote:
> this comment is about localtime(), so it should be kept in
zygote_main_linux.cc

Done.

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
File content/zygote/zygote_libc_override_linux.h (right):

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:5: #ifndef
CONTENT_ZYGOTE_ZYGOTE_LIBC_OVERRIDE_LINUX_H_
On 2012/07/10 22:42:04, sergeyu wrote:
> not sure if this is the best location for this file. Might be better to put it
> in content/common and remove zygote from the name (it's not zygote specific
> now).

Done.

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:13: extern bool
g_am_zygote_or_renderer;
On 2012/07/10 22:42:04, sergeyu wrote:
> instead of exposing this variable it's better to have a function that would
set
> this flag (e.g. InitUrandomLibcOverrides()) to make sure we don't reset it
flag
> to false by accident.

Done.

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:15: #endif
On 2012/07/10 22:42:04, sergeyu wrote:
> // !defined (CHROMIUM_SELINUX)

Done.

https://chromiumcodereview.appspot.com/10736017/diff/1/content/zygote/zygote_...
content/zygote/zygote_libc_override_linux.h:17: }
On 2012/07/10 22:42:04, sergeyu wrote:
> // namespace content

Done.

[jam, 2012-07-11 21:35:16]

http://codereview.chromium.org/10736017/diff/7002/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/10736017/diff/7002/content/content_browser.gyp...
content/content_browser.gypi:774:
'../sandbox/linux/services/libc_urandom_override.h',
these should be listed in the sandbox gyp, and content_browser can depend on it

[Nick Bray, 2012-07-11 22:46:04]

Thanks to everyone for help sorting out the style and organizational issues. 
Time for the review.

jln as main reviewer / linux sandbox OWNER.
jam as content OWNER.

[jam, 2012-07-12 18:40:30]

content lgtm

[jln (very slow on Chromium), 2012-07-12 19:19:05]

https://chromiumcodereview.appspot.com/10736017/diff/7003/sandbox/linux/servi...
File sandbox/linux/services/libc_urandom_override.cc (right):

https://chromiumcodereview.appspot.com/10736017/diff/7003/sandbox/linux/servi...
sandbox/linux/services/libc_urandom_override.cc:27: base::RandUint64();
Should this be base::GetUrandomFD() instead ? I don't think you'd need a full
warm up in the general case. If you do, shouldn't it be in NaCl ?

Perhaps with a comment along the lines of "Make sure /dev/urandom is open now" ?

[Nick Bray (chromium), 2012-07-12 21:42:48]

PTAL

https://chromiumcodereview.appspot.com/10736017/diff/7003/sandbox/linux/servi...
File sandbox/linux/services/libc_urandom_override.cc (right):

https://chromiumcodereview.appspot.com/10736017/diff/7003/sandbox/linux/servi...
sandbox/linux/services/libc_urandom_override.cc:27: base::RandUint64();
I did the change you suggested.

I was conflicted about adding this code in the first place, however.  Both
nacl_helper and the zygote make sure /dev/urandom is open anyways.  I put it
here to make the dependancy explicit, even if it is redundant.  Thoughts?

I simplified the call in nacl_helper, too.  Should I also simplify the zygote,
or is there some reason it needs to do a "full warm up"?  There's no
documentation either way.

On 2012/07/12 19:19:05, Julien Tinnes wrote:
> Should this be base::GetUrandomFD() instead ? I don't think you'd need a full
> warm up in the general case. If you do, shouldn't it be in NaCl ?
> 
> Perhaps with a comment along the lines of "Make sure /dev/urandom is open now"
?

[jln (very slow on Chromium), 2012-07-13 00:06:24]

LGTM

https://chromiumcodereview.appspot.com/10736017/diff/7003/sandbox/linux/servi...
File sandbox/linux/services/libc_urandom_override.cc (right):

https://chromiumcodereview.appspot.com/10736017/diff/7003/sandbox/linux/servi...
sandbox/linux/services/libc_urandom_override.cc:27: base::RandUint64();
In practice it won't matter because the "full warm up" in that case does nothing
more.

In general it's good to exercise APIs before you enter any sandboxing mode,
which is probably why a full RandUint64() was in the Zygote.

On 2012/07/12 21:42:49, Nick Bray (chromium) wrote:
> I did the change you suggested.
> 
> I was conflicted about adding this code in the first place, however.  Both
> nacl_helper and the zygote make sure /dev/urandom is open anyways.  I put it
> here to make the dependancy explicit, even if it is redundant.  Thoughts?
> 
> I simplified the call in nacl_helper, too.  Should I also simplify the zygote,
> or is there some reason it needs to do a "full warm up"?  There's no
> documentation either way.
> 
> On 2012/07/12 19:19:05, Julien Tinnes wrote:
> > Should this be base::GetUrandomFD() instead ? I don't think you'd need a
full
> > warm up in the general case. If you do, shouldn't it be in NaCl ?
> > 
> > Perhaps with a comment along the lines of "Make sure /dev/urandom is open
now"
> ?
>

[commit-bot: I haz the power, 2012-07-13 00:28:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/10736017/6004

[commit-bot: I haz the power, 2012-07-13 01:28:09]

Change committed as 146498