selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
@ 2018-12-13 21:32 bill.c.roberts
  2018-12-13 21:32 ` [PATCH 2/2] Makefile: add -Wstrict-overflow=5 to CFLAGS bill.c.roberts
  2018-12-14 13:43 ` [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error Stephen Smalley
  0 siblings, 2 replies; 12+ messages in thread
From: bill.c.roberts @ 2018-12-13 21:32 UTC (permalink / raw)
  To: selinux; +Cc: sds, William Roberts

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.

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] Makefile: add -Wstrict-overflow=5 to CFLAGS
  2018-12-13 21:32 [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error bill.c.roberts
@ 2018-12-13 21:32 ` bill.c.roberts
  2018-12-14 13:43 ` [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error Stephen Smalley
  1 sibling, 0 replies; 12+ messages in thread
From: bill.c.roberts @ 2018-12-13 21:32 UTC (permalink / raw)
  To: selinux; +Cc: sds, William Roberts

From: William Roberts <william.c.roberts@intel.com>

Build with strict overflow checking enabled. If the compiler optimizes
code that could be removed due to undefined signed overflow, then the
compiler will issue a warning.

Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
 libselinux/src/Makefile   | 3 ++-
 libselinux/utils/Makefile | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
index ee55bd0dbff7..a283ac9d90c2 100644
--- a/libselinux/src/Makefile
+++ b/libselinux/src/Makefile
@@ -64,7 +64,8 @@ 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 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
+	-Wno-suggest-attribute=pure -Wno-suggest-attribute=const -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 \
+        -Wstrict-overflow=5
 else
 EXTRA_CFLAGS = -Wunused-command-line-argument
 endif
diff --git a/libselinux/utils/Makefile b/libselinux/utils/Makefile
index 64ab877015c6..c2f59ea947cc 100644
--- a/libselinux/utils/Makefile
+++ b/libselinux/utils/Makefile
@@ -33,7 +33,7 @@ CFLAGS ?= -O -Wall -W -Wundef -Wformat-y2k -Wformat-security -Winit-self -Wmissi
           -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 \
+          -Werror -Wno-aggregate-return -Wno-redundant-decls -Wstrict-overflow=5 \
           $(EXTRA_CFLAGS)
 
 LD_SONAME_FLAGS=-soname,$(LIBSO),-z,defs,-z,relro
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-13 21:32 [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error bill.c.roberts
  2018-12-13 21:32 ` [PATCH 2/2] Makefile: add -Wstrict-overflow=5 to CFLAGS bill.c.roberts
@ 2018-12-14 13:43 ` Stephen Smalley
  2018-12-14 14:34   ` Stephen Smalley
  1 sibling, 1 reply; 12+ messages in thread
From: Stephen Smalley @ 2018-12-14 13:43 UTC (permalink / raw)
  To: bill.c.roberts, selinux; +Cc: William Roberts, James Carter

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.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-14 13:43 ` [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error Stephen Smalley
@ 2018-12-14 14:34   ` Stephen Smalley
  2018-12-14 16:02     ` William Roberts
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Smalley @ 2018-12-14 14:34 UTC (permalink / raw)
  To: bill.c.roberts, selinux; +Cc: William Roberts, James Carter

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-14 14:34   ` Stephen Smalley
@ 2018-12-14 16:02     ` William Roberts
  2018-12-18 16:03       ` William Roberts
  0 siblings, 1 reply; 12+ messages in thread
From: William Roberts @ 2018-12-14 16:02 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: selinux, William Roberts, James Carter

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'

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-14 16:02     ` William Roberts
@ 2018-12-18 16:03       ` William Roberts
  2018-12-18 19:02         ` William Roberts
  2018-12-19  9:12         ` Patrick Steinhardt
  0 siblings, 2 replies; 12+ messages in thread
From: William Roberts @ 2018-12-18 16:03 UTC (permalink / raw)
  To: Stephen Smalley, ps; +Cc: selinux, William Roberts, James Carter

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-18 16:03       ` William Roberts
@ 2018-12-18 19:02         ` William Roberts
  2018-12-19  6:15           ` Jason Zaman
  2018-12-19  9:12         ` Patrick Steinhardt
  1 sibling, 1 reply; 12+ messages in thread
From: William Roberts @ 2018-12-18 19:02 UTC (permalink / raw)
  To: Stephen Smalley, ps, berrange, Eric Paris, Eric Paris
  Cc: selinux, William Roberts, James Carter

+ 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
<bill.c.roberts@gmail.com> 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
> <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'
> >
> > >
> > > >
> > > >>
> > > >> 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 \
> > > >>
> > > >
> > >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-18 19:02         ` William Roberts
@ 2018-12-19  6:15           ` Jason Zaman
  0 siblings, 0 replies; 12+ messages in thread
From: Jason Zaman @ 2018-12-19  6:15 UTC (permalink / raw)
  To: William Roberts
  Cc: Stephen Smalley, ps, berrange, Eric Paris, Eric Paris, selinux,
	William Roberts, James Carter

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
> <bill.c.roberts@gmail.com> 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
> > <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'
> > >
> > > >
> > > > >
> > > > >>
> > > > >> 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 \
> > > > >>
> > > > >
> > > >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-18 16:03       ` William Roberts
  2018-12-18 19:02         ` William Roberts
@ 2018-12-19  9:12         ` Patrick Steinhardt
  2018-12-19 15:42           ` William Roberts
  1 sibling, 1 reply; 12+ messages in thread
From: Patrick Steinhardt @ 2018-12-19  9:12 UTC (permalink / raw)
  To: William Roberts; +Cc: Stephen Smalley, selinux, William Roberts, James Carter

[-- Attachment #1: Type: text/plain, Size: 6309 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-19  9:12         ` Patrick Steinhardt
@ 2018-12-19 15:42           ` William Roberts
  2018-12-19 15:46             ` Stephen Smalley
  0 siblings, 1 reply; 12+ messages in thread
From: William Roberts @ 2018-12-19 15:42 UTC (permalink / raw)
  To: Patrick Steinhardt
  Cc: Stephen Smalley, selinux, William Roberts, James Carter

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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-19 15:42           ` William Roberts
@ 2018-12-19 15:46             ` Stephen Smalley
  2018-12-19 15:48               ` William Roberts
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Smalley @ 2018-12-19 15:46 UTC (permalink / raw)
  To: William Roberts, Patrick Steinhardt
  Cc: selinux, William Roberts, James Carter

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 \


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
  2018-12-19 15:46             ` Stephen Smalley
@ 2018-12-19 15:48               ` William Roberts
  0 siblings, 0 replies; 12+ messages in thread
From: William Roberts @ 2018-12-19 15:48 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Patrick Steinhardt, selinux, William Roberts, James Carter

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-12-19 15:48 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-13 21:32 [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error bill.c.roberts
2018-12-13 21:32 ` [PATCH 2/2] Makefile: add -Wstrict-overflow=5 to CFLAGS bill.c.roberts
2018-12-14 13:43 ` [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error Stephen Smalley
2018-12-14 14:34   ` Stephen Smalley
2018-12-14 16:02     ` William Roberts
2018-12-18 16:03       ` William Roberts
2018-12-18 19:02         ` William Roberts
2018-12-19  6:15           ` Jason Zaman
2018-12-19  9:12         ` Patrick Steinhardt
2018-12-19 15:42           ` William Roberts
2018-12-19 15:46             ` Stephen Smalley
2018-12-19 15:48               ` William Roberts

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).