Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1393283005:
Enable Control Flow Integrity for the official Linux Chrome. (Closed)
DescriptionEnable Control Flow Integrity for the official Linux Chrome.
This CL turns on CFI, a security check:
https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-integrity
http://clang.llvm.org/docs/ControlFlowIntegrity.html
This feature enables LTO (Link-Time Optimization) builds, which slow down the linker by 3x-4x.
CFI also comes with a code size overhead of about 7%-9%. The runtime CPU cost is less than 1%,
and should not be an issue.
BUG=chromium:464797
Intent to Implement thread:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2oklCAAJ
Committed: https://crrev.com/9aedd443e02995d017138da7913e97650f889641
Cr-Commit-Position: refs/heads/master@{#362856}
Patch Set 1 #Patch Set 2 : fix condition #
Total comments: 2
Patch Set 3 : exclude chrome os #Patch Set 4 : . #Patch Set 5 : restrict to x64 #Patch Set 6 : fmt #Patch Set 7 : gn #Patch Set 8 : exclude ChromeOS on GN #Patch Set 9 : Import //build/config/chrome_build.gni for is_chrome_branded #Patch Set 10 : sync #Messages
Total messages: 43 (18 generated)
The CQ bit was checked by krasin@google.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/1393283005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393283005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. 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.
krasin@google.com changed reviewers: + thakis@chromium.org
Nico, how do I test this change? Is there a trybot for the official builds?
Since update.sh checks for both buildtype=Official branding=Chrome, do you want to check for both here too? update.sh and this file should agree on what the trigger condition is. On 2015/10/12 17:51:52, krasin wrote: > Nico, > > how do I test this change? Is there a trybot for the official builds? As far as I know, there isn't. That's something I would've expected you to know though, given that you sent the intent to implement: If someone breaks tests in an official build, how are they supposed to debug this? I guess the idea is that this isn't very different from an official-only test failure without CFI? How often have you seen CFI-only breakage on your FYI bot?
On 2015/10/12 18:00:56, Nico wrote: > Since update.sh checks for both buildtype=Official branding=Chrome, do you want > to check for both here too? update.sh and this file should agree on what the > trigger condition is. Done. > > On 2015/10/12 17:51:52, krasin wrote: > > Nico, > > > > how do I test this change? Is there a trybot for the official builds? > > As far as I know, there isn't. That's something I would've expected you to know > though, given that you sent the intent to implement: If someone breaks tests in > an official build, how are they supposed to debug this? I guess the idea is that > this isn't very different from an official-only test failure without CFI? I am sorry. It seems that my question was not clear. While I am relatively certain that CFI is good to go into a production build (modulo https://crbug.com/538952, which I am going to resolve before submitting this CL), I have never built an official Chrome by myself (is it something that engineers could actually do? I failed to find any doc). Given that there's no trybot for that, I would appreciate if you give me a hint about how to build an official Chrome locally. Setting GYP_DEFINES='buildtype=Official branding=Chrome' gives me errors while doing gclient sync. I expect some special setup at fetch stage, but not sure. > How > often have you seen CFI-only breakage on your FYI bot? CFI-only breakage happens once per 2-3 weeks, like this one: https://crbug.com/541708
lgtm once the cros bit is resolved, and you gyp'd with this enabled and checked
that the compiler flags look good locally.
On 2015/10/12 18:10:59, krasin wrote:
> I have never built an official Chrome by myself (is it something that
engineers
> could actually do? I failed to find any doc).
> Given that there's no trybot for that, I would appreciate if you give me a
hint
> about how to build an official Chrome locally.
Add this to your ..\.gclient:
{ "name" : "src-internal",
"url" :
"https://chrome-internal.googlesource.com/chrome/src-internal.git",
"deps_file" : ".DEPS.git",
},
and run `gclient sync`. It'll give you an error about doing
https://chromium.googlesource.com/new-password , do that with your @google. Then
run `gclient sync` again.
> > How
> > often have you seen CFI-only breakage on your FYI bot?
> CFI-only breakage happens once per 2-3 weeks, like this one:
> https://crbug.com/541708
Ok, that sounds reasonable, thanks.
https://codereview.chromium.org/1393283005/diff/20001/build/common.gypi
File build/common.gypi (right):
https://codereview.chromium.org/1393283005/diff/20001/build/common.gypi#newco...
build/common.gypi:852: # Enable Control Flow Integrity for the official Linux
Chrome.
nit: Maybe add a comment that this condition should match the one in update.sh,
and add one in update.sh saying that condition should match common.gypi (? not
sure if that's a good idea; feel free to ignore)
https://codereview.chromium.org/1393283005/diff/20001/build/common.gypi#newco...
build/common.gypi:853: ['OS=="linux" and buildtype=="Official" and
branding=="Chrome"', {
This will also enable this for chromeos. Is that intentional? Probably not? If
not, add `and chromeos==0`.
On 2015/10/12 18:17:29, Nico wrote:
> lgtm once the cros bit is resolved, and you gyp'd with this enabled and
checked
> that the compiler flags look good locally.
>
> On 2015/10/12 18:10:59, krasin wrote:
> > I have never built an official Chrome by myself (is it something that
> engineers
> > could actually do? I failed to find any doc).
> > Given that there's no trybot for that, I would appreciate if you give me a
> hint
> > about how to build an official Chrome locally.
>
> Add this to your ..\.gclient:
>
> { "name" : "src-internal",
> "url" :
> "https://chrome-internal.googlesource.com/chrome/src-internal.git",
> "deps_file" : ".DEPS.git",
> },
>
> and run `gclient sync`. It'll give you an error about doing
> https://chromium.googlesource.com/new-password , do that with your @google.
Then
> run `gclient sync` again.
Thank you! In progress...
>
> > > How
> > > often have you seen CFI-only breakage on your FYI bot?
> > CFI-only breakage happens once per 2-3 weeks, like this one:
> > https://crbug.com/541708
>
> Ok, that sounds reasonable, thanks.
>
> https://codereview.chromium.org/1393283005/diff/20001/build/common.gypi
> File build/common.gypi (right):
>
>
https://codereview.chromium.org/1393283005/diff/20001/build/common.gypi#newco...
> build/common.gypi:852: # Enable Control Flow Integrity for the official Linux
> Chrome.
> nit: Maybe add a comment that this condition should match the one in
update.sh,
> and add one in update.sh saying that condition should match common.gypi (? not
> sure if that's a good idea; feel free to ignore)
Done.
>
>
https://codereview.chromium.org/1393283005/diff/20001/build/common.gypi#newco...
> build/common.gypi:853: ['OS=="linux" and buildtype=="Official" and
> branding=="Chrome"', {
> This will also enable this for chromeos. Is that intentional? Probably not? If
> not, add `and chromeos==0`.
That's a very good catch, thank you! Chrome OS is not tested yet, so not
launching CFI there.
One more thing I keep forgetting to ask: Is there a tracking bug for getting this going in .gn files?
On 2015/10/13 21:17:29, Nico wrote: > One more thing I keep forgetting to ask: Is there a tracking bug for getting > this going in .gn files? There's already some support for GN, but I have filed https://crbug.com/542858 to have a CFI buildbot that uses GN. It's possible that instead of making a separate buildbot, it makes sence re-target one of two existing buildbots.
Nico, GN support is done. The buildbot CL is https://codereview.chromium.org/1478863004/ Please, take a final look. I intend to submit this today, once I've got a green build from a GN buildbot (not GN-based runs yet).
lgtm (again assuming you checked that a gn build actually ends up passing the right files to the compiler) I'd maybe wait at least until later tomorrow though, given that the clang roll has been in less than two US working days...
On 2015/11/30 20:04:53, Nico wrote: > lgtm (again assuming you checked that a gn build actually ends up passing the > right files to the compiler) > > I'd maybe wait at least until later tomorrow though, given that the clang roll > has been in less than two US working days... Roger that. Will submit this CL tomorrow, if everything look good.
The CQ bit was checked by krasin@google.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/1393283005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393283005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by krasin@google.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/1393283005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393283005/140001
The CQ bit was checked by krasin@google.com to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by krasin@google.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393283005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393283005/160001
The CQ bit was checked by krasin@google.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/1393283005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393283005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krasin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1393283005/#ps180001 (title: "sync")
The CQ bit was unchecked by krasin@google.com
The CQ bit was checked by krasin@google.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/1393283005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393283005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krasin@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393283005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393283005/180001
Message was sent while issue was closed.
Description was changed from ========== Enable Control Flow Integrity for the official Linux Chrome. This CL turns on CFI, a security check: https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-i... http://clang.llvm.org/docs/ControlFlowIntegrity.html This feature enables LTO (Link-Time Optimization) builds, which slow down the linker by 3x-4x. CFI also comes with a code size overhead of about 7%-9%. The runtime CPU cost is less than 1%, and should not be an issue. BUG=chromium:464797 Intent to Implement thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... ========== to ========== Enable Control Flow Integrity for the official Linux Chrome. This CL turns on CFI, a security check: https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-i... http://clang.llvm.org/docs/ControlFlowIntegrity.html This feature enables LTO (Link-Time Optimization) builds, which slow down the linker by 3x-4x. CFI also comes with a code size overhead of about 7%-9%. The runtime CPU cost is less than 1%, and should not be an issue. BUG=chromium:464797 Intent to Implement thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Enable Control Flow Integrity for the official Linux Chrome. This CL turns on CFI, a security check: https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-i... http://clang.llvm.org/docs/ControlFlowIntegrity.html This feature enables LTO (Link-Time Optimization) builds, which slow down the linker by 3x-4x. CFI also comes with a code size overhead of about 7%-9%. The runtime CPU cost is less than 1%, and should not be an issue. BUG=chromium:464797 Intent to Implement thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... ========== to ========== Enable Control Flow Integrity for the official Linux Chrome. This CL turns on CFI, a security check: https://sites.google.com/a/chromium.org/dev/developers/testing/control-flow-i... http://clang.llvm.org/docs/ControlFlowIntegrity.html This feature enables LTO (Link-Time Optimization) builds, which slow down the linker by 3x-4x. CFI also comes with a code size overhead of about 7%-9%. The runtime CPU cost is less than 1%, and should not be an issue. BUG=chromium:464797 Intent to Implement thread: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pbJqt6ccMII/7iJC2... Committed: https://crrev.com/9aedd443e02995d017138da7913e97650f889641 Cr-Commit-Position: refs/heads/master@{#362856} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9aedd443e02995d017138da7913e97650f889641 Cr-Commit-Position: refs/heads/master@{#362856}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1498723002/ by krasin@google.com. The reason for reverting is: One of the buildbots timed out while linking Chrome: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux.... |
