diff mbox series

testsuite: Fix vfprintf-chk-1.c with -fhardened

Message ID 20240215225806.2836388-1-sam@gentoo.org
State New
Headers show
Series testsuite: Fix vfprintf-chk-1.c with -fhardened | expand

Commit Message

Sam James Feb. 15, 2024, 10:53 p.m. UTC
With _FORTIFY_SOURCE >= 2 (enabled by -fhardened), vfprintf-chk-1.c's
__vfprintf_chk ends up calling __vprintf_chk rather than vprintf.

```
 
 #ifndef test
+#undef _FORTIFY_SOURCE
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>

Comments

Sam James March 12, 2024, 4:04 p.m. UTC | #1
Sam James <sam@gentoo.org> writes:

> With _FORTIFY_SOURCE >= 2 (enabled by -fhardened), vfprintf-chk-1.c's
> __vfprintf_chk ends up calling __vprintf_chk rather than vprintf.
>
> ```
> --- a/fortify.s
> +++ b/no-fortify.s
> @@ -8,27 +8,28 @@
> [...]
>  __vfprintf_chk:
> [...]
>         movl    $1, should_optimize(%rip)
> -       jmp     __vfprintf_chk
> +       jmp     vfprintf@PLT
> ```

Ping.

>
> 2024-02-15	Sam James <sam@gentoo.org>
>
> gcc/testsuite/ChangeLog:
> 	* gcc.c-torture/execute/vfprintf-chk-1.c (__vfprintf_chk): Undefine _FORTIFY_SOURCE
> 	to call the real vfprintf.
> ---
> The test, AIUI, is trying to test GCC's own basic _chk bits rather than
> any of e.g. glibc's _FORTIFY_SOURCE handling.
>
> I'm curious as to why only vfprintf triggers this right now. If this patch is right,
> perhaps we should do printf-chk-1.c, fprintf-chk-1.c, and vprintf-chk-1.
>
> Please push if OK as I don't have access.
>
>  gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c b/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c
> index 401eaf4304a4..a8e5689e3fe6 100644
> --- a/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c
> +++ b/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c
> @@ -1,6 +1,7 @@
>  /* { dg-skip-if "requires io" { freestanding } }  */
>  
>  #ifndef test
> +#undef _FORTIFY_SOURCE
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdarg.h>
Jakub Jelinek March 12, 2024, 4:19 p.m. UTC | #2
On Thu, Feb 15, 2024 at 10:53:08PM +0000, Sam James wrote:
> With _FORTIFY_SOURCE >= 2 (enabled by -fhardened), vfprintf-chk-1.c's
> __vfprintf_chk ends up calling __vprintf_chk rather than vprintf.

s/__vprintf_chk/__vfprintf_chk/ above

> 
> ```
> --- a/fortify.s
> +++ b/no-fortify.s
> @@ -8,27 +8,28 @@
> [...]
>  __vfprintf_chk:
> [...]
>         movl    $1, should_optimize(%rip)
> -       jmp     __vfprintf_chk
> +       jmp     vfprintf@PLT
> ```
> 
> 2024-02-15	Sam James <sam@gentoo.org>
> 
> gcc/testsuite/ChangeLog:
> 	* gcc.c-torture/execute/vfprintf-chk-1.c (__vfprintf_chk): Undefine _FORTIFY_SOURCE
> 	to call the real vfprintf.

I think we should change vprintf-chk-1.c as well.

> ---
> The test, AIUI, is trying to test GCC's own basic _chk bits rather than
> any of e.g. glibc's _FORTIFY_SOURCE handling.
> 
> I'm curious as to why only vfprintf triggers this right now. If this patch is right,
> perhaps we should do printf-chk-1.c, fprintf-chk-1.c, and vprintf-chk-1.

That is easy.  In the printf-chk-1.c case it calls vprintf which results in
__vfprintf_chk or __vprintf_chk calls but redefines just __printf_chk.
Similarly fprintf-chk-1.c case calls vfprintf which results in __vfprintf_chk
call which isn't redefined either.
In the __vfprintf_chk case it calls vfprintf, which the inline results in
__vfprintf_chk call and so it self-recurses instead of calling glibc for the
actual implementation.
In the vprintf-chk-1.c case, it depends.
__fortify_function int
vprintf (const char *__restrict __fmt, __gnuc_va_list __ap)
{
#ifdef __USE_EXTERN_INLINES
  return __vfprintf_chk (stdout, __USE_FORTIFY_LEVEL - 1, __fmt, __ap);
#else
  return __vprintf_chk (__USE_FORTIFY_LEVEL - 1, __fmt, __ap);
#endif
}
So, if __USE_EXTERN_INLINES is defined, it will work fine, if not, it will
also self-recurse like vfprintf-chk-1.c.

	Jakub
Xi Ruoyao March 13, 2024, 10:05 a.m. UTC | #3
On Tue, 2024-03-12 at 17:19 +0100, Jakub Jelinek wrote:
> On Thu, Feb 15, 2024 at 10:53:08PM +0000, Sam James wrote:
> > With _FORTIFY_SOURCE >= 2 (enabled by -fhardened), vfprintf-chk-1.c's
> > __vfprintf_chk ends up calling __vprintf_chk rather than vprintf.

Do we really want to support adding random CFLAGS running the test
suite?  AFAIK adding random CFLAGS will just cause test failures here or
there.  We are adjusting the test suite for -fPIE -pie and -fstack-
protector-strong but it's because they can be implicitly enabled with --
enable-default-* options, and we don't have --enable-default-hardened as
at now.

If we need to bootstrap a hardened GCC and test it, pass -fhardened as
how "info gccinstall" suggests:

make BOOT_CFLAGS="-O2 -g -fhardened"

instead of

env C{,XX}FLAGS="-O2 -g -fhardened" /path/to/gcc/configure ...

which will taint the test suite with -fhardened.
Jakub Jelinek March 13, 2024, 10:50 a.m. UTC | #4
On Wed, Mar 13, 2024 at 06:05:29PM +0800, Xi Ruoyao wrote:
> On Tue, 2024-03-12 at 17:19 +0100, Jakub Jelinek wrote:
> > On Thu, Feb 15, 2024 at 10:53:08PM +0000, Sam James wrote:
> > > With _FORTIFY_SOURCE >= 2 (enabled by -fhardened), vfprintf-chk-1.c's
> > > __vfprintf_chk ends up calling __vprintf_chk rather than vprintf.
> 
> Do we really want to support adding random CFLAGS running the test
> suite?

Random flags certainly not, but some flags should be supported and are very
useful.
We already support the various ABI changing options (-m32 -m64 -mx32 and
the like) and ISA options in there (-march=whatever, -msse2, etc.),
and testing with -fstack-protector-strong is what some distros do for years,
testing with -fhardened is desirable if pretty much everything in the
distros is built with that flag.
Another thing is using --param whatever=whatever in the target_board flags,
or -fno-tree-dce etc. that may or might not work and user needs to be
prepared there will be extra fails.

	Jakub
diff mbox series

Patch

--- a/fortify.s
+++ b/no-fortify.s
@@ -8,27 +8,28 @@ 
[...]
 __vfprintf_chk:
[...]
        movl    $1, should_optimize(%rip)
-       jmp     __vfprintf_chk
+       jmp     vfprintf@PLT
```

2024-02-15	Sam James <sam@gentoo.org>

gcc/testsuite/ChangeLog:
	* gcc.c-torture/execute/vfprintf-chk-1.c (__vfprintf_chk): Undefine _FORTIFY_SOURCE
	to call the real vfprintf.
---
The test, AIUI, is trying to test GCC's own basic _chk bits rather than
any of e.g. glibc's _FORTIFY_SOURCE handling.

I'm curious as to why only vfprintf triggers this right now. If this patch is right,
perhaps we should do printf-chk-1.c, fprintf-chk-1.c, and vprintf-chk-1.

Please push if OK as I don't have access.

 gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c b/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c
index 401eaf4304a4..a8e5689e3fe6 100644
--- a/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c
+++ b/gcc/testsuite/gcc.c-torture/execute/vfprintf-chk-1.c
@@ -1,6 +1,7 @@ 
 /* { dg-skip-if "requires io" { freestanding } }  */