Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1840923002:
Removed setuid Sandbox Check as Per Bug#598454 (Closed)
DescriptionRemoved setuid Sandbox Check as Per Bug#598454
I removed the setuid checks in the browser_main_loop.cc SetupSandbox method.
Because the only code left in the SetupSandbox method was tickling the
zygote host so it would fork, the method was renamed to TickleSandbox.
Additionally this is my first patch, so I need try-jobs to be run for me.
BUG=598454
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed call to init sandbox_binary #
Total comments: 1
Patch Set 3 : Added conditional to keep the sandbox in ChromeOS #
Total comments: 2
Messages
Total messages: 33 (13 generated)
nlurski@gmail.com changed reviewers: + dpranke@chromium.org
Removed setuid Sandbox Check as Per Bug#598454 I removed the setuid checks in the browser_main_loop.cc SetupSandbox method. Because the only code left in the SetupSandbox method was tickling the zygote host so it would fork, the method was renamed to TickleSandbox. Additionally this is my first patch, so I need try-jobs to be run for me. BUG=598454
nlurski@gmail.com changed reviewers: + xiyuan@chromium.org
nlurski@gmail.com changed reviewers: - dpranke@chromium.org
xiyuan@chromium.org changed reviewers: + jln@chromium.org
I am not familiar with the code, passing to jln@.
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840923002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840923002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== Removed setuid Sandbox Check as Per Bug#598454 I removed the setuid checks in the browser_main_loop.cc SetupSandbox method. Because the only code left in the SetupSandbox method was tickling the zygote host so it would fork, the method was renamed to TickleSandbox. Additionally this is my first patch, so I need try-jobs to be run for me. BUG=598454 ========== to ========== Removed setuid Sandbox Check as Per Bug#598454 I removed the setuid checks in the browser_main_loop.cc SetupSandbox method. Because the only code left in the SetupSandbox method was tickling the zygote host so it would fork, the method was renamed to TickleSandbox. Additionally this is my first patch, so I need try-jobs to be run for me. BUG=598454 ==========
jln@chromium.org changed reviewers: + mdempsky@chromium.org, rickyz@chromium.org
The goal of this code is in part to avoid very weird and puzzling failures later on while Chrome starts with the setuid sandbox. I don't believe that at this point we can completely deprecate the setuid sandbox. This is a complex decision because it needs to be informed by which Linux distros we want to still support. I haven't followed this recently, rickyz@ is the owner for this at this point. He's out for a bit, but let's wait for him to chime in.
On 2016/03/29 at 18:49:28, jln wrote: > The goal of this code is in part to avoid very weird and puzzling failures later on while Chrome starts with the setuid sandbox. I don't believe that at this point we can completely deprecate the setuid sandbox. This is a complex decision because it needs to be informed by which Linux distros we want to still support. > > I haven't followed this recently, rickyz@ is the owner for this at this point. He's out for a bit, but let's wait for him to chime in. The try jobs failed for some builds on linux along with casting on linux, so I do not think that this code is ready for commit yet. Would a good option for removing the setuid sandbox be having a "--with-setuid-sandbox" launch option for the distros that need it?
If you really want to tackle this bug, I'll review it. But in general, I would suggest that zygote and sandbox stuff is not a great starter project. https://codereview.chromium.org/1840923002/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1840923002/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:198: // ZygoteHostImpl::GetInstance()->Init(sandbox_binary.value()); We still need to call Init here.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
I defer to mdempsky and rickyz, but a couple of comments ... https://codereview.chromium.org/1840923002/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.cc (left): https://codereview.chromium.org/1840923002/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.cc:221: } Do we need to keep this logic on ChromeOS (i.e., change the #ifdefs to just #ifdef OS_CHROMEOS)? Also, "tickle" doesn't mean much to me. Perhaps we should leave it as SetupSandbox()?
Oh, also, if we don't need to use the setuid sandbox, we should probably also not build it at all, so maybe update the build files as well?
I added the call to Init(sandbox_binary.value()), but did not remove the build files yet. Seemed to pass the try-jobs that failed before when I ran them locally. I need to have them run for me again on the try server, thank you.
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840923002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840923002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nlurski@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840923002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840923002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/03/30 at 18:17:31, commit-bot wrote: > Dry run: No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Can someone restart the try-job? It looks like it failed through timing out in compilation of a file not affected by this patch.
On 2016/03/30 18:19:05, nlurski wrote: > Can someone restart the try-job? It looks like it failed through timing out in > compilation of a file not affected by this patch. It looks like someone did.
https://codereview.chromium.org/1840923002/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (left): https://codereview.chromium.org/1840923002/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:200: scoped_ptr<sandbox::SetuidSandboxHost> setuid_sandbox_host( We still need this code for Chrome OS. You can't just remove it unconditionally.
On 2016/03/30 at 19:10:41, mdempsky wrote: > https://codereview.chromium.org/1840923002/diff/20001/content/browser/browser... > File content/browser/browser_main_loop.cc (left): > > https://codereview.chromium.org/1840923002/diff/20001/content/browser/browser... > content/browser/browser_main_loop.cc:200: scoped_ptr<sandbox::SetuidSandboxHost> setuid_sandbox_host( > We still need this code for Chrome OS. You can't just remove it unconditionally. I added a conditional to leave in the original setuid sandbox checks for Chrome OS in my new patch.
https://codereview.chromium.org/1840923002/diff/40001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1840923002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:233: void SetupSandbox() { I don't understand why you're duplicating all of this code. Why not just wrap the CrOS-only logic in #if defined(OS_CHROMEOS)? https://codereview.chromium.org/1840923002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:453: SetupSandbox(); This won't compile on Chrome OS: your SetupSandbox signatures differ between the two cases. |
