Sandbox Worker process on the Mac.

Sandbox Worker process on the Mac.

* Add plumbing to allow multiple Sandbox profiles on OS X.
* Separate sandbox_init_wrapper into platform specific files.
* Sandbox Worker process & add plumbing to Sandbox utility process when we bring that up.
* Remove mention of stale bugs in utility process on Mac.

BUG=23582
TEST=Worker process should work.

[jeremy, 2009-10-29 16:54:58]

Mark: this CL still needs some work, but could you take a quick look and see if
the direction looks ok?

[Mark Mentovai, 2009-10-29 17:06:24]

Still a little rough around the edges, but the right approach.

http://codereview.chromium.org/341033/diff/1/7
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/341033/diff/1/7#newcode586
Line 586: // The utility process is sandboxed, with access to one directory this
flag
This should be two sentences, it seems.

http://codereview.chromium.org/341033/diff/1/10
File chrome/common/sandbox_init_wrapper_linux.cc (right):

http://codereview.chromium.org/341033/diff/1/10#newcode12
Line 12: // TODO(port): Does Linux need to do anything here?
indent

http://codereview.chromium.org/341033/diff/1/11
File chrome/common/sandbox_init_wrapper_mac.cc (right):

http://codereview.chromium.org/341033/diff/1/11#newcode15
Line 15: // So in the case of a renderer we bail here.
I don't get it.

I guess I get it now that I've referred to the callers.

Is it a better design to do all sandbox init in this function, but to make the
caller check whether it's the right time to do it?

http://codereview.chromium.org/341033/diff/1/11#newcode17
Line 17: return;
indent indent indent

http://codereview.chromium.org/341033/diff/1/11#newcode21
Line 21: if ((process_type == switches::kWorkerProcess) ||
This logic is starting to get heinous.  Too nested, rechecking the same
conditions repeatedly, etc.  Let's get friendly with some early returns. 
Something like:

if !hasswitch(kNoSandbox)
  return;

FP allowed;
if (ptype == utility)
  allowed = getswitchvalue(kUtilityProcessAllowedDir)
else if (ptype != worker)
  return;

EnableSandbox

http://codereview.chromium.org/341033/diff/1/13
File chrome/common/sandbox_mac.h (right):

http://codereview.chromium.org/341033/diff/1/13#newcode13
Line 13: SANDBOX_TYPE_RENDERER,
.

http://codereview.chromium.org/341033/diff/1/13#newcode14
Line 14: SANDBOX_TYPE_WORKER_PROCESS,
Settle on whether you're using the _PROCESS suffix or not.  (I vote for not.)

http://codereview.chromium.org/341033/diff/1/14
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/341033/diff/1/14#newcode111
Line 111: sandbox_config_filename = @"worker_process";
As before.  I vote for no _process suffixes.

[TVL, 2009-10-29 17:10:26]

http://codereview.chromium.org/341033/diff/1/11
File chrome/common/sandbox_init_wrapper_mac.cc (right):

http://codereview.chromium.org/341033/diff/1/11#newcode29
Line 29: if (process_type == switches::kRendererProcess) {
you can't get inside the above gate to reach here, why include it?

http://codereview.chromium.org/341033/diff/1/11#newcode42
Line 42: }
it would be nice to check for all the other know processes types and noop them
directly, that way you can NOTREACHED() other types to help catch if anyone adds
a new process type.

http://codereview.chromium.org/341033/diff/1/14
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/341033/diff/1/14#newcode88
Line 88: {
wrap the args so this isn't hanging out on its own?

http://codereview.chromium.org/341033/diff/1/14#newcode147
Line 147: withString:allowed_dir_ns];
this is a raw path going to a regex...

http://codereview.chromium.org/341033/diff/1/14#newcode164
Line 164: // surrounding sandbox syntax.
since you now have two paths, should we just write a regex escape api, and move
this also?

[pink (ping after 24hrs), 2009-11-02 17:12:13]

drive-by

http://codereview.chromium.org/341033/diff/2005/32
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/341033/diff/2005/32#newcode486
Line 486: if(process_type != switches::kRendererProcess)
space after |if| before paren.

http://codereview.chromium.org/341033/diff/2005/33
File chrome/browser/utility_process.sb (right):

http://codereview.chromium.org/341033/diff/2005/33#newcode6
Line 6: (version 1)
How about a file-level comment about when this should be used?

http://codereview.chromium.org/341033/diff/2005/34
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/341033/diff/2005/34#newcode70
Line 70: // TODO(port): We should not reach here on linux (crbug.com/22703).
I don't follow. does this mean it's ok for mac to hit this? Then the OS_POSIX
will still cause it to die.

http://codereview.chromium.org/341033/diff/2005/36
File chrome/browser/worker_process.sb (right):

http://codereview.chromium.org/341033/diff/2005/36#newcode6
Line 6: (version 1)
file-level comment needed.

http://codereview.chromium.org/341033/diff/2005/43
File chrome/common/sandbox_init_wrapper_mac.cc (right):

http://codereview.chromium.org/341033/diff/2005/43#newcode20
Line 20: // TODO(port): Look into sandboxing NaCl process.
new code shouldn't have TODO(port). Use TODO(jeremy)

http://codereview.chromium.org/341033/diff/2005/43#newcode31
Line 31: // Failsafe: If you hit an unreached here, is your new process type in
need
indent correctly

http://codereview.chromium.org/341033/diff/2005/43#newcode40
Line 40: using namespace sandbox;
this is forbidden by the C++ style guide (You may not use a using-directive to
make all names from a namespace available.)

http://codereview.chromium.org/341033/diff/2005/46
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/341033/diff/2005/46#newcode88
Line 88: {
move the 2nd param onto a new line so the open brace can go on the same line as
it.

[jeremy, 2009-11-02 17:38:41]

All fixed, ready for another look.

http://codereview.chromium.org/341033/diff/1/7
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/341033/diff/1/7#newcode586
Line 586: // The utility process is sandboxed, with access to one directory this
flag
On 2009/10/29 17:06:24, Mark Mentovai wrote:
> This should be two sentences, it seems.

Done.

http://codereview.chromium.org/341033/diff/1/10
File chrome/common/sandbox_init_wrapper_linux.cc (right):

http://codereview.chromium.org/341033/diff/1/10#newcode12
Line 12: // TODO(port): Does Linux need to do anything here?
On 2009/10/29 17:06:24, Mark Mentovai wrote:
> indent

Done.

http://codereview.chromium.org/341033/diff/1/11
File chrome/common/sandbox_init_wrapper_mac.cc (right):

http://codereview.chromium.org/341033/diff/1/11#newcode15
Line 15: // So in the case of a renderer we bail here.
On 2009/10/29 17:06:24, Mark Mentovai wrote:
> I don't get it.
> 
> I guess I get it now that I've referred to the callers.
> 
> Is it a better design to do all sandbox init in this function, but to make the
> caller check whether it's the right time to do it?

Done.

http://codereview.chromium.org/341033/diff/1/11#newcode21
Line 21: if ((process_type == switches::kWorkerProcess) ||
On 2009/10/29 17:06:24, Mark Mentovai wrote:
> This logic is starting to get heinous.  Too nested, rechecking the same
> conditions repeatedly, etc.  Let's get friendly with some early returns. 
> Something like:
> 
> if !hasswitch(kNoSandbox)
>   return;
> 
> FP allowed;
> if (ptype == utility)
>   allowed = getswitchvalue(kUtilityProcessAllowedDir)
> else if (ptype != worker)
>   return;
> 
> EnableSandbox

Done.

http://codereview.chromium.org/341033/diff/1/11#newcode29
Line 29: if (process_type == switches::kRendererProcess) {
On 2009/10/29 17:10:26, TVL wrote:
> you can't get inside the above gate to reach here, why include it?

Done.

http://codereview.chromium.org/341033/diff/1/11#newcode42
Line 42: }
On 2009/10/29 17:10:26, TVL wrote:
> it would be nice to check for all the other know processes types and noop them
> directly, that way you can NOTREACHED() other types to help catch if anyone
adds
> a new process type.

Done.

http://codereview.chromium.org/341033/diff/1/13
File chrome/common/sandbox_mac.h (right):

http://codereview.chromium.org/341033/diff/1/13#newcode14
Line 14: SANDBOX_TYPE_WORKER_PROCESS,
On 2009/10/29 17:06:24, Mark Mentovai wrote:
> Settle on whether you're using the _PROCESS suffix or not.  (I vote for not.)

Done.

http://codereview.chromium.org/341033/diff/1/14
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/341033/diff/1/14#newcode88
Line 88: {
Sorry, I'm not sure I understand the comment?

http://codereview.chromium.org/341033/diff/1/14#newcode111
Line 111: sandbox_config_filename = @"worker_process";
IMHO it makes some sense to name the sandbox profile files this way but if you
feel strongly about it I'm fine changing it.

http://codereview.chromium.org/341033/diff/1/14#newcode147
Line 147: withString:allowed_dir_ns];
Yes it is, see the TODO in the comment in the header.  IMHO it's OK to leave
this like this for now for 2 reasons:
1. This is only used in the utility process which is not enabled in the beta
release.
2. The string that's sent to this for the utility process is a temporary
directory which has filenames that match this pattern.

I can file a bug to do this if you feel strongly?

http://codereview.chromium.org/341033/diff/1/14#newcode164
Line 164: // surrounding sandbox syntax.
See previous comment.

http://codereview.chromium.org/341033/diff/2005/33
File chrome/browser/utility_process.sb (right):

http://codereview.chromium.org/341033/diff/2005/33#newcode6
Line 6: (version 1)
On 2009/11/02 17:12:14, pink wrote:
> How about a file-level comment about when this should be used?

Done.

http://codereview.chromium.org/341033/diff/2005/34
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/341033/diff/2005/34#newcode70
Line 70: // TODO(port): We should not reach here on linux (crbug.com/22703).
The Mac bug that was referenced in the comment previously is fixed so I removed
it.  I've updated the comment to point to the bug that tracks re-enabling this.

http://codereview.chromium.org/341033/diff/2005/36
File chrome/browser/worker_process.sb (right):

http://codereview.chromium.org/341033/diff/2005/36#newcode6
Line 6: (version 1)
On 2009/11/02 17:12:14, pink wrote:
> file-level comment needed.

Done.

http://codereview.chromium.org/341033/diff/2005/43
File chrome/common/sandbox_init_wrapper_mac.cc (right):

http://codereview.chromium.org/341033/diff/2005/43#newcode20
Line 20: // TODO(port): Look into sandboxing NaCl process.
On 2009/11/02 17:12:14, pink wrote:
> new code shouldn't have TODO(port). Use TODO(jeremy)

Done.

http://codereview.chromium.org/341033/diff/2005/43#newcode31
Line 31: // Failsafe: If you hit an unreached here, is your new process type in
need
On 2009/11/02 17:12:14, pink wrote:
> indent correctly

Done.

http://codereview.chromium.org/341033/diff/2005/43#newcode40
Line 40: using namespace sandbox;
On 2009/11/02 17:12:14, pink wrote:
> this is forbidden by the C++ style guide (You may not use a using-directive to
> make all names from a namespace available.)

Done.

http://codereview.chromium.org/341033/diff/2005/46
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/341033/diff/2005/46#newcode88
Line 88: {
On 2009/11/02 17:12:14, pink wrote:
> move the 2nd param onto a new line so the open brace can go on the same line
as
> it.

Done.

[TVL, 2009-11-02 17:54:17]

http://codereview.chromium.org/341033/diff/1/14
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/341033/diff/1/14#newcode88
Line 88: {
On 2009/11/02 17:38:41, jeremy wrote:
> Sorry, I'm not sure I understand the comment?

bool EnableSandbox(SandboxProcessType sandbox_type,
                   const FilePath& allowed_dir) {

like that.

http://codereview.chromium.org/341033/diff/1/14#newcode147
Line 147: withString:allowed_dir_ns];
On 2009/11/02 17:38:41, jeremy wrote:
> Yes it is, see the TODO in the comment in the header.  IMHO it's OK to leave
> this like this for now for 2 reasons:
> 1. This is only used in the utility process which is not enabled in the beta
> release.
> 2. The string that's sent to this for the utility process is a temporary
> directory which has filenames that match this pattern.
> 
> I can file a bug to do this if you feel strongly?

Are you sure it isn't used, I thought themes used this to decode/encode the
images so we don't have a security risk using them within the browser?

[jeremy, 2009-11-02 18:28:19]

All fixed + bug filed for regex issues - crbug.com/26492 

Ready for another look.

http://codereview.chromium.org/341033/diff/1/14
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/341033/diff/1/14#newcode147
Line 147: withString:allowed_dir_ns];
It's all in-process now - see use_utility_process in
sandbox_extension_unpacker.cc - crbug.com/23837

[pink (ping after 24hrs), 2009-11-03 19:13:41]

i think there just needs to be more documentation on what is a worker, what is a
utility process, and how those are different from the normal renderer config.
We're throwing around all these terms, but there's no definition of them
anywhere.

http://codereview.chromium.org/341033/diff/3009/4041
File chrome/browser/utility_process.sb (right):

http://codereview.chromium.org/341033/diff/3009/4041#newcode6
Line 6: ; This is the Sandbox configuration file used for safeguarding the
utility
what's the utility process used for? Why does it need its own config file that's
different from renderer?

http://codereview.chromium.org/341033/diff/3009/4042
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/341033/diff/3009/4042#newcode71
Line 71: // (crbug.com/23837) covers enabling this on Linux/OS X.
I'm still confused. There's a bug for linux, and another bug for linux & mac?

http://codereview.chromium.org/341033/diff/3009/4044
File chrome/browser/worker_process.sb (right):

http://codereview.chromium.org/341033/diff/3009/4044#newcode6
Line 6: ; This is the Sandbox configuration file used for safeguarding the
worker
what are worker processes for?

http://codereview.chromium.org/341033/diff/3009/4053
File chrome/common/sandbox_mac.h (right):

http://codereview.chromium.org/341033/diff/3009/4053#newcode13
Line 13: SANDBOX_TYPE_RENDERER,
what's the difference between these 3 enums?

[TVL, 2009-11-04 00:17:38]

lg

http://codereview.chromium.org/341033/diff/3009/4041
File chrome/browser/utility_process.sb (right):

http://codereview.chromium.org/341033/diff/3009/4041#newcode36
Line 36: (allow file* (regex #"^DIR_TO_ALLOW_ACCESS"))
the wildcard here scares me a little, any harm in listing the directory more
then once?

[Mark Mentovai, 2009-11-04 02:21:19]

The comments aren't inline because Rietveld was read-only when I began.

chrome_dll_main.cc:484
Lowercase "s" sandbox.

utility_process.sb:17
This comment is out of place in this file.  Why would full-page zoom controls
matter in the utility process?

worker_process.sb:18
Same.

chrome.gyp:2602
Please rename this to worker.sb to parallel renderer.sb.

chrome.gyp:2603
Same.

sandbox_init_wrapper.h:36
Since you're changing the return type from void to bool, you need to say what
the return values mean.  It's still not clear to me.  What does it return if the
process isn't of the type that gets sandboxed, for example?

sandbox_init_wrapper.h:53
Same.

sandbox_init_wrapper_linux.cc:13
This comment belongs above the return.

sandbox_init_wrapper_mac.mm:33
Put some kind of return (true or false?) after the NOTREACHED().

sandbox_init_wrapper_mac.mm:36
Don't declare utility_process_sandbox until you need it.  Specifically, you
don't need it until you're in the conditional that follows.  In fact, you only
use the value once, so I don't see the value in this being a variable.

sandbox_init_wrapper_mac.mm:37
Same.

sandbox_init_wrapper_mac.mm:40
These using declarations import names that are only used one time each.  I don't
think it's worth it.  Just write the qualified names out fully when you need
them.

sandbox_init_wrapper_mac.mm:45
Warmup is a noun or adjective.  Warm up (two words) is the verb.  Use two words.
 Also, Sandbox gets a lowercase s.

sandbox_init_wrapper_mac.mm:53
This should be an else block (|else if (process_type ==
switches::kRendererProcess)|).

sandbox_init_wrapper_mac.mm:60
Sandbox is not a proper name here, don't capitalize it.

sandbox_init_wrapper_mac.mm:60
To tell you the truth, the whole structure of this function seems wrong.  It
effectively goes through all of the work it needs to do twice.  My proposed
implementation is less than half the length - even when you sprinkle your
comments back in, it'll remain much more readable.  Watch:

  if (command_line.HasSwitch(switches::kNoSandbox))
    return true;

  sandbox::SandboxProcessType sandbox_process_type;
  FilePath allowed_dir;
  if (process_type.empty()) {
    return true;
  } if (process_type == switches::kRendererProcess) {
    sandbox_process_type = sandbox::SANDBOX_TYPE_RENDERER;
  } else if (process_type == switches::kWorkerProcess) {
    sandbox_process_type = sandbox::SANDBOX_TYPE_WORKER
  } else if (process_type == switches::kUtilityProcess) {
    sandbox_process_type = sandbox::SANDBOX_TYPE_UTILITY
    allowed_dir = FilePath::FromWStringHack(
        command_line.GetSwitchValue(switches::kUtilityProcessAllowedDir));
  } else if (process_type == switches::kPluginProcess ||
             process_type == switches::kProfileImportProcess ||
             process_type == switches::kNaClProcess) {
    return true;
  } else {
    NOTREACHED();
    return;
  }

  return sandbox::EnableSandbox(sandbox_process_type, allowed_dir);

sandbox_mac.h:1
It seems like this file should be merged into sandbox_init_wrapper_mac.h
somehow, but you can save that for future clean-up.  (Actually, we should
probably just have sandbox.h, sandbox_platform.h, etc., and not have the
sandbox_init names.)

sandbox_mac.mm:111
Rename to @"worker".  I know I mentioned this in the preliminary comments I gave
you.

sandbox_mac.mm:112
Same.

sandbox_mac.mm:91
  DCHECK_EQ(sandbox_type == SANDBOX_TYPE_UTILITY, !allowed_dir.empty())
      << "Your ad here! 212-555-1212"

renderer_main_platform_delegate_mac.mm:52
Line switches::kRendererProcess up with *parsed_command_line above it.

[jeremy, 2009-11-04 10:24:49]

All fixed, lgty?

cpu: could you please take a quick look over sandbox_init_wrapper_win.cc and see
if it looks ok to you?

http://codereview.chromium.org/341033/diff/3009/4041
File chrome/browser/utility_process.sb (right):

http://codereview.chromium.org/341033/diff/3009/4041#newcode6
Line 6: ; This is the Sandbox configuration file used for safeguarding the
utility
I've expanded this comment considerable, please let me know if there's anything
else you'd like to see mentioned here.

http://codereview.chromium.org/341033/diff/3009/4041#newcode36
Line 36: (allow file* (regex #"^DIR_TO_ALLOW_ACCESS"))
I've modified this to use the same exact syntax as the configuration files in
/usr/share/sandbox .

http://codereview.chromium.org/341033/diff/3009/4042
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/341033/diff/3009/4042#newcode71
Line 71: // (crbug.com/23837) covers enabling this on Linux/OS X.
crbug.com/22703 covers a bug that prevents enabling this on Linux.

crbug.com/23837 is a common tracking bug for re-enabling this on Linux/Mac -
there used to be a blocking bug on Mac as well but that was fixed.

http://codereview.chromium.org/341033/diff/3009/4044
File chrome/browser/worker_process.sb (right):

http://codereview.chromium.org/341033/diff/3009/4044#newcode6
Line 6: ; This is the Sandbox configuration file used for safeguarding the
worker
On 2009/11/03 19:13:41, pink wrote:
> what are worker processes for?

Done.

http://codereview.chromium.org/341033/diff/3009/4053
File chrome/common/sandbox_mac.h (right):

http://codereview.chromium.org/341033/diff/3009/4053#newcode13
Line 13: SANDBOX_TYPE_RENDERER,
On 2009/11/03 19:13:41, pink wrote:
> what's the difference between these 3 enums?

Done.

[cpu_(ooo_6.6-7.5), 2009-11-04 21:12:27]

looked at the 3 windows files, they lgtm.