Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2921733002:
Add flags for v2 sandbox to Chrome and Helper executable. (Closed)
DescriptionAdd flags for v2 sandbox to Chrome and Helper executable.
This adds the flags for the v2 sandbox to Chrome and the Helper
executable. The helper executable has its own declarations of the flags
to minimize the static linking size.
BUG=689306
Review-Url: https://codereview.chromium.org/2921733002
Cr-Commit-Position: refs/heads/master@{#476865}
Committed: https://chromium.googlesource.com/chromium/src/+/6276ea0023c2fdc01609622f5d6b4820d37e04c9
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address review feedback #
Total comments: 4
Patch Set 3 : Address nits #
Messages
Total messages: 34 (18 generated)
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kerrnel@chromium.org changed reviewers: + rsesek@chromium.org
PTAL. Thanks, Greg
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = "--v2-sandbox-enabled"; constexpr? https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = "--v2-sandbox-enabled"; Since "enabled" is so overloaded for features, I think maybe "--v2-sandbox-did-exec" or something more explicit? Also, I wonder if it'd be better to negate this? I.e., the browser launches with --v2-sandbox-exec and then the flag is removed from the argv before execv. Not sure it makes a difference, other than all running child processes then won't have the --v2-sandbox-exec flag in the args.
https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = "--v2-sandbox-enabled"; On 2017/06/01 22:07:49, Robert Sesek wrote: > Since "enabled" is so overloaded for features, I think maybe > "--v2-sandbox-did-exec" or something more explicit? > > Also, I wonder if it'd be better to negate this? I.e., the browser launches with > --v2-sandbox-exec and then the flag is removed from the argv before execv. Not > sure it makes a difference, other than all running child processes then won't > have the --v2-sandbox-exec flag in the args. The reason I make it a positive for now is because we're transitioning one process type at a time, so this means all processes will have some --not-v2-sandbox flag. I think we should use a positive flag and then flip that around once the migration is complete?
https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = "--v2-sandbox-enabled"; On 2017/06/01 22:10:40, Greg K wrote: > On 2017/06/01 22:07:49, Robert Sesek wrote: > > Since "enabled" is so overloaded for features, I think maybe > > "--v2-sandbox-did-exec" or something more explicit? > > > > Also, I wonder if it'd be better to negate this? I.e., the browser launches > with > > --v2-sandbox-exec and then the flag is removed from the argv before execv. Not > > sure it makes a difference, other than all running child processes then won't > > have the --v2-sandbox-exec flag in the args. > > The reason I make it a positive for now is because we're transitioning one > process type at a time, so this means all processes will have some > --not-v2-sandbox flag. I think we should use a positive flag and then flip that > around once the migration is complete? That's true, unless you propagated the --v2-sandbox flag rather than dropping it in the new process. It's fine to keep it positive until the transition is complete. Maybe leave a TODO for this?
https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = "--v2-sandbox-enabled"; On 2017/06/01 22:12:28, Robert Sesek wrote: > On 2017/06/01 22:10:40, Greg K wrote: > > On 2017/06/01 22:07:49, Robert Sesek wrote: > > > Since "enabled" is so overloaded for features, I think maybe > > > "--v2-sandbox-did-exec" or something more explicit? > > > > > > Also, I wonder if it'd be better to negate this? I.e., the browser launches > > with > > > --v2-sandbox-exec and then the flag is removed from the argv before execv. > Not > > > sure it makes a difference, other than all running child processes then > won't > > > have the --v2-sandbox-exec flag in the args. > > > > The reason I make it a positive for now is because we're transitioning one > > process type at a time, so this means all processes will have some > > --not-v2-sandbox flag. I think we should use a positive flag and then flip > that > > around once the migration is complete? > > That's true, unless you propagated the --v2-sandbox flag rather than dropping it > in the new process. It's fine to keep it positive until the transition is > complete. Maybe leave a TODO for this? Yes. I don't want to re-add the new flag because it gets confusing as it can cause an endless re-exec cycle, unless it's add just before entering ChromeMain. I'll add a TODO, thanks.
https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = "--v2-sandbox-enabled"; On 2017/06/01 22:07:49, Robert Sesek wrote: > constexpr? I don't actually know a better way to do this. The execv() API takes a "char* const" of all things, not even a const char*, so if this is constexpr, the only way to pass it along is to throw away the const qualifier. Do you know a better way?
https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = "--v2-sandbox-enabled"; On 2017/06/01 22:23:56, Greg K wrote: > On 2017/06/01 22:07:49, Robert Sesek wrote: > > constexpr? > > I don't actually know a better way to do this. The execv() API takes a "char* > const" of all things, not even a const char*, so if this is constexpr, the only > way to pass it along is to throw away the const qualifier. > > Do you know a better way? For what it's worth, these are document as constants but kept non-const in declaration to avoid breaking old code. I am fine using a const_cast<> in this case: http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/01 22:26:11, Greg K wrote: > https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... > File chrome/app/chrome_exe_main_mac.cc (right): > > https://codereview.chromium.org/2921733002/diff/1/chrome/app/chrome_exe_main_... > chrome/app/chrome_exe_main_mac.cc:39: char v2_sandbox_enabled_arg[] = > "--v2-sandbox-enabled"; > On 2017/06/01 22:23:56, Greg K wrote: > > On 2017/06/01 22:07:49, Robert Sesek wrote: > > > constexpr? > > > > I don't actually know a better way to do this. The execv() API takes a "char* > > const" of all things, not even a const char*, so if this is constexpr, the > only > > way to pass it along is to throw away the const qualifier. > > > > Do you know a better way? > > For what it's worth, these are document as constants but kept non-const in > declaration to avoid breaking old code. I am fine using a const_cast<> in this > case: http://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html New patchset uploaded.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
kerrnel@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content/ Thanks, Greg
kerrnel@chromium.org changed reviewers: + thakis@chromium.org
thakis@chromium.org: Please review changes in chrome/ Thanks, Greg
stampy lgtm based on rsesek's review
Thanks, LGTM with nits. https://codereview.chromium.org/2921733002/diff/20001/chrome/app/chrome_exe_m... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/20001/chrome/app/chrome_exe_m... chrome/app/chrome_exe_main_mac.cc:38: // should already be enabled. Can you repeat this comment in content_switches.cc? It's hard to understand why both flags are needed there at the moment, without digging around. https://codereview.chromium.org/2921733002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2921733002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:1088: // Indicate that the V2 sandbox has been enabled. nit: Add blank line before.
The CQ bit was checked by kerrnel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kerrnel@chromium.org
https://codereview.chromium.org/2921733002/diff/20001/chrome/app/chrome_exe_m... File chrome/app/chrome_exe_main_mac.cc (right): https://codereview.chromium.org/2921733002/diff/20001/chrome/app/chrome_exe_m... chrome/app/chrome_exe_main_mac.cc:38: // should already be enabled. On 2017/06/02 20:50:41, Charlie Reis wrote: > Can you repeat this comment in content_switches.cc? It's hard to understand why > both flags are needed there at the moment, without digging around. Done. https://codereview.chromium.org/2921733002/diff/20001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2921733002/diff/20001/content/public/common/c... content/public/common/content_switches.cc:1088: // Indicate that the V2 sandbox has been enabled. On 2017/06/02 20:50:42, Charlie Reis wrote: > nit: Add blank line before. Done.
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, creis@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2921733002/#ps40001 (title: "Address nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496447948015370,
"parent_rev": "cd4a66f23632d80f36aeb301d118a33360681e8b", "commit_rev":
"6276ea0023c2fdc01609622f5d6b4820d37e04c9"}
Message was sent while issue was closed.
Description was changed from ========== Add flags for v2 sandbox to Chrome and Helper executable. This adds the flags for the v2 sandbox to Chrome and the Helper executable. The helper executable has its own declarations of the flags to minimize the static linking size. BUG=689306 ========== to ========== Add flags for v2 sandbox to Chrome and Helper executable. This adds the flags for the v2 sandbox to Chrome and the Helper executable. The helper executable has its own declarations of the flags to minimize the static linking size. BUG=689306 Review-Url: https://codereview.chromium.org/2921733002 Cr-Commit-Position: refs/heads/master@{#476865} Committed: https://chromium.googlesource.com/chromium/src/+/6276ea0023c2fdc01609622f5d6b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6276ea0023c2fdc01609622f5d6b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
