Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180783019:
[Android] Define a baseline seccomp-bpf sandbox policy. (Closed)
|
Created:
6 years, 9 months ago by Robert Sesek Modified:
6 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, agl, jln+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Define a baseline seccomp-bpf sandbox policy.
This is not used in production yet, since Android kernels do not have seccomp
mode two support, yet.
BUG=308763, 166704
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263017
Patch Set 1 #
Total comments: 7
Patch Set 2 : Move to //content #
Total comments: 17
Patch Set 3 : Rebase & address comments #
Total comments: 4
Patch Set 4 : More moves, nits #
Total comments: 2
Patch Set 5 : Fun With Flags! #
Total comments: 6
Patch Set 6 : Address comments, remove warmup #
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:98: return true; Obviously these cannot go in as-is. I can see three options here: 1) Put this behind a flag. We want seccomp-bpf on Android behind a flag, but I'm not sure where to put this flag since it would need to be accessed by both //content/ and //sandbox/. 2) Just use #if defined(OS_ANDROID) 3) Use the expected prctl() tests for the threadgroup synchronization presence in the kernel, and only call IssingleThreaded() if that test fails.
Wow, this is looking pretty great! Can you confirm that it was tested with the policy being enforced on all threads? https://chromiumcodereview.appspot.com/180783019/diff/1/sandbox/linux/seccomp... File sandbox/linux/seccomp-bpf-helpers/baseline_policy_android.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/1/sandbox/linux/seccomp... sandbox/linux/seccomp-bpf-helpers/baseline_policy_android.cc:24: case __NR_open: Very excited if this works! We may want to experiment with having a "broker thread" in the browser, or a better broker process model and broker out open afterwards. https://chromiumcodereview.appspot.com/180783019/diff/1/sandbox/linux/seccomp... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/1/sandbox/linux/seccomp... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:98: return true; On 2014/03/05 18:27:54, rsesek wrote: > 3) Use the expected prctl() tests for the threadgroup synchronization presence > in the kernel, and only call IssingleThreaded() if that test fails. Yes, the sandbox:: seccomp-bpf class should support a StartSandboxWithThreads() that uses the new prctl flag and disables these checks. Later, we'll indeed probably want to simply detect if that flag is available and use it automatically in StartSandbox(). In your test, you did patch the prctl flag in, right? i.e. you're sure that everything has been tested with the sandbox being applied to all threads?
https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf-he... File sandbox/linux/seccomp-bpf-helpers/baseline_policy_android.cc (right): https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf-he... sandbox/linux/seccomp-bpf-helpers/baseline_policy_android.cc:24: case __NR_open: On 2014/03/07 01:30:30, jln wrote: > Very excited if this works! > > We may want to experiment with having a "broker thread" in the browser, or a > better broker process model and broker out open afterwards. Yes, this does work! This mostly appears to be reading data files in both the apk and the system partition. https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:98: return true; On 2014/03/07 01:30:30, jln wrote: > On 2014/03/05 18:27:54, rsesek wrote: > > 3) Use the expected prctl() tests for the threadgroup synchronization presence > > in the kernel, and only call IssingleThreaded() if that test fails. > > Yes, the sandbox:: seccomp-bpf class should support a StartSandboxWithThreads() > that uses the new prctl flag and disables these checks. OK I'll work on that, then. How do you think SupportsSeccompSandobx() should work, then? It internally fork()s which seems like a bad thing to do as a generic interface when we know we're multithreaded. But I don't know how else we should test for the seccomp extensions. > In your test, you did patch the prctl flag in, right? i.e. you're sure that > everything has been tested with the sandbox being applied to all threads? Yes, I have the kernel patches applied to my AOSP build.
https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:98: return true; On 2014/03/25 21:57:17, rsesek wrote: > On 2014/03/07 01:30:30, jln wrote: > > On 2014/03/05 18:27:54, rsesek wrote: > > > 3) Use the expected prctl() tests for the threadgroup synchronization > presence > > > in the kernel, and only call IssingleThreaded() if that test fails. > > > > Yes, the sandbox:: seccomp-bpf class should support a > StartSandboxWithThreads() > > that uses the new prctl flag and disables these checks. > > OK I'll work on that, then. How do you think SupportsSeccompSandobx() should > work, then? It internally fork()s which seems like a bad thing to do as a > generic interface when we know we're multithreaded. But I don't know how else we > should test for the seccomp extensions. We could have StartSandboxWithThreads() return a bool, depending on whether or not it managed to start. For consistency we could change StartSandbox() in a similar way and just make sure that the existing callers CHECK().
https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right): https://codereview.chromium.org/180783019/diff/1/sandbox/linux/seccomp-bpf/sa... sandbox/linux/seccomp-bpf/sandbox_bpf.cc:98: return true; On 2014/03/31 19:22:32, jln wrote: > On 2014/03/25 21:57:17, rsesek wrote: > > On 2014/03/07 01:30:30, jln wrote: > > > On 2014/03/05 18:27:54, rsesek wrote: > > > > 3) Use the expected prctl() tests for the threadgroup synchronization > > presence > > > > in the kernel, and only call IssingleThreaded() if that test fails. > > > > > > Yes, the sandbox:: seccomp-bpf class should support a > > StartSandboxWithThreads() > > > that uses the new prctl flag and disables these checks. > > > > OK I'll work on that, then. How do you think SupportsSeccompSandobx() should > > work, then? It internally fork()s which seems like a bad thing to do as a > > generic interface when we know we're multithreaded. But I don't know how else > we > > should test for the seccomp extensions. > > We could have StartSandboxWithThreads() return a bool, depending on whether or > not it managed to start. > For consistency we could change StartSandbox() in a similar way and just make > sure that the existing callers CHECK(). OK, that's also what I was thinking, though probably return SandboxStatus instead of bool. I'll do that in another CL and make this one just about landing the Android policy, which I'll also move to //content, per your suggestion.
OK this is ready for another review. PTAL.
Shouldn't we land patches to sandbox/ before landing this? It's a bit weird to land this CL before its dependency. Also I made a bunch of comments about changing StartSandbox()'s API. You may want to make it: bool StartSandbox(bool allow_threads); allow_thread would disable the thread checking and use the correct, new prctl() instead. https://chromiumcodereview.appspot.com/180783019/diff/50001/content/common/an... File content/common/android/sandbox_bpf_base_policy_android.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/50001/content/common/an... content/common/android/sandbox_bpf_base_policy_android.cc:25: case __NR_open: libc have tended in the past to deprecate open in favor of openat. You could add openat already, for future compatibility. Also FYI, ARM64 doesn't have open() anymore. Could you also add a comment to NR_open specifically. It breaks our traditional model, so explain that we expect uid separation to restrict file access and that this policy can only retrict the kernel's attack surface. Maybe even put it in a "section" of its own to make this extra clear. Also I'm a bit surprised that stat() / access() are not required, but that's not bad news :) https://chromiumcodereview.appspot.com/180783019/diff/50001/content/common/an... content/common/android/sandbox_bpf_base_policy_android.cc:26: Why the extra space? https://chromiumcodereview.appspot.com/180783019/diff/50001/content/common/an... content/common/android/sandbox_bpf_base_policy_android.cc:29: case __NR_flock: Please, sort these in alphabetical order https://chromiumcodereview.appspot.com/180783019/diff/50001/content/common/an... content/common/android/sandbox_bpf_base_policy_android.cc:42: if (allowed) Nit: I find the construct a bit misleading, because one could be tempted to disallow a system call by setting allow = false. Maybe rename "allowed" with "override_and_allow"? https://chromiumcodereview.appspot.com/180783019/diff/50001/content/content_c... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/180783019/diff/50001/content/content_c... content/content_common.gypi:135: 'common/android/sandbox_bpf_base_policy_android.cc', Shouldn't we have this in common/sandbox_linux/ with everything else? /android/ would be more justifiable if we didn't have to depend on /sandbox_linux/ anyways. Feel free to rename /sandbox_linux/ to a more encompassing term that includes Android if you can come up with one. Having all the policies in one place helps avoiding forgetting about the dependencies while changing a "base" policy. I don't feel too strongly about this, so ignore if you prefer. https://chromiumcodereview.appspot.com/180783019/diff/50001/content/renderer/... File content/renderer/renderer_main_platform_delegate_android.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/50001/content/renderer/... content/renderer/renderer_main_platform_delegate_android.cc:76: sandbox::SandboxBPF sandbox; I would rather not duplicate this. Could you use sandbox_seccomp_bpf_linux.h, StartSandboxWithExternalPolicy? It does call SupportsSeccompSandbox(), so we should fix that first. Probably the StartSandbox() API should return a bool across the board and we should be able to remove calls to SupportsSeccompSandbox() when actually enabling the sandbox. https://chromiumcodereview.appspot.com/180783019/diff/50001/content/renderer/... content/renderer/renderer_main_platform_delegate_android.cc:80: sandbox::SandboxBPF::STATUS_ENABLED; Let's try and fix StartSandbox() to return a bool instead.
On 2014/03/31 22:57:43, jln wrote: > Shouldn't we land patches to sandbox/ before landing this? > > It's a bit weird to land this CL before its dependency. Yes this is a logical dependency violation, but since this won't be tested with CI for a while (due to lack of kernel support), it in practice doesn't matter. I figured since this piece is done, I might as well land it. But I can implement the sandbox/ changes first.
Rebased on top of the sandbox API changes. https://codereview.chromium.org/180783019/diff/50001/content/common/android/s... File content/common/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/180783019/diff/50001/content/common/android/s... content/common/android/sandbox_bpf_base_policy_android.cc:25: case __NR_open: On 2014/03/31 22:57:43, jln wrote: > libc have tended in the past to deprecate open in favor of openat. You could add > openat already, for future compatibility. Also FYI, ARM64 doesn't have open() > anymore. Added __NR_openat and put __NR_open behind __aarch64__. > Could you also add a comment to NR_open specifically. It breaks our traditional > model, so explain that we expect uid separation to restrict file access and that > this policy can only retrict the kernel's attack surface. Done. > Maybe even put it in a "section" of its own to make this extra clear. I initially had it in sections (c.f. "Why the extra space?"), but I've collapsed it all. > Also I'm a bit surprised that stat() / access() are not required, but that's not > bad news :) Me too *shrug*. https://codereview.chromium.org/180783019/diff/50001/content/common/android/s... content/common/android/sandbox_bpf_base_policy_android.cc:26: On 2014/03/31 22:57:43, jln wrote: > Why the extra space? Done. https://codereview.chromium.org/180783019/diff/50001/content/common/android/s... content/common/android/sandbox_bpf_base_policy_android.cc:29: case __NR_flock: On 2014/03/31 22:57:43, jln wrote: > Please, sort these in alphabetical order Done. https://codereview.chromium.org/180783019/diff/50001/content/common/android/s... content/common/android/sandbox_bpf_base_policy_android.cc:42: if (allowed) On 2014/03/31 22:57:43, jln wrote: > Nit: I find the construct a bit misleading, because one could be tempted to > disallow a system call by setting allow = false. > > Maybe rename "allowed" with "override_and_allow"? Done. https://codereview.chromium.org/180783019/diff/50001/content/content_common.gypi File content/content_common.gypi (right): https://codereview.chromium.org/180783019/diff/50001/content/content_common.g... content/content_common.gypi:135: 'common/android/sandbox_bpf_base_policy_android.cc', On 2014/03/31 22:57:43, jln wrote: > Shouldn't we have this in common/sandbox_linux/ with everything else? /android/ > would be more justifiable if we didn't have to depend on /sandbox_linux/ > anyways. > > Feel free to rename /sandbox_linux/ to a more encompassing term that includes > Android if you can come up with one. > > Having all the policies in one place helps avoiding forgetting about the > dependencies while changing a "base" policy. > > I don't feel too strongly about this, so ignore if you prefer. I don't know if it makes more sense to organize them by OS (which is how most of //content is organized) or to centralize all the policies in one place. Maybe it makes the most sense to keep the shared components in //content/common/sandbox_bpf and then put the OS specific bits in //content/common/{linux,android}? https://codereview.chromium.org/180783019/diff/50001/content/renderer/rendere... File content/renderer/renderer_main_platform_delegate_android.cc (right): https://codereview.chromium.org/180783019/diff/50001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_android.cc:76: sandbox::SandboxBPF sandbox; On 2014/03/31 22:57:43, jln wrote: > I would rather not duplicate this. > > Could you use sandbox_seccomp_bpf_linux.h, StartSandboxWithExternalPolicy? > > It does call SupportsSeccompSandbox(), so we should fix that first. > Probably the StartSandbox() API should return a bool across the board and we > should be able to remove calls to SupportsSeccompSandbox() when actually > enabling the sandbox. StartSandboxWithExternalPolicy() will call through the single-threadedness checks. I think with the new bool-returning API, this is not enough code to worry about duplicating. https://codereview.chromium.org/180783019/diff/50001/content/renderer/rendere... content/renderer/renderer_main_platform_delegate_android.cc:80: sandbox::SandboxBPF::STATUS_ENABLED; On 2014/03/31 22:57:43, jln wrote: > Let's try and fix StartSandbox() to return a bool instead. Done.
lgtm, but please consider unifying the command line flags. https://chromiumcodereview.appspot.com/180783019/diff/50001/content/content_c... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/180783019/diff/50001/content/content_c... content/content_common.gypi:135: 'common/android/sandbox_bpf_base_policy_android.cc', On 2014/04/08 20:33:45, rsesek wrote: > I don't know if it makes more sense to organize them by OS (which is how most of > //content is organized) or to centralize all the policies in one place. Maybe it > makes the most sense to keep the shared components in > //content/common/sandbox_bpf and then put the OS specific bits in > //content/common/{linux,android}? Maybe //content/common/sandbox_bpf/{linux,android} with the common files in the top directory? https://chromiumcodereview.appspot.com/180783019/diff/50001/content/renderer/... File content/renderer/renderer_main_platform_delegate_android.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/50001/content/renderer/... content/renderer/renderer_main_platform_delegate_android.cc:76: sandbox::SandboxBPF sandbox; On 2014/04/08 20:33:45, rsesek wrote: > StartSandboxWithExternalPolicy() will call through the single-threadedness > checks. I think with the new bool-returning API, this is not enough code to > worry about duplicating. I'm on the fence (we could add a parameter to StartSandboxWithExternalPolicy), so let's do it your way. But I'm always reminded by how evil code duplication is in surprising ways :) https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... content/public/common/content_switches.cc:361: const char kEnableAndroidSeccompBPF[] = "enable-android-seccomp-bpf"; Maybe call it kEnableAndroidSeccompFilter for consistency with kDisableSeccompFilterSandbox ? I wonder if we shouldn't just have kDisableSeccompFilterSandbox and kEnableSeccompFilterSandbox (without specifying Android). Then each OS can implement its own policy of what is the default and let the command line override that.
Addendum: could you make sure that yourself + the current sandbox_linux owners are OWNERS of the new files?
https://chromiumcodereview.appspot.com/180783019/diff/50001/content/content_c... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/180783019/diff/50001/content/content_c... content/content_common.gypi:135: 'common/android/sandbox_bpf_base_policy_android.cc', On 2014/04/09 05:11:07, jln wrote: > On 2014/04/08 20:33:45, rsesek wrote: > > > I don't know if it makes more sense to organize them by OS (which is how most > of > > //content is organized) or to centralize all the policies in one place. Maybe > it > > makes the most sense to keep the shared components in > > //content/common/sandbox_bpf and then put the OS specific bits in > > //content/common/{linux,android}? > > Maybe //content/common/sandbox_bpf/{linux,android} with the common files in the > top directory? OK that SG. I've moved these into //content/common/sandbox_linux/android for now. I'll move files around in a followup. https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... content/public/common/content_switches.cc:361: const char kEnableAndroidSeccompBPF[] = "enable-android-seccomp-bpf"; On 2014/04/09 05:11:07, jln wrote: > Maybe call it kEnableAndroidSeccompFilter for consistency with > kDisableSeccompFilterSandbox ? > > I wonder if we shouldn't just have kDisableSeccompFilterSandbox and > kEnableSeccompFilterSandbox (without specifying Android). Then each OS can > implement its own policy of what is the default and let the command line > override that. Renamed to match kDisableSeccompFilterSandbox. I'm not sure we need something more generic, since we expect all our Linux flavors to eventually have seccomp-bpf, right? I imagine this flag will go away once all the stars align, though we'll keep the disable switch for testing purposes. https://chromiumcodereview.appspot.com/180783019/diff/90001/content/common/sa... File content/common/sandbox_linux/OWNERS (right): https://chromiumcodereview.appspot.com/180783019/diff/90001/content/common/sa... content/common/sandbox_linux/OWNERS:4: rsesek@chromium.org Let me know if you only want me in .../android/ instead.
Still lgtm, but still having a slight preference for --enable-seccomp-filter-sandbox https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... content/public/common/content_switches.cc:361: const char kEnableAndroidSeccompBPF[] = "enable-android-seccomp-bpf"; On 2014/04/09 21:00:58, rsesek wrote: > On 2014/04/09 05:11:07, jln wrote: > > Maybe call it kEnableAndroidSeccompFilter for consistency with > > kDisableSeccompFilterSandbox ? > > > > I wonder if we shouldn't just have kDisableSeccompFilterSandbox and > > kEnableSeccompFilterSandbox (without specifying Android). Then each OS can > > implement its own policy of what is the default and let the command line > > override that. > > Renamed to match kDisableSeccompFilterSandbox. I'm not sure we need something > more generic, since we expect all our Linux flavors to eventually have > seccomp-bpf, right? I imagine this flag will go away once all the stars align, > though we'll keep the disable switch for testing purposes. That works, but I still would think that kEnableSeccompFilterSandbox is better. What I meant is that I don't see the need for per-OS flags here. We can have two generic flags, each platform can have its own default and use the flags as appropriate. For instance, in Linux, we could use this flag to FORCE a seccomp sandbox (instead of failing silently if it's disabled - which may be a good thing to do on Chrome OS). I also fear that we may need this flag as we bring-up new platforms (Aarch64 / MIPS). https://chromiumcodereview.appspot.com/180783019/diff/90001/content/common/sa... File content/common/sandbox_linux/OWNERS (right): https://chromiumcodereview.appspot.com/180783019/diff/90001/content/common/sa... content/common/sandbox_linux/OWNERS:4: rsesek@chromium.org On 2014/04/09 21:00:58, rsesek wrote: > Let me know if you only want me in .../android/ instead. It's fine since I trust you to know what you will know how to review. But if you start getting too many reviews that you need to forward, you can simply add a comment such as "# For Android". It's unlikely to be a problem though, these files rarely get any change from non security folks.
+jochen for content OWNERS https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... File content/public/common/content_switches.cc (right): https://chromiumcodereview.appspot.com/180783019/diff/70001/content/public/co... content/public/common/content_switches.cc:361: const char kEnableAndroidSeccompBPF[] = "enable-android-seccomp-bpf"; On 2014/04/09 21:56:19, jln wrote: > On 2014/04/09 21:00:58, rsesek wrote: > > On 2014/04/09 05:11:07, jln wrote: > > > Maybe call it kEnableAndroidSeccompFilter for consistency with > > > kDisableSeccompFilterSandbox ? > > > > > > I wonder if we shouldn't just have kDisableSeccompFilterSandbox and > > > kEnableSeccompFilterSandbox (without specifying Android). Then each OS can > > > implement its own policy of what is the default and let the command line > > > override that. > > > > Renamed to match kDisableSeccompFilterSandbox. I'm not sure we need something > > more generic, since we expect all our Linux flavors to eventually have > > seccomp-bpf, right? I imagine this flag will go away once all the stars align, > > though we'll keep the disable switch for testing purposes. > > That works, but I still would think that kEnableSeccompFilterSandbox is better. > > What I meant is that I don't see the need for per-OS flags here. We can have two > generic flags, each platform can have its own default and use the flags as > appropriate. > For instance, in Linux, we could use this flag to FORCE a seccomp sandbox > (instead of failing silently if it's disabled - which may be a good thing to do > on Chrome OS). I also fear that we may need this flag as we bring-up new > platforms (Aarch64 / MIPS). Done.
https://codereview.chromium.org/180783019/diff/110001/content/common/sandbox_... File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/180783019/diff/110001/content/common/sandbox_... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:34: #if !defined(__aarch64__) should be ARCH_CPU_ARM64 https://codereview.chromium.org/180783019/diff/110001/content/renderer/render... File content/renderer/renderer_main_platform_delegate_android.cc (right): https://codereview.chromium.org/180783019/diff/110001/content/renderer/render... content/renderer/renderer_main_platform_delegate_android.cc:33: base::SysInfo::AmountOfPhysicalMemory(); add base::SysInfo::AmountOfVirtualMemory() https://codereview.chromium.org/180783019/diff/110001/content/renderer/render... content/renderer/renderer_main_platform_delegate_android.cc:38: v8::V8::Initialize(); can you explain why you add this here? This conflicts with the initialization that happens in blink/gin?
https://codereview.chromium.org/180783019/diff/110001/content/common/sandbox_... File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/180783019/diff/110001/content/common/sandbox_... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:34: #if !defined(__aarch64__) On 2014/04/10 07:37:42, jochen wrote: > should be ARCH_CPU_ARM64 Done. https://codereview.chromium.org/180783019/diff/110001/content/renderer/render... File content/renderer/renderer_main_platform_delegate_android.cc (right): https://codereview.chromium.org/180783019/diff/110001/content/renderer/render... content/renderer/renderer_main_platform_delegate_android.cc:33: base::SysInfo::AmountOfPhysicalMemory(); On 2014/04/10 07:37:42, jochen wrote: > add base::SysInfo::AmountOfVirtualMemory() See below. https://codereview.chromium.org/180783019/diff/110001/content/renderer/render... content/renderer/renderer_main_platform_delegate_android.cc:38: v8::V8::Initialize(); On 2014/04/10 07:37:42, jochen wrote: > can you explain why you add this here? This conflicts with the initialization > that happens in blink/gin? Thanks for calling this out. All of this was leftover from before we decided we have to allow __NR_open. Therefore, none of this warmup is actually necessary at all. We need to take a different approach to restrict the filesystem.
lgtm
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/180783019/130001
Message was sent while issue was closed.
Change committed as 263017
Message was sent while issue was closed.
There is something wrong with this change, as seccomp related header files are included even for platforms which do not include support for seccomp (such as MIPS). This is breaking Chromium Android build for such platforms. For more details please see: http://www.rt-rk.com/mips-buildbot/builders/Release_build/builds/189/steps/Bu...
Message was sent while issue was closed.
On 2014/04/11 17:04:51, gordanac wrote: > There is something wrong with this change, as seccomp related header files are > included even for platforms which do not include support for seccomp (such as > MIPS). > > This is breaking Chromium Android build for such platforms. > > For more details please see: > http://www.rt-rk.com/mips-buildbot/builders/Release_build/builds/189/steps/Bu... That is being tracked at http://code.google.com/p/chromium/issues/detail?id=362357. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
