On 12/19/18 10:42 AM, William Roberts wrote:
On Wed, Dec 19, 2018 at 1:12 AM Patrick Steinhardt <ps@xxxxxx> 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@xxxxxxxxx> wrote:
On Fri, Dec 14, 2018 at 6:32 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
On 12/14/18 8:43 AM, Stephen Smalley wrote:
On 12/13/18 4:32 PM, bill.c.roberts@xxxxxxxxx wrote:
From: William Roberts <william.c.roberts@xxxxxxxxx>
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@xxxxxxxxx>
---
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 \
