Re: [PATCH v3 02/16] Exclude routines from fortification

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-07-03 11:16, Frederic Berat wrote:
> 
> 
> On Fri, Jun 30, 2023 at 4:55 PM Siddhesh Poyarekar <siddhesh@gotplt.org 
> <mailto:siddhesh@gotplt.org>> wrote:
> 
>     On 2023-06-28 04:42, Frédéric Bérat wrote:
>      > Since the _FORTIFY_SOURCE feature uses some routines of Glibc,
>     they need to
>      > be excluded from the fortification.
>      >
>      > On top of that:
>      >   - some tests explicitly verify that some level of fortification
>     works
>      >     appropriately, we therefore shouldn't modify the level set
>     for them.
>      >   - some objects need to be build with optimization disabled, which
>      >     prevents _FORTIFY_SOURCE to be used for them.
>      >
>      > Assembler files that implement architecture specific versions of the
>      > fortified routines were not excluded from _FORTIFY_SOURCE as
>     there is no
>      > C header included that would impact their behavior.
>      > ---
>      >   debug/Makefile                              | 12 +--
>      >   io/Makefile                                 | 16 ++++
>      >   libio/Makefile                              | 21 +++++-
>      >   login/Makefile                              |  6 ++
>      >   misc/Makefile                               |  7 ++
>      >   posix/Makefile                              | 11 +++
>      >   rt/Makefile                                 |  5 ++
>      >   setjmp/Makefile                             |  9 +++
>      >   socket/Makefile                             |  6 ++
>      >   stdio-common/Makefile                       | 15 +++-
>      >   stdlib/Makefile                             |  7 ++
>      >   string/Makefile                             | 17 +++++
>      >   sysdeps/ieee754/ldbl-128ibm-compat/Makefile | 81
>     +++++++++++++++++----
>      >   sysdeps/ieee754/ldbl-opt/Makefile           | 29 ++++++++
>      >   sysdeps/pthread/Makefile                    |  4 +
>      >   sysdeps/unix/sysv/linux/Makefile            |  3 +
>      >   wcsmbs/Makefile                             | 23 +++++-
>      >   17 files changed, 247 insertions(+), 25 deletions(-)
>      >
>      > diff --git a/debug/Makefile b/debug/Makefile
>      > index 9d658e3002..434e52f780 100644
>      > --- a/debug/Makefile
>      > +++ b/debug/Makefile
>      > @@ -171,13 +171,13 @@ CFLAGS-recvfrom_chk.c += -fexceptions
>     -fasynchronous-unwind-tables
>      >   # set up for us, so keep the CFLAGS/CPPFLAGS split logical as
>     the order is:
>      >   # <user CFLAGS> <test CFLAGS> <user CPPFLAGS> <test CPPFLAGS>
>      >   CFLAGS-tst-longjmp_chk.c += -fexceptions
>     -fasynchronous-unwind-tables
>      > -CPPFLAGS-tst-longjmp_chk.c += -D_FORTIFY_SOURCE=1
>      > +CPPFLAGS-tst-longjmp_chk.c +=
>     $(no-fortify-source),-D_FORTIFY_SOURCE=1
>      >   CFLAGS-tst-longjmp_chk2.c += -fexceptions
>     -fasynchronous-unwind-tables
>      > -CPPFLAGS-tst-longjmp_chk2.c += -D_FORTIFY_SOURCE=1
>      > +CPPFLAGS-tst-longjmp_chk2.c +=
>     $(no-fortify-source),-D_FORTIFY_SOURCE=1
>      >   CFLAGS-tst-longjmp_chk3.c += -fexceptions
>     -fasynchronous-unwind-tables
>      > -CPPFLAGS-tst-longjmp_chk3.c += -D_FORTIFY_SOURCE=1
>      > -CPPFLAGS-tst-realpath-chk.c += -D_FORTIFY_SOURCE=2
>      > -CPPFLAGS-tst-chk-cancel.c += -D_FORTIFY_SOURCE=2
>      > +CPPFLAGS-tst-longjmp_chk3.c +=
>     $(no-fortify-source),-D_FORTIFY_SOURCE=1
>      > +CPPFLAGS-tst-realpath-chk.c +=
>     $(no-fortify-source),-D_FORTIFY_SOURCE=2
>      > +CPPFLAGS-tst-chk-cancel.c +=
>     $(no-fortify-source),-D_FORTIFY_SOURCE=2
>      >
>      >   # _FORTIFY_SOURCE tests.
>      >   # Auto-generate tests for _FORTIFY_SOURCE for different levels,
>     compilers and
>      > @@ -215,7 +215,7 @@ src-chk-nongnu = \#undef _GNU_SOURCE
>      >   # cannot be disabled via pragmas, so require -Wno-error to be used.
>      >   define gen-chk-test
>      >   tests-$(1)-$(4)-chk += tst-fortify-$(1)-$(2)-$(3)-$(4)
>      > -CFLAGS-tst-fortify-$(1)-$(2)-$(3)-$(4).$(1) +=
>     -D_FORTIFY_SOURCE=$(3) -Wno-format \
>      > +CFLAGS-tst-fortify-$(1)-$(2)-$(3)-$(4).$(1) +=
>     $(no-fortify-source),-D_FORTIFY_SOURCE=$(3) -Wno-format \
>      >                                       
>       -Wno-deprecated-declarations \
>      >                                         -Wno-error
>      >   $(eval $(call cflags-$(2),$(1),$(3),$(4)))
>      > diff --git a/io/Makefile b/io/Makefile
>      > index d573064ecc..6ccc0e8691 100644
>      > --- a/io/Makefile
>      > +++ b/io/Makefile
>      > @@ -149,6 +149,22 @@ routines := \
>      >     write \
>      >     # routines
>      >
>      > +# Exclude fortified routines from being built with _FORTIFY_SOURCE
>      > +routines_no_fortify += \
>      > +  getcwd \
>      > +  getwd \
>      > +  open \
>      > +  open64 \
>      > +  openat \
>      > +  openat64 \
>      > +  poll \
>      > +  ppoll \
>      > +  read \
>      > +  readlink \
>      > +  readlinkat \
>      > +  ttyname_r \
>      > +  # routines_no_fortify
>      > +
>      >   others := \
>      >    pwd \
>      >    # others
>      > diff --git a/libio/Makefile b/libio/Makefile
>      > index 2877fec484..f5c487d9f5 100644
>      > --- a/libio/Makefile
>      > +++ b/libio/Makefile
>      > @@ -53,6 +53,21 @@ routines   :=                                 
>                                \
>      >
>      >   gen-as-const-headers += libio-macros.sym
>      >
>      > +# Exclude fortified routines from being built with _FORTIFY_SOURCE
>      > +routines_no_fortify += \
>      > +  fwprintf \
>      > +  iofgets \
>      > +  iofgets_u \
>      > +  iofgetws \
>      > +  iofgetws_u \
>      > +  swprintf \
>      > +  vasprintf \
>      > +  vsnprintf \
>      > +  vswprintf \
>      > +  vwprintf \
>      > +  wprintf \
>      > +  # routines_no_fortify
>      > +
>      >   tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf
>     tst_getwc tst_putwc   \
>      >       tst_wprintf2 tst-widetext test-fmemopen tst-ext tst-ext2 \
>      >       tst-fgetws tst-ungetwc1 tst-ungetwc2 tst-swscanf
>     tst-sscanf           \
>      > @@ -165,11 +180,15 @@ CFLAGS-iofgets_u.c +=
>     $(config-cflags-wno-ignored-attributes)
>      >   CFLAGS-iofputs_u.c += $(config-cflags-wno-ignored-attributes)
>      >   # XXX Do we need filedoalloc and wfiledoalloc?  Others?
>      >
>      > +# Prevent fortification as these are built with -O0
>      > +CFLAGS-tst-bz24051.c += $(no-fortify-source)
>      > +CFLAGS-tst-bz24153.c += $(no-fortify-source)
>      > +
>      >   CFLAGS-tst_putwc.c += -DOBJPFX=\"$(objpfx)\"
>      >
>      >   # These test cases intentionally use overlapping arguments
>      >   CFLAGS-tst-sprintf-ub.c += -Wno-restrict
> 
>     This should also be built without fortification because the test
>     specifically tries to validate the sprintf entry point; the
>     __sprintf_chk entry point ought to get checked by the
>     tst-sprintf-chk-ub.c test.
> 
>     In fact, I wonder if *all* tests should be built without fortification
>     by default regardless of whether glibc is built with fortification.  We
>     have specific tests in debug/ to test the _chk entry points and it
>     seems
>     like the tests should stick to validating only the regular entry points
>     unless otherwise specified.
> 
> I'm not so sure.  The fact that fortification is enabled doesn't 
> diminish the validity of the tests, at the very end fortified function 
> shouldn't modify the behavior of these routines (modulo the additional 
> tests on input parameters).
> Unless the test breaks because of fortification (like when tests 
> voluntarily mess with input parameters in a way that the test aborts on 
> chk routines), I don't see the need to undefine _FORTIFY_SOURCE.
> 
> Thus, by having fortification enabled during the tests, I could catch 
> errors in the tests (e.g. Incorrect maxlen parameter for swprintf 
> 427dbaee86bcec31ba2fe9a42f32842cf17c4e77).
> 
> On top of that in the current configuration, assuming 
> "--enable-fortify-source" is **not** set, and the _FORTIFY_SOURCE macro 
> is **not** set through the environment neither, these are still tested 
> without fortification.
> In one sense, having the glibc CI testing the entry points directly, 
> while the community will probably test with fortification, may help 
> catch unwanted behavioral changes (if that ever happens) due to 
> incorrect check routines implementation.
> 
> All of that said, we may need to reconsider the tests like 
> tst-sprintf-chk-ub.c though, considering the capability to enable 
> fortification from configure.
> 
> What do you think ?

OK that's fine, just that we'd need to do further fixups to tests in 
future where we're testing fortified and unfortified variants.

Sid