Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(492)

Issue 897723005: Allow using the namespace sandbox in zygote host. (Closed)

Created:
5 years, 10 months ago by rickyz (no longer on Chrome)
Modified:
5 years, 10 months ago
CC:
chromium-reviews, grt+watch_chromium.org, darin-cc_chromium.org, Michael Moss, jam, wfh+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Allow using the namespace sandbox in zygote host. Currently, this is gated behind the enable-namespace-sandbox switch. Furthermore, the namespace sandbox is only used if seccomp-bpf is supported. BUG=312380 Committed: https://crrev.com/660e2d468bf22b4c315463173d0651af4fe6143f Cr-Commit-Position: refs/heads/master@{#315177}

Patch Set 1 : #

Total comments: 1

Patch Set 2 : Add back the flag check. #

Total comments: 14

Patch Set 3 : Respond to comments. #

Patch Set 4 : Add back the flag check. #

Patch Set 5 : Remove outdated comment. #

Patch Set 6 : Rebase #

Patch Set 7 : Enable NamespaceSandbox on the nacl helper. #

Patch Set 8 : Add back the flag check. #

Total comments: 10

Patch Set 9 : Respond to comments. #

Total comments: 12

Patch Set 10 : More comments. #

Total comments: 1

Patch Set 11 : Rebase #

Patch Set 12 : More comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+177 lines, -75 lines) Patch
M chrome/installer/linux/debian/expected_deps_ia32 View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/installer/linux/debian/expected_deps_x64 View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/installer/linux/rpm/expected_deps_i386 View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/installer/linux/rpm/expected_deps_x86_64 View 1 chunk +1 line, -0 lines 0 comments Download
M components/nacl.gyp View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +24 lines, -13 lines 0 comments Download
M components/nacl/zygote/nacl_fork_delegate_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +23 lines, -4 lines 0 comments Download
M content/browser/zygote_host/zygote_host_impl_linux.h View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/zygote_host/zygote_host_impl_linux.cc View 1 2 3 4 5 7 10 chunks +48 lines, -11 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M content/zygote/zygote_main_linux.cc View 1 2 3 4 5 6 7 8 9 9 chunks +55 lines, -45 lines 0 comments Download
M sandbox/linux/services/namespace_sandbox.h View 1 2 3 4 5 6 2 chunks +5 lines, -0 lines 0 comments Download
M sandbox/linux/services/namespace_sandbox.cc View 1 2 3 4 5 6 3 chunks +9 lines, -1 line 0 comments Download

Messages

Total messages: 31 (15 generated)
rickyz (no longer on Chrome)
5 years, 10 months ago (2015-02-04 08:09:33 UTC) #5
jln (very slow on Chromium)
This looks good in general. But I would move a little more carefully and have ...
5 years, 10 months ago (2015-02-05 00:26:56 UTC) #6
rickyz (no longer on Chrome)
https://codereview.chromium.org/897723005/diff/80001/content/browser/zygote_host/zygote_host_impl_linux.cc File content/browser/zygote_host/zygote_host_impl_linux.cc (right): https://codereview.chromium.org/897723005/diff/80001/content/browser/zygote_host/zygote_host_impl_linux.cc#newcode590 content/browser/zygote_host/zygote_host_impl_linux.cc:590: // Unlike the setuid sandbox, the namespace sandbox does ...
5 years, 10 months ago (2015-02-05 01:42:51 UTC) #7
jln (very slow on Chromium)
Sorry, I only skimmed through. I'll take another look tomorrow, as it's getting late. - ...
5 years, 10 months ago (2015-02-05 05:44:15 UTC) #8
rickyz (no longer on Chrome)
Ah ouch, I completely missed the fact that the nacl helper is forked from the ...
5 years, 10 months ago (2015-02-05 09:01:15 UTC) #10
jln (very slow on Chromium)
Looks good, but I would keep the Paranoia with the "must_enable_layer1" things. It's hard to ...
5 years, 10 months ago (2015-02-06 00:37:29 UTC) #17
rickyz (no longer on Chrome)
https://codereview.chromium.org/897723005/diff/300002/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/897723005/diff/300002/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc#newcode57 components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:57: bool MaybeSetProcessNonDumpable() { On 2015/02/06 00:37:29, jln wrote: > ...
5 years, 10 months ago (2015-02-06 01:53:19 UTC) #18
jln (very slow on Chromium)
lgtm In general I've been extremely wary of patterns that rely on the environment (or ...
5 years, 10 months ago (2015-02-06 02:17:04 UTC) #19
rickyz (no longer on Chrome)
Thanks, adding: nasko@ for content/public/common/content_switches thestig@ for chrome/installer/linux/*/expected_deps_* mseaborn@ For components/nacl https://codereview.chromium.org/897723005/diff/350001/components/nacl/zygote/nacl_fork_delegate_linux.cc File components/nacl/zygote/nacl_fork_delegate_linux.cc (right): ...
5 years, 10 months ago (2015-02-06 03:01:02 UTC) #21
rickyz (no longer on Chrome)
Oops, didn't actually add anyone back there - adding: nasko@ for content/public/common/content_switches thestig@ for chrome/installer/linux/*/expected_deps_* ...
5 years, 10 months ago (2015-02-06 03:01:52 UTC) #23
Lei Zhang
chrome/installer lgtm
5 years, 10 months ago (2015-02-06 03:06:58 UTC) #24
nasko
content/public/common/content_switches LGTM
5 years, 10 months ago (2015-02-06 22:35:59 UTC) #25
Mark Seaborn
LGTM for components/nacl https://codereview.chromium.org/897723005/diff/370001/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right): https://codereview.chromium.org/897723005/diff/370001/components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc#newcode122 components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:122: CHECK_EQ(kExpectedNumFds + 1, Nit: It seems ...
5 years, 10 months ago (2015-02-07 01:43:12 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/897723005/410001
5 years, 10 months ago (2015-02-07 02:42:21 UTC) #29
commit-bot: I haz the power
Committed patchset #12 (id:410001)
5 years, 10 months ago (2015-02-07 03:51:48 UTC) #30
commit-bot: I haz the power
5 years, 10 months ago (2015-02-07 03:52:39 UTC) #31
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/660e2d468bf22b4c315463173d0651af4fe6143f
Cr-Commit-Position: refs/heads/master@{#315177}

Powered by Google App Engine
This is Rietveld 408576698