1418052 - Disable FORTIFY_SOURCE if --disable-hardening is set

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Comment 1

[Tom Ritter [:tjr] (OOTO until April)]

Attachment: Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review commit: https://reviewboard.mozilla.org/r/201502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201502/

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review207240

At this point, it seems it would be less convoluted to set MOZ_HARDENING_FLAGS to whatever you want, including _FORTIFY_SOURCE, in toolchain.configure, and add it to C*FLAGS in old-configure (or even through CompileFlags).

Comment 3

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/1-2/

Comment 4

[Tom Ritter [:tjr] (OOTO until April)]

clearing review flags i want to refactor this actually.

Comment 5

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/2-3/

Comment 6

[Tom Ritter [:tjr] (OOTO until April)]

Okay, I think it's ready for review now.

Comment 7

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/3-4/

Comment 8

[Christian Holler (:decoder)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review209310

Looks good to me as long as it doesn't break any builds.

Comment 9

[Christian Holler (:decoder)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review209534

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review210066

::: build/moz.configure/toolchain.configure:1297
(Diff revision 4)
>         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']:
> +@depends('--enable-hardening', '--enable-address-sanitizer',
> +         '--enable-optimize', c_compiler, target)
> +def security_hardening_cflags(hardening_flag, asan_flag, optimize_flag, c_compiler, target):

you might as well rename the arguments to asan and optimize, and not create local variables that just copy them.

::: build/moz.configure/toolchain.configure:1298
(Diff revision 4)
>  
>  
> -@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'
> +@depends('--enable-hardening', '--enable-address-sanitizer',
> +         '--enable-optimize', c_compiler, target)
> +def security_hardening_cflags(hardening_flag, asan_flag, optimize_flag, c_compiler, target):
> +    compiler_is_gccish = c_compiler.type in ['gcc', 'clang']

('gcc', 'clang')

::: build/moz.configure/toolchain.configure:1307
(Diff revision 4)
> +
> +    flags = []
> +    js_flags = []
>  
> +    # FORTIFY_SOURCE ------------------------------------
> +    # Enable if no --x-hardening option is passed or --enable-hardening is passed

"If hardening is explicitly enabled, or not explicitly disabled" might be clearer.

::: build/moz.configure/toolchain.configure:1320
(Diff revision 4)
> +    if asan_is_on:
> +        flags.append("-U_FORTIFY_SOURCE")
> +        js_flags.append("-U_FORTIFY_SOURCE")

if hardening is enabled, then you have -U... -D... -U... in your flags.

::: build/moz.configure/toolchain.configure:1326
(Diff revision 4)
> +    if hardening_flag:
> +        if compiler_is_gccish and not asan_is_on:

you can merge both ifs.

Comment 12

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review210754

Addressed all but one comment:

::: build/moz.configure/toolchain.configure:1320
(Diff revision 4)
> +    if asan_is_on:
> +        flags.append("-U_FORTIFY_SOURCE")
> +        js_flags.append("-U_FORTIFY_SOURCE")

I don't think so, maybe you misread? I ran configure with --enable-hardening and -enable-address-sanitizer, and only see one -U as expected.

Comment 13

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/4-5/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review211754

::: build/moz.configure/toolchain.configure:1311
(Diff revision 5)
> +    if hardening_flag.origin == "default" or hardening_flag:
> +        # Require optimization for FORTIFY_SOURCE. See Bug 1417452
> +        # Also, undefine it before defining it just in case a distro adds it, see Bug 1418398
> +        if compiler_is_gccish and optimize and not asan:
> +            # Don't enable FORTIFY_SOURCE on Android on the top-level, but do enable in js/
> +            if not is_android:

you might as well use the condition directly... if target.os != 'Android':

Comment 15

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/5-6/

Comment 16

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/6-7/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ee72d1a9430
Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize r=decoder,glandium

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57d8721abda7
Backed out changeset 1ee72d1a9430 for Linux x64 asan build bustages on /builds/worker/workspace/build/src/config/recurse.mk:32 r=backout on a CLOSED TREE

Comment 19

[Cosmin Sabou [:CosminS]]

Backed out for Linux x64 asan build bustages on /builds/worker/workspace/build/src/config/recurse.mk:32

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1ee72d1a943068ec55007fbc11ae58df10522d0c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=150747344

https://treeherder.mozilla.org/logviewer.html#?job_id=150747344&repo=autoland&lineNumber=30009

https://hg.mozilla.org/integration/autoland/rev/57d8721abda7e2e8471fa353f5d9dd399d156f54

Comment 20

[Cosmin Sabou [:CosminS]]

Also please look at this fail:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1ee72d1a943068ec55007fbc11ae58df10522d0c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=150763506

https://treeherder.mozilla.org/logviewer.html#?job_id=150763506&repo=autoland&lineNumber=2631

Thank you!

Comment 21

[Tom Ritter [:tjr] (OOTO until April)]

My best guess is that the cause of this is because we are going from '-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2' for this job to just '-U_FORTIFY_SOURCE' - but I have no idea why that's a problem.

I'm going to try re-arranging the code and seeing if that makes the error go away...

Comment 22

[Tom Ritter [:tjr] (OOTO until April)]

Sure enough, moving the function fixed it.

Comment 23

[Tom Ritter [:tjr] (OOTO until April)]

Attachment: Bug 1418052 Used the wrong form for add_old_configure_assignment

If we remove -D_FORTIFY_SOURCE=2 from the Fuzzing ASAN build we get several linker errors like

 INPUT("StaticXULComponentsEnd/StaticXULComponentsEnd.o")
 ../../image/Unified_cpp_image0.o: In function `mozilla::image::ISurfaceProvider::Surface()':
 /builds/worker/workspace/build/src/image/ISurfaceProvider.h:228: undefined reference to `__sanitizer_cov_trace_pc_guard'

ISurfaceProvider::Surface() was specified in a .h, which while legal, made me suspect the
compiler/linker just couldn't get things straight. Moving it into its own .cpp file does
indeed help them figure it out.

It's one of those cases: I don't know why this makes the compiler happy, but it does.

Review commit: https://reviewboard.mozilla.org/r/212708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212708/

Comment 24

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/7-8/

Comment 25

[Tom Ritter [:tjr] (OOTO until April)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba4272dc616480123e62bc3fdf7e36410dc604d8&selectedJob=156066620

Comment 26

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

I spoke to soon (was looking at the wrong job)

Comment 27

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212708/diff/1-2/

Comment 28

[Tom Ritter [:tjr] (OOTO until April)]

Okay the problem was actually that I my initial attempt at add_old_configure_assignment was incorrect: the function needs to be written to return True (not just the val) for the old-configure assignment to be set. Without MOZ_ASAN we weren't disabling the sandbox, leading to weird compilation errors.

glandium - could you review my change? If it looks good I will fold it into the original commit, do a full try run, and then checkin just a single patch.

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

https://reviewboard.mozilla.org/r/212708/#review218678

::: build/moz.configure/toolchain.configure
(Diff revision 2)
>  # Security Hardening
>  # ==============================================================
>  
>  js_option('--enable-address-sanitizer', help='Enable Address Sanitizer')
>  
> -

Removing this line will likely make some linter barf.

::: build/moz.configure/toolchain.configure:1302
(Diff revision 2)
>  # ==============================================================
>  
>  js_option('--enable-address-sanitizer', help='Enable Address Sanitizer')
>  
> -
>  @depends('--enable-address-sanitizer')

you could use @depends_if here and not need that if in the function.

Comment 30

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212708/diff/2-3/

Comment 31

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212708/diff/3-4/

Comment 32

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212708/diff/4-5/

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

https://reviewboard.mozilla.org/r/212708/#review219602

Comment 34

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201502/diff/8-9/

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1578425fbe9f
Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize r=decoder,glandium

Comment 36

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1578425fbe9f