Clear environment variables for nacl_helper

Clear environment variables for nacl_helper

BUG=358413
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275259

[elijahtaylor1, 2014-05-30 00:01:48]

PTAL

[jln (very slow on Chromium), 2014-06-02 21:31:46]

Looks good in general.

What is expect to use NACL_DANGEROUS_NACL_HELPER_ENV_PASSTHROUGH and how?

Currently, everything works with it just being empty?

https://codereview.chromium.org/308073002/diff/1/base/process/process_util_un...
File base/process/process_util_unittest.cc (right):

https://codereview.chromium.org/308073002/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:654: }
Could you add a test with clear_environ set to true?

I suspect that EXPECT_EQ("\n", TestLaunchProces(., true /* clear_environ */,
..); should work?

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:418: for (size_t i = 0; i <
pass_through_vars.size(); ++i) {
Why not use a vector::const_iterator instead?

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.h (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.h:51: static void
AddPassthroughEnvToOptions(base::LaunchOptions& options);
Style: make this a pointer instead as it is non-const.

Also could you add some basic documentation?

Shouldn't it be private?

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:1: 
Style: no empty line here.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:6: #include
"components/nacl/zygote/nacl_fork_delegate_linux.h"
Nit: empty line after including the interface that you're testing.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:11: 
Nit: add an anonymous namespace.

[Mark Seaborn, 2014-06-02 23:10:29]

https://codereview.chromium.org/308073002/diff/1/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/308073002/diff/1/base/process/launch.h#newcode91
base/process/launch.h:91: // Set/unset environment variables. Empty (the
default) means to inherit
Maybe clarify this comment to say that environ is applied on top of the
process's current environment?  Otherwise it suggests that 'environ' being empty
is treated as a special case.

https://codereview.chromium.org/308073002/diff/1/base/process/process_util_un...
File base/process/process_util_unittest.cc (right):

https://codereview.chromium.org/308073002/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:573: args.push_back("echo $BASE_TEST");
Could you add a separate test that runs /usr/bin/env to check that no env vars
are set?

The current test doesn't actually test that BASE_TEST is absent.  It would pass
if BASE_TEST were set to the empty string.

It also doesn't check that other env vars aren't getting tacked onto the
environment somewhere.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:48: const char
kNaClDangerousNaClHelperEnvPassthrough[] =
Could you add a comment saying what this is for?

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:49:
"NACL_DANGEROUS_NACL_HELPER_ENV_PASSTHROUGH";
Maybe just "NACL_ENV_PASSTHROUGH", otherwise it's a bit long?  It's not very
dangerous so the "DANGEROUS" prefix seems unnecessarily scary. :-)

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:250: options.clear_environ =
true;
Maybe add a comment like: To avoid information leaks in Non-SFI mode, ensure
that environment variables never appear in the address space of the child
process.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:416:
base::SplitStringAlongWhitespace(pass_through_string, &pass_through_vars);
Nit: Could you split on commas instead?  Spaces are unusual for env vars and
it's sometimes tricky to get shells (and makefiles) to preserve them.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:417: std::string temp;
Nit: could go inside the following loop

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:1: 
Nit: remove empty line at start

[elijahtaylor1, 2014-06-03 20:47:55]

https://codereview.chromium.org/308073002/diff/1/base/process/launch.h
File base/process/launch.h (right):

https://codereview.chromium.org/308073002/diff/1/base/process/launch.h#newcode91
base/process/launch.h:91: // Set/unset environment variables. Empty (the
default) means to inherit
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Maybe clarify this comment to say that environ is applied on top of the
> process's current environment?  Otherwise it suggests that 'environ' being
empty
> is treated as a special case.

Done.

https://codereview.chromium.org/308073002/diff/1/base/process/process_util_un...
File base/process/process_util_unittest.cc (right):

https://codereview.chromium.org/308073002/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:573: args.push_back("echo $BASE_TEST");
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Could you add a separate test that runs /usr/bin/env to check that no env vars
> are set?
> 
> The current test doesn't actually test that BASE_TEST is absent.  It would
pass
> if BASE_TEST were set to the empty string.
> 
> It also doesn't check that other env vars aren't getting tacked onto the
> environment somewhere.

Done.

https://codereview.chromium.org/308073002/diff/1/base/process/process_util_un...
base/process/process_util_unittest.cc:654: }
On 2014/06/02 21:31:47, jln wrote:
> Could you add a test with clear_environ set to true?
> 
> I suspect that EXPECT_EQ("\n", TestLaunchProces(., true /* clear_environ */,
> ..); should work?

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:48: const char
kNaClDangerousNaClHelperEnvPassthrough[] =
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Could you add a comment saying what this is for?

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:49:
"NACL_DANGEROUS_NACL_HELPER_ENV_PASSTHROUGH";
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Maybe just "NACL_ENV_PASSTHROUGH", otherwise it's a bit long?  It's not very
> dangerous so the "DANGEROUS" prefix seems unnecessarily scary. :-)

I added "DANGEROUS" because I was anticipating a request from *you* :)

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:250: options.clear_environ =
true;
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Maybe add a comment like: To avoid information leaks in Non-SFI mode, ensure
> that environment variables never appear in the address space of the child
> process.

Done, but modified this comment slightly.  It's not technically true that they
*never* appear in the address space of NaCl Helper, since fork() inherits the
environment.  We just immediately overwrite it.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:416:
base::SplitStringAlongWhitespace(pass_through_string, &pass_through_vars);
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Nit: Could you split on commas instead?  Spaces are unusual for env vars and
> it's sometimes tricky to get shells (and makefiles) to preserve them.

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:417: std::string temp;
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Nit: could go inside the following loop

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:418: for (size_t i = 0; i <
pass_through_vars.size(); ++i) {
On 2014/06/02 21:31:47, jln wrote:
> Why not use a vector::const_iterator instead?

verbosity/clarity.  I'll leave as is unless you feel strongly about this.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.h (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.h:51: static void
AddPassthroughEnvToOptions(base::LaunchOptions& options);
On 2014/06/02 21:31:47, jln wrote:
> Style: make this a pointer instead as it is non-const.
Done.

> 
> Also could you add some basic documentation?
Added to the declaration of NACL_ENV_PASSTHROUGH

> 
> Shouldn't it be private?
Sure, if you don't mind a friend declaration for testing

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc (right):

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:1: 
On 2014/06/02 21:31:47, jln wrote:
> Style: no empty line here.

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:1: 
On 2014/06/02 23:10:29, Mark Seaborn wrote:
> Nit: remove empty line at start

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:6: #include
"components/nacl/zygote/nacl_fork_delegate_linux.h"
On 2014/06/02 21:31:47, jln wrote:
> Nit: empty line after including the interface that you're testing.

Done.

https://codereview.chromium.org/308073002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:11: 
On 2014/06/02 21:31:47, jln wrote:
> Nit: add an anonymous namespace.

I don't know if that's possible since we need to make this test class a friend
of NaClForkDelegate.  What's the reason you want it in a namespace?  I thought
we wouldn't care so much since this is just compiled into components_unittests. 
I could put it in the nacl namespace if it's still a concern

[jln (very slow on Chromium), 2014-06-03 22:58:13]

lgtm, but please change the signature of AddPassthroughEnvToOptions() to take a
pointer.

https://chromiumcodereview.appspot.com/308073002/diff/1/components/nacl/zygot...
File components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc (right):

https://chromiumcodereview.appspot.com/308073002/diff/1/components/nacl/zygot...
components/nacl/zygote/nacl_fork_delegate_linux_unittest.cc:11: 
On 2014/06/03 20:47:55, elijahtaylor1 wrote:
> On 2014/06/02 21:31:47, jln wrote:
> > Nit: add an anonymous namespace.
> 
> I don't know if that's possible since we need to make this test class a friend
> of NaClForkDelegate.  What's the reason you want it in a namespace?  I thought
> we wouldn't care so much since this is just compiled into
components_unittests. 
> I could put it in the nacl namespace if it's still a concern

Yeah, probably a good idea to have it in the nacl namespace at least. It will
save you nacl:: below as well.

https://chromiumcodereview.appspot.com/308073002/diff/40001/components/nacl/z...
File components/nacl/zygote/nacl_fork_delegate_linux.h (right):

https://chromiumcodereview.appspot.com/308073002/diff/40001/components/nacl/z...
components/nacl/zygote/nacl_fork_delegate_linux.h:55: static void
AddPassthroughEnvToOptions(base::LaunchOptions& options);
Style: this really should be a pointer.

[elijahtaylor1, 2014-06-03 23:22:11]

+mark@chromium.org for base/ OWNERS

https://chromiumcodereview.appspot.com/308073002/diff/40001/components/nacl/z...
File components/nacl/zygote/nacl_fork_delegate_linux.h (right):

https://chromiumcodereview.appspot.com/308073002/diff/40001/components/nacl/z...
components/nacl/zygote/nacl_fork_delegate_linux.h:55: static void
AddPassthroughEnvToOptions(base::LaunchOptions& options);
On 2014/06/03 22:58:13, jln wrote:
> Style: this really should be a pointer.

In fairness, I *really* meant to do it, but didn't :)

Done.

[Mark Mentovai, 2014-06-04 02:16:34]

LGTM in base.

[Mark Seaborn, 2014-06-04 15:16:50]

LGTM

https://chromiumcodereview.appspot.com/308073002/diff/60001/components/nacl/z...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://chromiumcodereview.appspot.com/308073002/diff/60001/components/nacl/z...
components/nacl/zygote/nacl_fork_delegate_linux.cc:50: //
NACL_ENV_PASSTHROUGH="PATH PWD" would pass both $PATH and $PWD to the child
This should be "PATH,CWD".

[elijahtaylor1, 2014-06-04 16:36:36]

https://codereview.chromium.org/308073002/diff/60001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/308073002/diff/60001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:50: //
NACL_ENV_PASSTHROUGH="PATH PWD" would pass both $PATH and $PWD to the child
On 2014/06/04 15:16:50, Mark Seaborn wrote:
> This should be "PATH,CWD".

Done.

[elijahtaylor1, 2014-06-04 16:36:50]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-06-04 16:38:09]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/308073002/80001

[commit-bot: I haz the power, 2014-06-04 19:33:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-04 19:37:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 19:37:38]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[elijahtaylor1, 2014-06-04 20:10:32]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-06-04 20:13:07]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/308073002/80001

[commit-bot: I haz the power, 2014-06-04 20:22:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-04 20:27:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 20:27:55]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[elijahtaylor1, 2014-06-05 18:21:50]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-06-05 18:22:57]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/308073002/10...

[commit-bot: I haz the power, 2014-06-05 22:16:19]

Change committed as 275259