don't set _FORTIFY_SOURCE in configure

[rnpnr]

distributions that want this flag set do so on a system wide level. for example Gentoo, Fedora, Debian, and OpenSUSE.

since vis sets it when invoking cc via make it overwrites the system setting (and pollutes the output with redefinition warnings).

For reference here is the related bug in Gentoo: https://bugs.gentoo.org/892960

Below is context for the discussion that follows:

Gentoo, Fedora, Debian and likely others add their own _FORTIFY_SOURCE declaration. At least for Gentoo this is in the compiler's default flags (now _FORTIFY_SOURCE=3) so the vis build command line overwrites it.

This commit allows packagers to skip the addition of _FORTIFY_SOURCE via the configure script so that their own define is properly applied. The default remains unchanged.

For reference here is the related bug in Gentoo: https://bugs.gentoo.org/892960

Note: This patch is relatively dumb. It doesn't stop _FORTIFY_SOURCE from being added via any pkg-config commands. When I tried on my Arch system earlier TRE's pkg-config just added its own _FORTIFY_SOURCE=2. I think this might just be an issue on Arch's end though because I didn't have that problem with TRE on Gentoo.

[tosch42]

Hi Randy,

this is indeed a good observation, thanks! However, I think your approach is a bit static. What about something like the following? This would allow the user to freely choose whatever he/she wants.

diff --git a/Makefile b/Makefile
index c86264361b62..2717794d5a27 100644
--- a/Makefile
+++ b/Makefile
@@ -53,7 +53,7 @@ CFLAGS_LIBC ?= -DHAVE_MEMRCHR=0

 CFLAGS_VIS = $(CFLAGS_AUTO) $(CFLAGS_TERMKEY) $(CFLAGS_CURSES) $(CFLAGS_ACL) \
    $(CFLAGS_SELINUX) $(CFLAGS_TRE) $(CFLAGS_LUA) $(CFLAGS_LPEG) $(CFLAGS_STD) \
-   $(CFLAGS_LIBC)
+   $(CFLAGS_HARDENED) $(CFLAGS_LIBC)

 CFLAGS_VIS += -DVIS_PATH=\"${SHAREPREFIX}/vis\"
 CFLAGS_VIS += -DCONFIG_HELP=${CONFIG_HELP}
diff --git a/configure b/configure
index 1e72e2bdb3bf..27bb862d99b4 100755
--- a/configure
+++ b/configure
@@ -30,6 +30,7 @@ Optional features:
   --enable-tre            build with TRE regex support [auto]
   --enable-selinux        build with SELinux support [auto]
   --enable-acl            build with POSIX ACL support [auto]
+  --hardened-lvl          build with specicied _FORTIFY_SOURCE level [2]
   --enable-help           build with built-in help texts [yes]

 Some influential environment variables:
@@ -120,6 +121,7 @@ MANDIR='$(PREFIX)/share/man'

 help=yes
 curses=yes
+hardened=2
 lua=auto
 lpeg=auto
 tre=auto
@@ -151,6 +153,9 @@ case "$arg" in
 --disable-selinux|--enable-selinux=no) selinux=no ;;
 --enable-acl|--enable-acl=yes) acl=yes ;;
 --disable-acl|--enable-acl=no) acl=no ;;
+--hardened-lvl=1) hardened=1 ;;
+--hardened-lvl=2) hardened=2 ;;
+--hardened-lvl=3) hardened=3 ;;
 --enable-*|--disable-*|--with-*|--without-*|--*dir=*|--build=*) ;;
 -* ) echo "$0: unknown option $arg" ;;
 CC=*) CC=${arg#*=} ;;
@@ -220,7 +225,7 @@ tryflag   CFLAGS_TRY  -Werror=unused-command-line-argument
 tryldflag LDFLAGS_TRY -Werror=unknown-warning-option
 tryldflag LDFLAGS_TRY -Werror=unused-command-line-argument

-CFLAGS_STD="-std=c99 -D_POSIX_C_SOURCE=200809L -U_XOPEN_SOURCE -D_XOPEN_SOURCE=700 -DNDEBUG -D_FORTIFY_SOURCE=2"
+CFLAGS_STD="-std=c99 -D_POSIX_C_SOURCE=200809L -U_XOPEN_SOURCE -D_XOPEN_SOURCE=700 -DNDEBUG"
 LDFLAGS_STD="-lc"

 OS=$(uname)
@@ -294,6 +299,17 @@ else
    CONFIG_HELP=0
 fi

+if test "$hardened" = "1" ; then
+   CFLAGS_HARDENED="-D_FORTIFY_SOURCE=1"
+elif test "$hardened" = "2" ; then
+   CFLAGS_HARDENED="-D_FORTIFY_SOURCE=2"
+elif test "$hardened" = "3" ; then
+   CFLAGS_HARDENED="-D_FORTIFY_SOURCE=3"
+else
+   printf "Invalid hardened level"
+   exit 1
+fi
+
 CONFIG_CURSES=0

 if test "$curses" != "no" ; then
@@ -655,6 +671,7 @@ LDFLAGS_ACL = $LDFLAGS_ACL
 CONFIG_SELINUX = $CONFIG_SELINUX
 CFLAGS_SELINUX = $CFLAGS_SELINUX
 LDFLAGS_SELINUX = $LDFLAGS_SELINUX
+CFLAGS_HARDENED = $CFLAGS_HARDENED
 CFLAGS_LIBC = -DHAVE_MEMRCHR=$HAVE_MEMRCHR
 EOF
 exec 1>&3 3>&-

I tested this only very briefly and I already see some improvements that could be made. But I think it gets the idea across :-)

[rnpnr]

Hi Tom,

Thanks for the feedback, I will push an updated version.

I still think its a good idea from a packaging perspective to have an option that doesn't set _FORTIFY_SOURCE. I will include it with --hardened-lvl=0

[tosch42]

Your right and your improvements are pretty much the ones I had in mind. I wonder if we should keep a variation of this option that does not take arguments. i.e. --hardened-lvl emits an error. This is expected behavior, but I'm unsure if it would confuse users. On the other hand, just having an --hardened-lvl option which magically sets _FORTIFY_SOURCE=2 might be even more confusing. Generally, I think your patch is good as-is and I have no problem with it. If you could send a git-format-patch(1)'ed diff to git.sr.ht, that would be even better :-)

@ninewise @mcepl do you guys have any opinions? worries? errors?

LGTM

[mcepl]

My opinion is that this feature is mostly useless and partially harmful. Useless, because every distribution/system (in the loose meaning of those words) have some system CFLAGS which should be applied. They are created usually after a long discussion by people much more smarter than your average users/packager/application developer, and they are used rigorously for all C packages in the system (e.g., in SUSE/openSUSE we have CFLAGS='-O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -flto=auto -g -fcommon'). So, it seems to me that this option is useless (because there are some system CFLAGS already on all reasonable systems, not sure about Windows, but it shouldn’t be our problem anyway) and they are harmful (because application developer or even user should not mess with them).

[rnpnr]

In that case I am also completely fine with a patch like this:

diff --git a/configure b/configure
index 1e72e2b..7baf3bf 100755
--- a/configure
+++ b/configure
@@ -220,7 +223,7 @@ tryflag   CFLAGS_TRY  -Werror=unused-command-line-argument
 tryldflag LDFLAGS_TRY -Werror=unknown-warning-option
 tryldflag LDFLAGS_TRY -Werror=unused-command-line-argument

-CFLAGS_STD="-std=c99 -D_POSIX_C_SOURCE=200809L -U_XOPEN_SOURCE -D_XOPEN_SOURCE=700 -DNDEBUG -D_FORTIFY_SOURCE=2"
+CFLAGS_STD="-std=c99 -D_POSIX_C_SOURCE=200809L -U_XOPEN_SOURCE -D_XOPEN_SOURCE=700 -DNDEBUG"
 LDFLAGS_STD="-lc"

 OS=$(uname)

I didn't really know why _FORTIFY_SOURCE was being set in the default build flags so I made the original patch around being the least disruptive. But I also agree that it makes more sense to have it set globally at a system wide level rather than having individual applications setting it.

The whole purpose of the original patch was to stop vis from clobbering the system setting.

[mcepl]

I didn't really know why _FORTIFY_SOURCE was being set in the default build flags so I made the original patch around being the least disruptive.

That’s one Linux distribution (although I would think it is part of the standard CFLAGS on most systems, but I dont’ have any evidence of it).

[rnpnr]

Let me summarize:

Gentoo** sets _FORTIFY_SOURCE in the default compiler flags, i.e. you run cc with no flags and it is already set. If you explicitly set _FORTIFY_SOURCE, as vis is doing now via its configure script, it overwrites the default flags. You as a packager now have to ship a patch to stop vis from doing this. This has only recently become a problem because distros have been switching to _FORTIFY_SOURCE=3 with gcc-12 becoming standard.

** I'd be surprised to find that this wasn't the same for every other distro that sets _FORTIFY_SOURCE by default.

[mcepl]

Gentoo** sets _FORTIFY_SOURCE in the default compiler flags, i.e. you run cc with no flags and it is already set. If you explicitly set _FORTIFY_SOURCE, as vis is doing now via its configure script, it overwrites the default flags. You as a packager now have to ship a patch to stop vis from doing this. This has only recently become a problem because distros have been switching to _FORTIFY_SOURCE=3 with gcc-12 becoming standard.

** I'd be surprised to find that this wasn't the same for every other distro that sets _FORTIFY_SOURCE by default.

Exactly, I think fiddling with CFLAGS is just not something lowly application like vis should play with.

[tosch42]

As I said (or at least thought) I don't have strong opinions on this. So in conclusion, fiddling with CFLAGSshould be avoided. I'm not a packager so I don't know of all the problems you might encounter. However, Randy's second patch looks good then. no?

[rnpnr]

I pushed an updated commit which only removes -D_FORTIFY_SOURCE=2 from CFLAGS_STD. The commit message was updated to reflect the discussion here.

[ninewise]

I'm not familiar with this flag, so I'm not sure what to reply. It seems to me this is indeed good for package managers. However, I wonder, is removing the flag not a worse default for the vis users compiling vis themselves? (Which I suspect is the majority)

[mcepl]

However, I wonder, is removing the flag not a worse default for the vis users compiling vis themselves?

The point is that a regular application should not fiddle with it at all. It should go, IMHO.

[tosch42]

I slept over this and I'm divided on the topic. If we completely remove the ability for the user to choose the _FORTIFY_SOURCE level, it feels like a restriction to me. How should I, as an ordinary user, am able to set _FORTIFY_SOURCE=1? I think I prefer the approach I sent first (with additional zero-level ofc). If this is somehow too inconvenient for users/packagers, shout at me :-)

As for @mcepl's comment, a "regular application" will never mess with those flags since it doesn't compile from source. Is there something I oversee?

[mcepl]

How you are going to set it?

export CFLAGS="something _FORTIFY_SOURCE=1"
./configure --something-else
make

What’s the problem?

[rnpnr]

Putting on my bikeshedding hat:

I don't really think this flag is something vis should worry about for a couple of reasons:

1. vis isn't a long running system daemon nor can it connect to the network. The types of bugs _FORTIFY_SOURCE attempts to guard against will just lead to a SEGFAULT.
2. Directly from features_test_macros(7): "some conforming programs might fail." This means that the flag could insert exit conditions into otherwise correct parts of the program which is arguably worse from an end user standpoint.

[tosch42]

How you are going to set it?

With the newly introduced flags from the top of the discussion (improved, of course).

export CFLAGS="something _FORTIFY_SOURCE=1"
./configure --something-else
make

What’s the problem?

I just think that it's convenient to have a flag for such an option. Given all the other CFLAGS this would override, it's not practical. disabling _FORTIFY_SOURCE with a flag is much more convenient. Or am I misunderstanding your answer completely?

[mcepl]

I just think that it's convenient to have a flag for such an option. Given all the other CFLAGS this would override, it's not practical. disabling _FORTIFY_SOURCE with a flag is much more convenient. Or am I misunderstanding your answer completely?

Take a look at the openSUSE CFLAGS I shown above? Do you need a flag for each of these flags? And why should your configure bother with them at all? Also, practically, in my SPEC file (kind of a recipe for building openSUSE packages) I have this:

export CFLAGS="%{optflags} -fcommon"
%configure
%make_build debug

because %{optflags} (i.e., standard value of CFLAGS for openSUSE) are defined by our system.

[tosch42]

Take a look at the openSUSE CFLAGS I shown above? Do you need a flag for each of these flags? And why should your configure bother with them at all? Also, practically, in my SPEC file (kind of a recipe for building openSUSE packages) I have this:

I think _FORTIFY_SOURCE is special here. For packagers, it might be more convenient to just set their CFLAGS, but if I'm a user who likes to set _FORTIFY_SOURCE=1 I'd have to either gather all of vis' CFLAGS plus _FORTIFY_SOURCE=1 or edit the Makefile by hand. I think this is cumbersome and would fit in our configure script perfectly.

export CFLAGS="%{optflags} -fcommon"
%configure
%make_build debug

because %{optflags} (i.e., standard value of CFLAGS for openSUSE) are defined by our system.

The problem here is that you see it out of a packagers perspective. I on the other side, represent the user. By removing this flag we loose flexibility. How am I supposed to set _FORTIFY_SOURCE in a straight forward way? (rhetorical question) The minority of users have a system as yours and the majority builds vis themself. So do we restrict flexibility or not? Is it even that much of a restriction?

Don't get me wrong, generally I think fiddling with CFLAGS is not good. In this case, it just makes sense to me. Honestly, I'm fine with both approaches and if enough people are for one approach, I'd happily accept it :-)

[mcepl]

The problem here is that you see it out of a packagers perspective. I on the other side, represent the user. By removing this flag we loose flexibility. How am I supposed to set _FORTIFY_SOURCE in a straight forward way? (rhetorical question) The minority of users have a system as yours and the majority builds vis themself. So do we restrict flexibility or not? Is it even that much of a restriction?

What’s wrong with:

make CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2"

[tosch42]

If you mean that $CFLAGS is an environmental variable on your system, it's not on mine. So I only get _FORTIFY_SOURCE=2 as my CFLAGS. (I use Void Linux, if that interests you)

[tosch42]

I think we just need some more opinions on this. Both of us have very different views coming from the things we do. If ten packagers desperately want what you want, then sure, remove it from the configure script (well, the funny thing is it isn't even there yet x). But the other way around should also be possible.

So give us opinions!! :-)

[mcepl]

Frankly, … no I give a damn, but I don’t care that much. I will rewrite it in the end anyway.

[rnpnr]

If you mean that $CFLAGS is an environmental variable on your system, it's not on mine. So I only get _FORTIFY_SOURCE=2 as my CFLAGS. (I use Void Linux, if that interests you)

Setting CFLAGS as part of the environment doesn't remove any of vis's preexisting CFLAGS. So if you run:

env CFLAGS="-D_FORTIFY_SOURCE=2" ./configure

-D_FORTIFY_SOURCE=2 just gets appended to the start of the default flags. If you could do the same thing to remove the default CFLAGS this patch would be completely unnecessary.

EDIT: any of vis's preexisting CFLAGS.

[mcepl]

Actually looking at my logs again, I see that the additional _FORTIFY_SOURCE is not that altogether harmless. I get many

<command-line>: warning: "_FORTIFY_SOURCE" redefined

lines in gcc output.

[tosch42]

If you mean that $CFLAGS is an environmental variable on your system, it's not on mine. So I only get _FORTIFY_SOURCE=2 as my CFLAGS. (I use Void Linux, if that interests you)

Setting CFLAGS as part of the environment doesn't remove any of vis's preexisting CFLAGS. So if you run:

env CFLAGS="-D_FORTIFY_SOURCE=2" ./configure

-D_FORTIFY_SOURCE=2 just gets appended to the start of the default flags. If you could do the same thing to remove the default CFLAGS this patch would be completely unnecessary.

EDIT: any of vis's preexisting CFLAGS.

I was responding to his message saying

make CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2"

and that would override vis's CFLAGS and left me with _FORTIFY_SOURCE=2 because, well, I don't have a variable called CFLAGS on my system.

EDIT: quoting is weird

[mcepl]

If you mean that $CFLAGS is an environmental variable on your system, it's not on mine. So I only get _FORTIFY_SOURCE=2 as my CFLAGS. (I use Void Linux, if that interests you) I was responding to his message saying

make CFLAGS="$CFLAGS -D_FORTIFY_SOURCE=2"

and that would override vis's CFLAGS and left me with _FORTIFY_SOURCE=2 because, well, I don't have a variable called CFLAGS on my system.

Yes, it should be make CFLAGS="-D_FORTIFY_SOURCE=2" and existence of $CFLAGS on your system shouldn’t matter, because if I am not mistaken undefined variable expands to nothing, doesn’t it? Oh, you have set -u in your build system?

[tosch42]

Yes, but I'd loose all the other flags with which vis normally compiles. That's my point.

EDIT: Github is not able to properly add a quoted reply, so I removed that half-backed stuff

[rnpnr]

I don't really understand the problem. If you run the configure command I posted and then run make without any CFLAGS=... you get the same behavior as vis currently has. If you run make CFLAGS=... with or without my patch applied the behavior doesn't change.

[tosch42]

I think it's cumbersome, but I also think that I got overruled here. Further discussion is pointless unless a fair amount of people step in and share my opinion.

[mcepl]

@ninewise Just to say that I think this is ready to be merged as it is now.

[tosch42]

I agree and have to apologize. I had made some local changes to my Makefile, so ignore the last few comments from me. They're simply not true.

[ninewise]

Applying. Thanks for the patch and for packaging vis! Again, thanks for the comments.