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

Issue 226033002: Ensure seccomp-bpf cannot be silently disabled for non-SFI NaCl (Closed)

Created:
6 years, 8 months ago by hamaji
Modified:
6 years, 8 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Ensure seccomp-bpf cannot be silently disabled for non-SFI NaCl Also introduce --nacl-dangerous-no-sandbox-nonsfi, which allows us to skip sandbox entirely for development. TEST=out/Release/browser_tests --gtest_filter='NaCl*' TEST=trybots BUG=359230 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263824

Patch Set 1 #

Total comments: 7

Patch Set 2 : #

Total comments: 8

Patch Set 3 : #

Total comments: 4

Patch Set 4 : #

Patch Set 5 : #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -23 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 1 chunk +1 line, -0 lines 3 comments Download
M chrome/browser/ui/startup/bad_flags_prompt.cc View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M components/nacl.gyp View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/nacl/common/nacl_switches.h View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M components/nacl/common/nacl_switches.cc View 1 2 2 chunks +11 lines, -6 lines 0 comments Download
M components/nacl/loader/nacl_helper_linux.cc View 1 2 3 4 4 chunks +42 lines, -15 lines 0 comments Download
M components/nacl/zygote/nacl_fork_delegate_linux.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
hamaji
https://codereview.chromium.org/226033002/diff/1/components/nacl/loader/nacl_helper_linux.cc File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/226033002/diff/1/components/nacl/loader/nacl_helper_linux.cc#newcode58 components/nacl/loader/nacl_helper_linux.cc:58: if (getenv("NACL_DANGEROUS_DISABLE_NONSFI_SANDBOX")) { We need to whitelist this (and ...
6 years, 8 months ago (2014-04-04 14:36:58 UTC) #1
Mark Seaborn
FYI, you should manually run the "linux_rel_precise32" trybot for all Non-SFI-Mode-related changes, now that Hidehiko ...
6 years, 8 months ago (2014-04-04 15:45:37 UTC) #2
hamaji
https://codereview.chromium.org/226033002/diff/1/components/nacl/loader/nacl_helper_linux.cc File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/226033002/diff/1/components/nacl/loader/nacl_helper_linux.cc#newcode58 components/nacl/loader/nacl_helper_linux.cc:58: if (getenv("NACL_DANGEROUS_DISABLE_NONSFI_SANDBOX")) { On 2014/04/04 15:45:37, Mark Seaborn wrote: ...
6 years, 8 months ago (2014-04-04 18:57:48 UTC) #3
Mark Seaborn
On 4 April 2014 11:57, <hamaji@chromium.org> wrote: > > https://codereview.chromium.org/226033002/diff/1/ > components/nacl/loader/nacl_helper_linux.cc > File components/nacl/loader/nacl_helper_linux.cc ...
6 years, 8 months ago (2014-04-04 19:49:52 UTC) #4
hamaji
On 2014/04/04 19:49:52, Mark Seaborn wrote: > On 4 April 2014 11:57, <mailto:hamaji@chromium.org> wrote: > ...
6 years, 8 months ago (2014-04-07 08:51:58 UTC) #5
jln (very slow on Chromium)
> > Julien and I talked about this yesterday. nacl_helper doesn't inherit all > > ...
6 years, 8 months ago (2014-04-07 23:20:14 UTC) #6
jln (very slow on Chromium)
https://codereview.chromium.org/226033002/diff/20001/components/nacl/loader/nacl_helper_linux.cc File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/226033002/diff/20001/components/nacl/loader/nacl_helper_linux.cc#newcode110 components/nacl/loader/nacl_helper_linux.cc:110: no_sandbox = cmd_line->HasSwitch(switches::kNaClDangerousNoSandboxNonSfi); Does this really work? How does ...
6 years, 8 months ago (2014-04-07 23:38:10 UTC) #7
Mark Seaborn
https://codereview.chromium.org/226033002/diff/20001/components/nacl/common/nacl_switches.cc File components/nacl/common/nacl_switches.cc (right): https://codereview.chromium.org/226033002/diff/20001/components/nacl/common/nacl_switches.cc#newcode52 components/nacl/common/nacl_switches.cc:52: const char kNaClDangerousNoSandboxNonSfi[] = Nit: please sort these https://codereview.chromium.org/226033002/diff/20001/components/nacl/common/nacl_switches.h ...
6 years, 8 months ago (2014-04-07 23:39:47 UTC) #8
hamaji
https://codereview.chromium.org/226033002/diff/20001/components/nacl/common/nacl_switches.cc File components/nacl/common/nacl_switches.cc (right): https://codereview.chromium.org/226033002/diff/20001/components/nacl/common/nacl_switches.cc#newcode52 components/nacl/common/nacl_switches.cc:52: const char kNaClDangerousNoSandboxNonSfi[] = On 2014/04/07 23:39:48, Mark Seaborn ...
6 years, 8 months ago (2014-04-08 04:38:30 UTC) #9
jln (very slow on Chromium)
https://chromiumcodereview.appspot.com/226033002/diff/30001/components/nacl/common/nacl_switches.h File components/nacl/common/nacl_switches.h (right): https://chromiumcodereview.appspot.com/226033002/diff/30001/components/nacl/common/nacl_switches.h#newcode19 components/nacl/common/nacl_switches.h:19: extern const char kNaClDangerousNoSandboxNonSfi[]; Please, add this to chrome/browser/ui/startup/bad_flags_prompt.cc ...
6 years, 8 months ago (2014-04-08 22:25:16 UTC) #10
hamaji
https://codereview.chromium.org/226033002/diff/30001/components/nacl/common/nacl_switches.h File components/nacl/common/nacl_switches.h (right): https://codereview.chromium.org/226033002/diff/30001/components/nacl/common/nacl_switches.h#newcode19 components/nacl/common/nacl_switches.h:19: extern const char kNaClDangerousNoSandboxNonSfi[]; On 2014/04/08 22:25:16, jln wrote: ...
6 years, 8 months ago (2014-04-09 00:06:47 UTC) #11
jln (very slow on Chromium)
This looks ok in general, let's see what Mark says! https://chromiumcodereview.appspot.com/226033002/diff/70001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://chromiumcodereview.appspot.com/226033002/diff/70001/chrome/browser/chrome_content_browser_client.cc#newcode1641 ...
6 years, 8 months ago (2014-04-10 19:08:24 UTC) #12
jln (very slow on Chromium)
Note: I'm planning to change a few things anyways: - Enforce the sandbox status (as ...
6 years, 8 months ago (2014-04-10 19:11:16 UTC) #13
hamaji
https://codereview.chromium.org/226033002/diff/70001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/226033002/diff/70001/chrome/browser/chrome_content_browser_client.cc#newcode1641 chrome/browser/chrome_content_browser_client.cc:1641: switches::kNaClDangerousNoSandboxNonSfi, On 2014/04/10 19:08:24, jln wrote: > I don't ...
6 years, 8 months ago (2014-04-10 19:17:54 UTC) #14
jln (very slow on Chromium)
lgtm (but let's wait for Mark) https://codereview.chromium.org/226033002/diff/70001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/226033002/diff/70001/chrome/browser/chrome_content_browser_client.cc#newcode1641 chrome/browser/chrome_content_browser_client.cc:1641: switches::kNaClDangerousNoSandboxNonSfi, On 2014/04/10 ...
6 years, 8 months ago (2014-04-10 19:19:32 UTC) #15
Mark Seaborn
Please update the commit message. It still refers to the NACL_DANGEROUS_DISABLE_NONSFI_SANDBOX env var. Then LGTM.
6 years, 8 months ago (2014-04-14 17:51:48 UTC) #16
hamaji
The CQ bit was checked by hamaji@chromium.org
6 years, 8 months ago (2014-04-14 18:00:01 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/226033002/70001
6 years, 8 months ago (2014-04-14 18:01:07 UTC) #18
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-14 18:36:20 UTC) #19
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=61216
6 years, 8 months ago (2014-04-14 18:36:20 UTC) #20
hamaji
+sky for chrome/browser/ui OWNER approval.
6 years, 8 months ago (2014-04-14 18:40:22 UTC) #21
sky
LGTM
6 years, 8 months ago (2014-04-14 20:21:51 UTC) #22
hamaji
The CQ bit was checked by hamaji@chromium.org
6 years, 8 months ago (2014-04-14 20:38:15 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/226033002/70001
6 years, 8 months ago (2014-04-14 20:39:18 UTC) #24
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-15 05:58:03 UTC) #25
commit-bot: I haz the power
The commit queue went berserk retrying too often for a seemingly flaky test on builder ...
6 years, 8 months ago (2014-04-15 05:58:04 UTC) #26
jln (very slow on Chromium)
The CQ bit was checked by jln@chromium.org
6 years, 8 months ago (2014-04-15 06:01:17 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/226033002/70001
6 years, 8 months ago (2014-04-15 06:02:35 UTC) #28
commit-bot: I haz the power
6 years, 8 months ago (2014-04-15 12:59:47 UTC) #29
Message was sent while issue was closed.
Change committed as 263824

Powered by Google App Engine
This is Rietveld 408576698