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

Commit Message

William Roberts Dec. 13, 2018, 9:32 p.m. UTC

From: William Roberts <[e-mail]>

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.

Signed-off-by: William Roberts <[e-mail]>
---
 libselinux/src/Makefile   | 2 +-
 libselinux/utils/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Stephen Smalley Dec. 14, 2018, 1:43 p.m. UTC | #1

On 12/13/18 4:32 PM, [e-mail] wrote:
> From: William Roberts <[e-mail]>
> 
> 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.

> 
> Signed-off-by: William Roberts <[e-mail]>
> ---
>   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 \
>

Stephen Smalley Dec. 14, 2018, 2:34 p.m. UTC | #2

On 12/14/18 8:43 AM, Stephen Smalley wrote:
> On 12/13/18 4:32 PM, [e-mail] wrote:
>> From: William Roberts <[e-mail]>
>>
>> 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 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.

> 
>>
>> Signed-off-by: William Roberts <[e-mail]>
>> ---
>>   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 \
>>
>

William Roberts Dec. 14, 2018, 4:02 p.m. UTC | #3

On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
>
> On 12/14/18 8:43 AM, Stephen Smalley wrote:
> > On 12/13/18 4:32 PM, [e-mail] wrote:
> >> From: William Roberts <[e-mail]>
> >>
> >> 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'

>
> >
> >>
> >> Signed-off-by: William Roberts <[e-mail]>
> >> ---
> >>   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 \
> >>
> >
>

William Roberts Dec. 18, 2018, 4:03 p.m. UTC | #4

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.

On Fri, Dec 14, 2018 at 8:02 AM William Roberts
<[e-mail]> wrote:
>
> On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
> >
> > On 12/14/18 8:43 AM, Stephen Smalley wrote:
> > > On 12/13/18 4:32 PM, [e-mail] wrote:
> > >> From: William Roberts <[e-mail]>
> > >>
> > >> 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'
>
> >
> > >
> > >>
> > >> Signed-off-by: William Roberts <[e-mail]>
> > >> ---
> > >>   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 \
> > >>
> > >
> >

William Roberts Dec. 18, 2018, 7:02 p.m. UTC | #5

+ Eric and Dan, as Stephen pointed out to me, they introduced the -Wp
portion in commit 41649ca78

On Tue, Dec 18, 2018 at 8:03 AM William Roberts
<[e-mail]> 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.
>
> On Fri, Dec 14, 2018 at 8:02 AM William Roberts
> <[e-mail]> wrote:
> >
> > On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
> > >
> > > On 12/14/18 8:43 AM, Stephen Smalley wrote:
> > > > On 12/13/18 4:32 PM, [e-mail] wrote:
> > > >> From: William Roberts <[e-mail]>
> > > >>
> > > >> 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'
> >
> > >
> > > >
> > > >>
> > > >> Signed-off-by: William Roberts <[e-mail]>
> > > >> ---
> > > >>   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 \
> > > >>
> > > >
> > >

Jason Zaman Dec. 19, 2018, 6:15 a.m. UTC | #6

On Tue, Dec 18, 2018 at 11:02:54AM -0800, William Roberts wrote:
> + Eric and Dan, as Stephen pointed out to me, they introduced the -Wp
> portion in commit 41649ca78
> 
> On Tue, Dec 18, 2018 at 8:03 AM William Roberts
> <[e-mail]> 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.

Just wanted to say that in Gentoo Hardened our toolchain also already
has _FORTIFY_SOURCE set to 2. If anything we should set it to 2 not 1.
and if setting it, we gotta add the undefine first otherwise we get
this:

$ gcc -Werror -O2 -D_FORTIFY_SOURCE=2 main.c 
<command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]
<built-in>: note: this is the location of the previous definition
<command-line>:0:17: error: expected identifier or ‘(’ before numeric constant
main.c:3:1: note: in expansion of macro ‘_FORTIFY_SOURCE’
 _FORTIFY_SOURCE
 ^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Its not really a big deal since basically everyone sets their own CFLAGS
but =2 is safer so we should have it as the default.

-- Jason

> >
> > On Fri, Dec 14, 2018 at 8:02 AM William Roberts
> > <[e-mail]> wrote:
> > >
> > > On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
> > > >
> > > > On 12/14/18 8:43 AM, Stephen Smalley wrote:
> > > > > On 12/13/18 4:32 PM, [e-mail] wrote:
> > > > >> From: William Roberts <[e-mail]>
> > > > >>
> > > > >> 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'
> > >
> > > >
> > > > >
> > > > >>
> > > > >> Signed-off-by: William Roberts <[e-mail]>
> > > > >> ---
> > > > >>   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 \
> > > > >>
> > > > >
> > > >

Patrick Steinhardt Dec. 19, 2018, 9:12 a.m. UTC | #7

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
> <[e-mail]> wrote:
> >
> > On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
> > >
> > > On 12/14/18 8:43 AM, Stephen Smalley wrote:
> > > > On 12/13/18 4:32 PM, [e-mail] wrote:
> > > >> From: William Roberts <[e-mail]>
> > > >>
> > > >> 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'
> >
> > >
> > > >
> > > >>
> > > >> Signed-off-by: William Roberts <[e-mail]>
> > > >> ---
> > > >>   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 \

William Roberts Dec. 19, 2018, 3:42 p.m. UTC | #8

On Wed, Dec 19, 2018 at 1:12 AM Patrick Steinhardt <[e-mail]> 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
> > <[e-mail]> wrote:
> > >
> > > On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
> > > >
> > > > On 12/14/18 8:43 AM, Stephen Smalley wrote:
> > > > > On 12/13/18 4:32 PM, [e-mail] wrote:
> > > > >> From: William Roberts <[e-mail]>
> > > > >>
> > > > >> 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.

> > >
> > > >
> > > > >
> > > > >>
> > > > >> Signed-off-by: William Roberts <[e-mail]>
> > > > >> ---
> > > > >>   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 \

Stephen Smalley Dec. 19, 2018, 3:46 p.m. UTC | #9

On 12/19/18 10:42 AM, William Roberts wrote:
> On Wed, Dec 19, 2018 at 1:12 AM Patrick Steinhardt <[e-mail]> 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
>>> <[e-mail]> wrote:
>>>>
>>>> On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
>>>>>
>>>>> On 12/14/18 8:43 AM, Stephen Smalley wrote:
>>>>>> On 12/13/18 4:32 PM, [e-mail] wrote:
>>>>>>> From: William Roberts <[e-mail]>
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>> ---
>>>>>>>    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 \

William Roberts Dec. 19, 2018, 3:48 p.m. UTC | #10

On Wed, Dec 19, 2018 at 7:44 AM Stephen Smalley <[e-mail]> wrote:
>
> On 12/19/18 10:42 AM, William Roberts wrote:
> > On Wed, Dec 19, 2018 at 1:12 AM Patrick Steinhardt <[e-mail]> 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
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <[e-mail]> wrote:
> >>>>>
> >>>>> On 12/14/18 8:43 AM, Stephen Smalley wrote:
> >>>>>> On 12/13/18 4:32 PM, [e-mail] wrote:
> >>>>>>> From: William Roberts <[e-mail]>
> >>>>>>>
> >>>>>>> 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 <[e-mail]>
> >>>>>>> ---
> >>>>>>>    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 \
>

Patch

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 \