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

[Stephen Smalley <sds@tycho.nsa.gov>]

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...

> 
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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 \