Closed Bug 1414067 Opened 7 years ago Closed 7 years ago

Fortify Source test has a bad comparison, throws warning

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(1 file)

Bug 1414067 Fix the compiler test for FORTIFY_SOURCE
59 bytes, text/x-review-board-request
glandium
: review+
Details 
From https://bugzilla.mozilla.org/show_bug.cgi?id=1359908#c31

> INFO -  z:/build/build/src/old-configure: line 5045: test: too many arguments
Comment on attachment 8924727 [details]
Bug 1414067 Fix the compiler test for FORTIFY_SOURCE

https://reviewboard.mozilla.org/r/195950/#review201202
Attachment #8924727 - Flags: review?(mh+mozilla) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f8eae610782a
Fix the compiler test for FORTIFY_SOURCE r=glandium
Keywords: checkin-needed
Backed out for Android build bustage at media/libstagefright/system/core/include/cutils/properties.h:61: use of undeclared identifier 'PROP_VALUE_MAX':

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f8eae610782afc8212877b0c3301ca0c31e6dbf7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141979595&repo=autoland

[task 2017-11-03T16:26:03.854Z] 16:26:03     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/media/libstagefright/Unified_cpp_media_libstagefright1.cpp:38:
[task 2017-11-03T16:26:03.855Z] 16:26:03     INFO -  In file included from /builds/worker/workspace/build/src/media/libstagefright/frameworks/av/media/libstagefright/Utils.cpp:25:
[task 2017-11-03T16:26:03.855Z] 16:26:03     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers/cutils/properties.h:3:
[task 2017-11-03T16:26:03.855Z] 16:26:03     INFO -  /builds/worker/workspace/build/src/media/libstagefright/system/core/include/cutils/properties.h:61:15: error: use of undeclared identifier 'PROP_VALUE_MAX'
[task 2017-11-03T16:26:03.855Z] 16:26:03     INFO -      if (bos < PROPERTY_VALUE_MAX) {
[task 2017-11-03T16:26:03.856Z] 16:26:03     INFO -                ^
[task 2017-11-03T16:26:03.856Z] 16:26:03     INFO -  /builds/worker/workspace/build/src/media/libstagefright/system/core/include/cutils/properties.h:35:29: note: expanded from macro 'PROPERTY_VALUE_MAX'
[task 2017-11-03T16:26:03.857Z] 16:26:03     INFO -  #define PROPERTY_VALUE_MAX  PROP_VALUE_MAX
[task 2017-11-03T16:26:03.857Z] 16:26:03     INFO -                              ^
[task 2017-11-03T16:26:03.857Z] 16:26:03     INFO -  1 error generated.
Flags: needinfo?(tom)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/daca25aef92c
Backed out changeset f8eae610782a for Android build bustage at media/libstagefright/system/core/include/cutils/properties.h:61: use of undeclared identifier 'PROP_VALUE_MAX'. r=backout on a CLOSED TREE
This also breaks the Linux x64 asan fuzzing opt build: https://treeherder.mozilla.org/logviewer.html#?job_id=141979548&repo=autoland

tools/fuzzing/libfuzzer/FuzzerIOPosix.cpp:118:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
I think that means:
 * "--enable-hardening" doesn't actually do anything right now.
 * With this bug fixed, it does actually turn on the flags that it's meant to turn on. (and it's activated in this ASAN build via "--enable-pie" which implies "--enable-hardening" AFAICT)
 * With its build-flags actually activated, we error out because our increased strictness makes us catch a legitimate-looking issue in FuzzerIOPosix.cpp.  (ignoring the return value of a "write()" invocation)
(In reply to Daniel Holbert [:dholbert] from comment #8)
> I think that means:

Close :)

>  * "--enable-hardening" doesn't actually do anything right now.

--enable-hardening turns on stack protector=strong

>  * With this bug fixed, it does actually turn on the flags that it's meant
> to turn on. (and it's activated in this ASAN build via "--enable-pie" which
> implies "--enable-hardening" AFAICT)

With this bug fix we turn on FORTIFY_SOURCE on all builds by default. We do not turn on --enable-hardening

>  * With its build-flags actually activated, we error out because our
> increased strictness makes us catch a legitimate-looking issue in
> FuzzerIOPosix.cpp.  (ignoring the return value of a "write()" invocation)

Yea, this is a legit warning that FORTIFY_SOURCE turns into an error. I had resolved a few of these but apparently didn't test the ASAN builds.




The unknown question is why this breaks Android. PROP_VALUE_MAX is defined in system_properties.h, so I suspect it has to do with libstagefright's special blank file; but why this only occurs under FORTIFY_SOURCE will take some digging.
Flags: needinfo?(tom)
Okay, traced this down.

Problem 1: PROP_VALUE_MAX is not defined inside the libstagefright directory. This is because we set up some empty stub files (I'm not sure why.) I can resolve this with this type of patch: https://hg.mozilla.org/try/rev/13e7ccbe49ff


Problem 2: With FORTIFY_SOURCE enabled, the android ndk defines a macro for snprintf (only in AArch64) in android-ndk/platforms/android-21/arch-arm64/usr/include/stdio.h.  You can see some details of here:
https://treeherder.mozilla.org/logviewer.html#?job_id=142520083&repo=try&lineNumber=8646
https://pastebin.mozilla.org/9072222 (line 394)

In the tree, we have our own functions named snprintf (at least two, below) and the macro clobbers it in a way that doesn't work.

http://searchfox.org/mozilla-central/source/ipc/chromium/src/base/string_util.h#55
http://searchfox.org/mozilla-central/source/xpcom/string/nsTextFormatter.h#54

I might be able to resolve these using #ifdef snprintf / #undef snprintf



But in general both of these problems have a better solution than the 'hacky' solution, and I'm not sure exactly what it is. So I'll propose we turn off FORTIFY_SOURCE for Android and then file bugs with these details to re-enable it.
Flags: needinfo?(nfroyd)
(In reply to Tom Ritter [:tjr] from comment #10)
> Problem 1: PROP_VALUE_MAX is not defined inside the libstagefright
> directory. This is because we set up some empty stub files (I'm not sure
> why.) I can resolve this with this type of patch:
> https://hg.mozilla.org/try/rev/13e7ccbe49ff

Ralph, do you know the history of why we do this?

> Problem 2: With FORTIFY_SOURCE enabled, the android ndk defines a macro for
> snprintf (only in AArch64) in
> android-ndk/platforms/android-21/arch-arm64/usr/include/stdio.h.  You can
> see some details of here:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=142520083&repo=try&lineNumber=8646
> https://pastebin.mozilla.org/9072222 (line 394)
> 
> In the tree, we have our own functions named snprintf (at least two, below)
> and the macro clobbers it in a way that doesn't work.
> 
> http://searchfox.org/mozilla-central/source/ipc/chromium/src/base/
> string_util.h#55
> http://searchfox.org/mozilla-central/source/xpcom/string/nsTextFormatter.h#54
> 
> I might be able to resolve these using #ifdef snprintf / #undef snprintf

Yes, fixing those wouldn't be very much fun.

> But in general both of these problems have a better solution than the
> 'hacky' solution, and I'm not sure exactly what it is. So I'll propose we
> turn off FORTIFY_SOURCE for Android and then file bugs with these details to
> re-enable it.

This seems reasonable to me.  We could turn it off specifically for AArch64 Android, too...though I would suspect other architectures would eventually get similar treatment as AArch64?  ni? to snorp just to make sure he doesn't have strong opinions on doing Android support at a later date.
Flags: needinfo?(snorp)
Flags: needinfo?(nfroyd)
Flags: needinfo?(giles)
> I can resolve this with this type of patch: https://hg.mozilla.org/try/rev/13e7ccbe49ff

I think the idea here was to stop #include errors for headers we don't actually need without too much noise when updating from upstream. We don't do that any more, so any fix is fine. Your patch looks reasonable to me. r+.

We want to remove this code, so you could also disable just the offending #ifdef _FORTIFY_SOURCE code on Android. It won't been a long-term testing hole.

Alfredo, what's the timeline for removing stagefright? Can we do that sooner, since it's causing problems here?
Flags: needinfo?(giles) → needinfo?(ayang)
Yes, I plan to complete it as soon as possible, if there is no other bug to interrupt me.

https://bugzilla.mozilla.org/show_bug.cgi?id=1408298
Flags: needinfo?(ayang)
Okay, here's a try run with it fixed and disabled on Android for every Build I could fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6016476a0d8c1ef650e4f81533ec14957ef999da

Requesting re-review because I don't think I can clear a r+ in MozReview, let me know if it looks okay and I'll land it.

Note that it is intentional that it only disables it in one old-configure.in and not the one in js/src - we can get the feature there without breakage, so we take what we can get.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8924727 [details]
Bug 1414067 Fix the compiler test for FORTIFY_SOURCE

https://reviewboard.mozilla.org/r/195950/#review203176

::: old-configure.in:509
(Diff revision 2)
> +*-android*|*-linuxandroid*)
> +    dnl FORTIFY_SOURCE is not supported on Android at this time
> +    dnl See Bug 1415595
> +    ;;
> +*)
> +    if test "$GNU_CC" -o -n "${CLANG_CC}${CLANG_CL}"; then

I'd wrap the conditions the other way around. That is, first test the compiler, then the target. Also, you should just test whether OS_TARGET is Android, rather than test $target.
Attachment #8924727 - Flags: review+
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd [:froydnj] from comment #11)
> 
> > But in general both of these problems have a better solution than the
> > 'hacky' solution, and I'm not sure exactly what it is. So I'll propose we
> > turn off FORTIFY_SOURCE for Android and then file bugs with these details to
> > re-enable it.
> 
> This seems reasonable to me.  We could turn it off specifically for AArch64
> Android, too...though I would suspect other architectures would eventually
> get similar treatment as AArch64?  ni? to snorp just to make sure he doesn't
> have strong opinions on doing Android support at a later date.

I would love to have it on for Android, but we have no plans currently.
Flags: needinfo?(snorp)
Comment on attachment 8924727 [details]
Bug 1414067 Fix the compiler test for FORTIFY_SOURCE

https://reviewboard.mozilla.org/r/195950/#review204372

::: old-configure.in:504
(Diff revision 3)
> +    case $OS_TARGET in
> +    Android)
> +         dnl FORTIFY_SOURCE is not supported on Android at this time
> +         dnl See Bug 1415595
> +         ;;

Seems this should be applied to js/src/old-configure.in as well.
Attachment #8924727 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 8924727 [details]
> Bug 1414067 Fix the compiler test for FORTIFY_SOURCE
> 
> https://reviewboard.mozilla.org/r/195950/#review204372
> 
> ::: old-configure.in:504
> (Diff revision 3)
> > +    case $OS_TARGET in
> > +    Android)
> > +         dnl FORTIFY_SOURCE is not supported on Android at this time
> > +         dnl See Bug 1415595
> > +         ;;
> 
> Seems this should be applied to js/src/old-configure.in as well.

We do actually want to enable it on js/ on Android (because we can, it works). Unless you mean I should put a comment there referencing that it is applied on Android there but not on the top-level config.
Flags: needinfo?(mh+mozilla)
Yeah, you should add a comment explaining that FORTIFY_SOURCE *does* work on Android for js.
Flags: needinfo?(mh+mozilla)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d240b19cc50c
Fix the compiler test for FORTIFY_SOURCE r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d240b19cc50c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: