Re: [PATCH 2/2] mingw: enable DEP and ASLR

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 8, 2019, 11:27 UTC
Message-ID: <nycvar.QRO.7.76.6.1905081319570.44@tvgsbejvaqbjf.bet>
In-Reply-To: <20190501220219.GA42435@google.com>

Hi Jonathan & Peff,

On Wed, 1 May 2019, Jonathan Nieder wrote:

Show 44 quoted lines

> Jeff King wrote:
>
> > I wonder if this points to this patch touching the wrong level. These
> > compiler flags are a thing that _some_ builds want (i.e., production
> > builds where people care most about security and not about debugging),
> > but not necessarily all.
> >
> > I'd have expected this to be tweakable by a Makefile knob (either a
> > specific knob, or just the caller setting the right CFLAGS etc), and
> > then for the builds of Git for Windows to turn those knobs when making a
> > package to distribute.
> >
> > Our internal package builds at GitHub all have this in their config.mak
> > (for Linux, of course):
> >
> >   CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
> >   CFLAGS += -fstack-protector-strong
> >
> >   CFLAGS += -fpie
> >   LDFLAGS += -z relro -z now
> >   LDFLAGS += -pie
> >
> > and I wouldn't be surprised if other binary distributors (like the
> > Debian package) do something similar.
>
> Yes, the Debian package uses
>
> 	CFLAGS := -Wall \
> 		$(shell dpkg-buildflags --get CFLAGS) \
> 		$(shell dpkg-buildflags --get CPPFLAGS)
>
> and then passes CFLAGS='$(CFLAGS)' to "make".
>
> That means we're using
>
> 	-g -O2 -fstack-protector-strong -Wformat -Werror=format-security
> 	-Wdate-time -D_FORTIFY_SOURCE=2
>
> Dscho's suggestion for the Windows build sounds fine to me (if
> checking for -Og, too).  Maybe it would make sense to factor out a
> makefile variable for this, that could be used for builds on other
> platforms, too.  That way, the autodetection can be in one place, and
> there is a standard way to override it when the user wants something
> else.

Indeed, if I was to add a generic "are we building for production?" function, this would be incorrect.

But this is not the case here, we are doing something very specific, Windows-only here, and for the sole reason to keep debuggability (for which the presence of the `-g` option indeed would not be a good indicator: in Git for Windows, we build `.pdb` files so that stackdumps can be more meaningful, but we do not want to have full debug information in those executables).

In the long run, I think we need to become more explicit about this, by adding a "FOR_PRODUCTION" flag. It's really no good if we use implementation details such as CFLAGS to deduce intent.

That's for another patch series, though, as it is pretty clear-cut here: If you build with optimization flags using Git for Windows' SDK, you cannot use gdb for single-stepping, likewise if you use ASLR, so we can totally piggyback the latter onto the former.

Ciao, Dscho