This bug was created as a clone of Bug 620058 which has more context.

This patch adds -D_FORTIFY_SOURCE=2 to the set of hardening flags. Four different cases are possible, see: https://gcc.gnu.org/ml/gcc-patches/2004-09/msg02055.html Most used is level 2, seems also the best choice here. On macOS level 1 is enabled by default when compiling for Mac OS X v10.6 and later: https://developer.apple.com/library/content/documentation/Security/Conceptual/SecureCodingGuide/Articles/BufferOverflows.html#//apple_ref/doc/uid/TP40002577-SW26

Thanks for taking a look at this one! Before we add this to the hardening flags, we should test to see if there's a negative performance impact to enabling this; if not it should probably be on by default.

OK. I do not have access to the try server. I've just checked locally that it builds properly.

Comment on attachment 8864320 [details] [diff] [review] Add FORTIFY_SOURCE security flag Review of attachment 8864320 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r=me with the change below assuming that it builds correctly. Note that as Alex has said, we'll need a try run to sort out any performance issues. ::: build/moz.configure/toolchain.configure @@ +1004,5 @@ > > @depends('--enable-hardening', c_compiler) > def security_hardening_cflags(value, c_compiler): > if value and c_compiler.type in ['gcc', 'clang']: > + return '-fstack-protector-strong -D_FORTIFY_SOURCE=2' We want to make this a list now that we have multiple options. So: return ['-fstack-protector-strong', '-D_FORTIFY_SOURCE=2']

Comment on attachment 8864320 [details] [diff] [review] Add FORTIFY_SOURCE security flag ># HG changeset patch ># User Nomis101 <Nomis101@web.de> ># Date 1493850400 -7200 ># Thu May 04 00:26:40 2017 +0200 ># Node ID 62f613eb634feef3ae7e3de24aed2f4fb760883d ># Parent b25ad0674afd563e888dc07981baa626e8d794db >Bug 1359908 - Add FORTIFY_SOURCE security flag > >diff --git a/build/moz.configure/toolchain.configure b/build/moz.configure/toolchain.configure >--- a/build/moz.configure/toolchain.configure >+++ b/build/moz.configure/toolchain.configure >@@ -1000,12 +1000,12 @@ include('windows.configure', when=is_win > # ============================================================== > > option('--enable-hardening', env='MOZ_SECURITY_HARDENING', > help='Enables security hardening compiler options') > > @depends('--enable-hardening', c_compiler) > def security_hardening_cflags(value, c_compiler): > if value and c_compiler.type in ['gcc', 'clang']: >- return '-fstack-protector-strong' >+ return ['-fstack-protector-strong', '-D_FORTIFY_SOURCE=2'] > > add_old_configure_assignment('HARDENING_CFLAGS', security_hardening_cflags) > imply_option('--enable-pie', depends_if('--enable-hardening')(lambda v: v))

OK, seems I need to upload a new patch. I was hoping I just can edit it.

OK, I've applied the review comments. Could somebody put this on try run to sort out any performance issues, please? I don't have access to the try server. But I checked local that it builds (on macOS with Apple Clang).

Tom: is comment 7 directed at you, or could you redirect it?

Try run with full unit tests to see if this breaks anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f688cf575f02f3d543f03c3ba060ec148ac85716 Try run for talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33d3a4b99d1713461795808c819293693f751091 Talos: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=33d3a4b99d1713461795808c819293693f751091

(In reply to Tom Ritter [:tjr] from comment #9) > Try run with full unit tests to see if this breaks anything: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=f688cf575f02f3d543f03c3ba060ec148ac85716 > > Try run for talos: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=33d3a4b99d1713461795808c819293693f751091 > Talos: > https://treeherder.mozilla.org/perf.html#/ > comparechooser?newProject=try&newRevision=33d3a4b99d1713461795808c819293693f7 > 51091 Thank you so much! Looks good, does it?

Okay, I think I cleared up the warnings-as-errors, third time's a charm? https://treeherder.mozilla.org/#/jobs?repo=try&revision=735960651681145410715aaa2b9baa3f0a99f370 https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b6e61e33c57c84bb6d3a1ccb3d2f0455742f896 https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=3b6e61e33c57c84bb6d3a1ccb3d2f0455742f896

Thanks for clearing up the warnings. Now that Bug 1406687 has checked in, is it OK if I set the checkin-needed flag?

(In reply to Nomis101 from comment #13) > Thanks for clearing up the warnings. Now that Bug 1406687 has checked in, is > it OK if I set the checkin-needed flag? I'd like to try and get someone from the performance team to look at the Talos results. If we can land this by default (and not hidden behind a flag) that would be better than putting it behind a flag we don't use on shipped releases.

adding myself/:igoldan here as there could be perf changes when this lands :)

OSX is still coming in, hopefully will be in when you check tomorrow, but the new linux results look... like there's barely any change. :) https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16

overall this looks to have no significant or measurable changes. while many of the data points show changes, they are low confidence, or some are medium confidence- this means they have outliers or a higher stddev and overlap in large portions with the data points from mozilla-central. A few looked sort of interesting: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16&framework=1&filter=responsiveness%20linux%20pgo&selectedTimeRange=172800 * as those are pgo specific, there isn't much to do other than change the pgo profiling script or remove certain blocks of code from the profiling script tp6 youtube on osx: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16&framework=1&filter=tp6%20youtube%20osx&selectedTimeRange=172800 * this might be a real regression tp6 amazon opt: linux e10s, osx stylo-disabled e10s: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=38aca2eb60945c80f1d80d788e2ddc6cec3acc16&framework=1&filter=tp6%20amazon%20opt&selectedTimeRange=172800 * these have a higher chance of being a regression. to be honest nothing is jumping out as a must look at given the noise levels.

I realized I had omitted it from js/ I have a run here I was hoping you could look at also: https://treeherder.mozilla.org/#/jobs?repo=try&revision=482d2433fe40ca1a618270a19a876ce1a762c590&selectedJob=138110086 https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=482d2433fe40ca1a618270a19a876ce1a762c590&framework=1&showOnlyImportant=1&selectedTimeRange=172800 I will prepare a patch that turns this on by default for everything except js/ (and maybe for js/ too depending on what you think) and flag you for review.

the green results (improvements) are all looking valid, I am more concerned with a11y and speedometer as those look to be real regressions. Speedometer is one of the Firefox 57 key benchmarks we tracked, so there might be more concerns with regressing that so much- but look at the subtests there- if you can figure out why the deletingitems/async subtests are causing regressions- there are a lot of other improvements. I do like a lot of the improvements we see overall, maybe a little bit of work for big gains.

wait, I probably spoke too soon, speedometer recently (2 days ago) improved for linux64-qr and the base of the try push is 3 days old. So speedometer is posting at parity with the previous data.

Comment on attachment 8920018 [details] Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default I am not really qualified to review this- I am not familiar with those files which are changed- I suspect the build team will be better qualified- but the talos numbers seem to be stable or improved :)

Comment on attachment 8920018 [details] Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default https://reviewboard.mozilla.org/r/190996/#review197294 Assuming this is talos-neutral, and maybe even if it's not, this is fine.

Comment on attachment 8920018 [details] Bug 1359908 Add FORTIFY_SOURCE to gcc and clang builds by default Sigh, mozreview.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/057166cb35fe Add FORTIFY_SOURCE to gcc and clang builds by default r=froydnj

Thanks Nomis101 for your work on this!

(In reply to Tom Ritter [:tjr] from comment #28) > Thanks Nomis101 for your work on this! You are welcome. Thanks for moving it forward.

The following line gives an error in Windows/mozilla-build environments. https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/old-configure.in#503 > INFO - z:/build/build/src/old-configure: line 5045: test: too many arguments The configure continues after this and build succeeds, but we should probably fix this. I believe the change needed is: s/-o test/-o/

Filed as Bug 1414067