Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error

[William Roberts <bill.c.roberts@gmail.com>]

On Wed, Dec 19, 2018 at 7:44 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 12/19/18 10:42 AM, William Roberts wrote:
> > On Wed, Dec 19, 2018 at 1:12 AM Patrick Steinhardt <ps@pks.im> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, Dec 18, 2018 at 08:03:54AM -0800, William Roberts wrote:
> >>> Patrick,
> >>>
> >>> Hoping you could maybe weigh in on your choice for bypassing the
> >>> compiler driver with -Wp and not setting _FORTIFY_SOURCE to something
> >>> like 1 or 2?
> >>>
> >>> I'm seeing this issue on Ubuntu 16.04.5:
> >>> <command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]
> >>>
> >>> gcc version:
> >>> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609
> >>>
> >>> My thought is to undef/redef _FORTIFY_SOURCE in CFLAGS and set the
> >>> level to 2. Setting CFLAGS via the env/make arg will override this
> >>> behavior
> >>> and use CFLAGS as is.
> >>
> >> I used "-Wp" simply because it was existing previously, so I just
> >> stuck to what was there already. The original issue I had was
> >> that Gentoo Hardened, as Jason notes, already defines
> >> _FORTIFY_SOURCE as part of the compiler spec. Due to that, I was
> >> seeing a lot of warnings.
> >>
> >> So I set the flag to a simple define without setting a specific
> >> value, which _seemed_ to let the issue go away. But going back to
> >> the initial issue, this didn't seem to have solved it correctly.
> >> Dunno what I've been doing back then to not see the warnings
> >> after my change anymore, but I noticed that they have resurface
> >> recently.
> >>
> >> So I guess the real fix would be to redefine the value by first
> >> undef'ing it and then redefining it to the desired value. And I
> >> do agree that in that case, we should simply revert to
> >> _FORTIFY_SOURCE=2.
> >>
> >> Patrick
> >>
> >>> On Fri, Dec 14, 2018 at 8:02 AM William Roberts
> >>> <bill.c.roberts@gmail.com> wrote:
> >>>>
> >>>> On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >>>>>
> >>>>> On 12/14/18 8:43 AM, Stephen Smalley wrote:
> >>>>>> On 12/13/18 4:32 PM, bill.c.roberts@gmail.com wrote:
> >>>>>>> From: William Roberts <william.c.roberts@intel.com>
> >>>>>>>
> >>>>>>> Certain builds of gcc enable _FORTIFY_SOURCE which results in the error:
> >>>>>>> <command-line>:0:0: warning: "_FORTIFY_SOURCE" redefined
> >>>>>>> <command-line>:0:0: note: this is the location of the previous definition
> >>>>>>>
> >>>>>>> Correct this by undefining it first and redefining it. Also, the previous
> >>>>>>> command line option was using -Wp which passing the value *AS IS* to the
> >>>>>>> pre-processor rather than to the compiler driver. The C pre-processor has
> >>>>>>> an undocumented interface subject to change per man 1 gcc. Just use the
> >>>>>>> -D option as is.
> >>>>>>
> >>>>>> See commit ca07a2ad46be141dad90d885dd33a2ac31c6559a ("libselinux: avoid
> >>>>>> redefining _FORTIFY_SOURCE") for why we don't specify a value for
> >>>>>> _FORTIFY_SOURCE here.  Not sure about the -Wp,-D vs -D rationale.
> >>>>
> >>>> I'm not 100% convinced that the patch is the best solution or the commit message
> >>>> is describing the problem correctly. I could also be understanding it
> >>>> wrong here.
> >>>> The man page is saying not to bypass the compiler driver via -Wp, and I don't
> >>>> see a good reason for it either.
> >>>>
> >>>> See my comments below, they feed back into this.
> >>>>
> >>>>>
> >>>>> I guess the issue here is that we want to provide sane defaults for
> >>>>> building without breaking the build when others specify their own
> >>>>> definitions and without weakening those definitions.  By undefining and
> >>>>> re-defining, it seems like we might weaken existing builds that were
> >>>>> specifying 2.
> >>>>
> >>>> We conditionally assign to CFLAGS via ?= operator. Thus, CFLAGS and the
> >>>> corresponding addition of EXTRA_CFLAGS which contains the undef/def
> >>>> is not appended. CFLAGS specified via the environment or as an argument to
> >>>> make will cause this assignment not to occur (via ?= semantics) and whatever
> >>>> they specify for CFLAGS is sent to CC.
> >>>>
> >>>> Here is some sample output:
> >>>> make CFLAGS='-D_FORTIFY_SOURCE=2'
> >>>> cc -D_FORTIFY_SOURCE=2 -I../include -D_GNU_SOURCE -DNO_ANDROID_BACKEND
> >>>>   -fPIC -DSHARED -c -o stringrep.lo stringrep.c
> >>>>
> >>>> With that said, *i think its safe* to bump it back to '-D_FORTIFY_SOURCE=2'
> >
> > I agree that 2 is the better value. I'll re-roll these to use 2. No
> > one seems to be complaining
> > on using the -D/-U options.
>
> The only alternative that I could see would be to just drop
> FORTIFY_SOURCE altogether from the upstream Makefile and defer setting
> of it entirely to the packagers...

I like having a sane set of defaults and I think _FORTIFY_SOURCE=2 is
inline with it.
I guess we can try moving back to 2, and if someone complains, just
stop setting it.

>
> >
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> >>>>>>> ---
> >>>>>>>    libselinux/src/Makefile   | 2 +-
> >>>>>>>    libselinux/utils/Makefile | 2 +-
> >>>>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> >>>>>>> index 977b5c8cfcca..ee55bd0dbff7 100644
> >>>>>>> --- a/libselinux/src/Makefile
> >>>>>>> +++ b/libselinux/src/Makefile
> >>>>>>> @@ -64,7 +64,7 @@ ifeq ($(COMPILER), gcc)
> >>>>>>>    EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op
> >>>>>>> -Wpacked-bitfield-compat -Wsync-nand \
> >>>>>>>        -Wcoverage-mismatch -Wcpp -Wformat-contains-nul -Wnormalized=nfc
> >>>>>>> -Wsuggest-attribute=const \
> >>>>>>>        -Wsuggest-attribute=noreturn -Wsuggest-attribute=pure
> >>>>>>> -Wtrampolines -Wjump-misses-init \
> >>>>>>> -    -Wno-suggest-attribute=pure -Wno-suggest-attribute=const
> >>>>>>> -Wp,-D_FORTIFY_SOURCE
> >>>>>>> +    -Wno-suggest-attribute=pure -Wno-suggest-attribute=const
> >>>>>>> -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
> >>>>>>>    else
> >>>>>>>    EXTRA_CFLAGS = -Wunused-command-line-argument
> >>>>>>>    endif
> >>>>>>> diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
> >>>>>>> index d06ffd66893b..64ab877015c6 100644
> >>>>>>> --- a/libselinux/utils/Makefile
> >>>>>>> +++ b/libselinux/utils/Makefile
> >>>>>>> @@ -30,7 +30,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k
> >>>>>>> -Wformat-security -Winit-self -Wmissi
> >>>>>>>              -Wformat-extra-args -Wformat-zero-length -Wformat=2
> >>>>>>> -Wmultichar \
> >>>>>>>              -Woverflow -Wpointer-to-int-cast -Wpragmas \
> >>>>>>>              -Wno-missing-field-initializers -Wno-sign-compare \
> >>>>>>> -          -Wno-format-nonliteral
> >>>>>>> -Wframe-larger-than=$(MAX_STACK_SIZE) -Wp,-D_FORTIFY_SOURCE \
> >>>>>>> +          -Wno-format-nonliteral
> >>>>>>> -Wframe-larger-than=$(MAX_STACK_SIZE) -U_FORTIFY_SOURCE
> >>>>>>> -D_FORTIFY_SOURCE=1 \
> >>>>>>>              -fstack-protector-all --param=ssp-buffer-size=4
> >>>>>>> -fexceptions \
> >>>>>>>              -fasynchronous-unwind-tables -fdiagnostics-show-option
> >>>>>>> -funit-at-a-time \
> >>>>>>>              -Werror -Wno-aggregate-return -Wno-redundant-decls \
>