From: William Roberts <bill.c.roberts@gmail.com>
To: Stephen Smalley <sds@tycho.nsa.gov>,
ps@pks.im, berrange@redhat.com,
Eric Paris <eparis@parisplace.org>,
Eric Paris <eparis@redhat.com>
Cc: selinux@vger.kernel.org,
William Roberts <william.c.roberts@intel.com>,
James Carter <jwcart2@tycho.nsa.gov>
Subject: Re: [PATCH 1/2] Makefile: fix _FORTIFY_SOURCE redefined build error
Date: Tue, 18 Dec 2018 11:02:54 -0800 [thread overview]
Message-ID: <CAFftDdoMFfjn=2yB-qXhwH=TonH=42pBNbczkVVwvQtr7UZn9w@mail.gmail.com> (raw)
In-Reply-To: <CAFftDdr+1zWpGeBgpfhQ02fbfhCD2F5_F0mUTB2zQobKpANg8g@mail.gmail.com>
+ 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 \
> > > >>
> > > >
> > >
next prev parent reply other threads:[~2018-12-18 19:03 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFftDdoMFfjn=2yB-qXhwH=TonH=42pBNbczkVVwvQtr7UZn9w@mail.gmail.com' \
--to=bill.c.roberts@gmail.com \
--cc=berrange@redhat.com \
--cc=eparis@parisplace.org \
--cc=eparis@redhat.com \
--cc=jwcart2@tycho.nsa.gov \
--cc=ps@pks.im \
--cc=sds@tycho.nsa.gov \
--cc=selinux@vger.kernel.org \
--cc=william.c.roberts@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).