diff mbox series

Enable _FORTIFY_SOURCE=3 for gcc 12 and above

Message ID 20211217040753.4176265-1-siddhesh@sourceware.org
State Committed
Commit 86bf0feb0e3ec8e37872f72499d6ae33406561d7
Headers show
Series Enable _FORTIFY_SOURCE=3 for gcc 12 and above | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Siddhesh Poyarekar Dec. 17, 2021, 4:07 a.m. UTC
gcc 12 now has support for the __builtin_dynamic_object_size builtin.
Adapt the macro checks to enable _FORTIFY_SOURCE=3 on gcc 12 and above.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 include/features.h | 4 +++-
 misc/sys/cdefs.h   | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Adhemerval Zanella Dec. 17, 2021, 1:17 p.m. UTC | #1
On 17/12/2021 01:07, Siddhesh Poyarekar via Libc-alpha wrote:
> gcc 12 now has support for the __builtin_dynamic_object_size builtin.
> Adapt the macro checks to enable _FORTIFY_SOURCE=3 on gcc 12 and above.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

With gcc version 12.0.0 20211217 (experimental) [master r12-6038-g411ac94611f]
I am seeing two new failures:

  FAIL: debug/tst-chk7
  FAIL: debug/tst-chk8

And both explicit set _FORTIFY_SOURCE to 3.  I am not if is really related to
_FORTIFY_SOURCE=3:

__GI_getenv (name=0x7ffff7f6ec25 "CPATH", name@entry=0x7ffff7f6ec23 "LOCPATH") at get
env.c:84
84                if (name_start == ep_start && !strncmp (*ep + 2, name, len)
(gdb) bt
#0  __GI_getenv (name=0x7ffff7f6ec25 "CPATH", name@entry=0x7ffff7f6ec23 "LOCPATH")
    at getenv.c:84
#1  0x00007ffff7dd87bc in __GI_setlocale (category=category@entry=6, 
    locale=locale@entry=0x555555564195 "de_DE.UTF-8") at setlocale.c:252
#2  0x000055555555e956 in do_test ()
    at /home/azanella/Projects/glibc/glibc-git/debug/tst-chk1.c:1424
#3  0x0000555555562cd2 in support_test_main (argc=1, argv=0x7fffffffd810, 
    config=config@entry=0x7fffffffd670) at support_test_main.c:415
#4  0x0000555555559bad in main (argc=<optimized out>, argv=<optimized out>)
    at ../support/test-driver.c:170

> ---
>  include/features.h | 4 +++-
>  misc/sys/cdefs.h   | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/features.h b/include/features.h
> index d974eabfaf..933499bcff 100644
> --- a/include/features.h
> +++ b/include/features.h
> @@ -412,7 +412,9 @@
>  #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
>  # elif !__GNUC_PREREQ (4, 1)
>  #  warning _FORTIFY_SOURCE requires GCC 4.1 or later
> -# elif _FORTIFY_SOURCE > 2 && __glibc_clang_prereq (9, 0)
> +# elif _FORTIFY_SOURCE > 2 && (__glibc_clang_prereq (9, 0)		      \
> +			       || __GNUC_PREREQ (12, 0))
> +
>  #  if _FORTIFY_SOURCE > 3
>  #   warning _FORTIFY_SOURCE > 3 is treated like 3 on this platform
>  #  endif
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index a05b538579..1875e5c6ed 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -143,7 +143,8 @@
>  #define __bos0(ptr) __builtin_object_size (ptr, 0)
>  
>  /* Use __builtin_dynamic_object_size at _FORTIFY_SOURCE=3 when available.  */
> -#if __USE_FORTIFY_LEVEL == 3 && __glibc_clang_prereq (9, 0)
> +#if __USE_FORTIFY_LEVEL == 3 && (__glibc_clang_prereq (9, 0)		      \
> +				 || __GNUC_PREREQ (12, 0))
>  # define __glibc_objsize0(__o) __builtin_dynamic_object_size (__o, 0)
>  # define __glibc_objsize(__o) __builtin_dynamic_object_size (__o, 1)
>  #else
Siddhesh Poyarekar Dec. 17, 2021, 1:54 p.m. UTC | #2
On 12/17/21 18:47, Adhemerval Zanella wrote:
> On 17/12/2021 01:07, Siddhesh Poyarekar via Libc-alpha wrote:
>> gcc 12 now has support for the __builtin_dynamic_object_size builtin.
>> Adapt the macro checks to enable _FORTIFY_SOURCE=3 on gcc 12 and above.
>>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
> With gcc version 12.0.0 20211217 (experimental) [master r12-6038-g411ac94611f]
> I am seeing two new failures:
> 
>    FAIL: debug/tst-chk7
>    FAIL: debug/tst-chk8
> 
> And both explicit set _FORTIFY_SOURCE to 3.  I am not if is really related to
> _FORTIFY_SOURCE=3:
> 
> __GI_getenv (name=0x7ffff7f6ec25 "CPATH", name@entry=0x7ffff7f6ec23 "LOCPATH") at get
> env.c:84
> 84                if (name_start == ep_start && !strncmp (*ep + 2, name, len)
> (gdb) bt
> #0  __GI_getenv (name=0x7ffff7f6ec25 "CPATH", name@entry=0x7ffff7f6ec23 "LOCPATH")
>      at getenv.c:84
> #1  0x00007ffff7dd87bc in __GI_setlocale (category=category@entry=6,
>      locale=locale@entry=0x555555564195 "de_DE.UTF-8") at setlocale.c:252
> #2  0x000055555555e956 in do_test ()
>      at /home/azanella/Projects/glibc/glibc-git/debug/tst-chk1.c:1424
> #3  0x0000555555562cd2 in support_test_main (argc=1, argv=0x7fffffffd810,
>      config=config@entry=0x7fffffffd670) at support_test_main.c:415
> #4  0x0000555555559bad in main (argc=<optimized out>, argv=<optimized out>)
>      at ../support/test-driver.c:170

That looks unrelated but I'll take a look.  I've been testing with the 
full __bdos patchset and that did not fail.

Thanks,
Siddhesh
Siddhesh Poyarekar Dec. 17, 2021, 3:24 p.m. UTC | #3
On 12/17/21 19:24, Siddhesh Poyarekar wrote:
> On 12/17/21 18:47, Adhemerval Zanella wrote:
>> On 17/12/2021 01:07, Siddhesh Poyarekar via Libc-alpha wrote:
>>> gcc 12 now has support for the __builtin_dynamic_object_size builtin.
>>> Adapt the macro checks to enable _FORTIFY_SOURCE=3 on gcc 12 and above.
>>>
>>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>>
>> With gcc version 12.0.0 20211217 (experimental) [master 
>> r12-6038-g411ac94611f]
>> I am seeing two new failures:
>>
>>    FAIL: debug/tst-chk7
>>    FAIL: debug/tst-chk8
>>
>> And both explicit set _FORTIFY_SOURCE to 3.  I am not if is really 
>> related to
>> _FORTIFY_SOURCE=3:
>>
>> __GI_getenv (name=0x7ffff7f6ec25 "CPATH", name@entry=0x7ffff7f6ec23 
>> "LOCPATH") at get
>> env.c:84
>> 84                if (name_start == ep_start && !strncmp (*ep + 2, 
>> name, len)
>> (gdb) bt
>> #0  __GI_getenv (name=0x7ffff7f6ec25 "CPATH", 
>> name@entry=0x7ffff7f6ec23 "LOCPATH")
>>      at getenv.c:84
>> #1  0x00007ffff7dd87bc in __GI_setlocale (category=category@entry=6,
>>      locale=locale@entry=0x555555564195 "de_DE.UTF-8") at setlocale.c:252
>> #2  0x000055555555e956 in do_test ()
>>      at /home/azanella/Projects/glibc/glibc-git/debug/tst-chk1.c:1424
>> #3  0x0000555555562cd2 in support_test_main (argc=1, argv=0x7fffffffd810,
>>      config=config@entry=0x7fffffffd670) at support_test_main.c:415
>> #4  0x0000555555559bad in main (argc=<optimized out>, argv=<optimized 
>> out>)
>>      at ../support/test-driver.c:170
> 
> That looks unrelated but I'll take a look.  I've been testing with the 
> full __bdos patchset and that did not fail.

Sorry that was tardy of me; that's just the test failing to catch buffer 
overflows, which ends up corrupting state for later tests and 
eventually, getenv.  I should have anticipated that.

I'm basically trying to get the support in early in case the remaining 
gcc patches take longer and glibc 2.35 freezes over.  Should I xfail 
these tests for gcc for now and then revert that bit once the rest of 
the support goes in?  If it's not acceptable to push this change before 
all of the compiler support is in, then I'll settle for the possibility 
of having to wait until 2.36 development opens and then backport the 
change to 2.35.

What would you suggest?

Thanks,
Siddhesh
Adhemerval Zanella Dec. 17, 2021, 7:04 p.m. UTC | #4
On 17/12/2021 12:24, Siddhesh Poyarekar wrote:
> On 12/17/21 19:24, Siddhesh Poyarekar wrote:
>> On 12/17/21 18:47, Adhemerval Zanella wrote:
>>> On 17/12/2021 01:07, Siddhesh Poyarekar via Libc-alpha wrote:
>>>> gcc 12 now has support for the __builtin_dynamic_object_size builtin.
>>>> Adapt the macro checks to enable _FORTIFY_SOURCE=3 on gcc 12 and above.
>>>>
>>>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>>>
>>> With gcc version 12.0.0 20211217 (experimental) [master r12-6038-g411ac94611f]
>>> I am seeing two new failures:
>>>
>>>    FAIL: debug/tst-chk7
>>>    FAIL: debug/tst-chk8
>>>
>>> And both explicit set _FORTIFY_SOURCE to 3.  I am not if is really related to
>>> _FORTIFY_SOURCE=3:
>>>
>>> __GI_getenv (name=0x7ffff7f6ec25 "CPATH", name@entry=0x7ffff7f6ec23 "LOCPATH") at get
>>> env.c:84
>>> 84                if (name_start == ep_start && !strncmp (*ep + 2, name, len)
>>> (gdb) bt
>>> #0  __GI_getenv (name=0x7ffff7f6ec25 "CPATH", name@entry=0x7ffff7f6ec23 "LOCPATH")
>>>      at getenv.c:84
>>> #1  0x00007ffff7dd87bc in __GI_setlocale (category=category@entry=6,
>>>      locale=locale@entry=0x555555564195 "de_DE.UTF-8") at setlocale.c:252
>>> #2  0x000055555555e956 in do_test ()
>>>      at /home/azanella/Projects/glibc/glibc-git/debug/tst-chk1.c:1424
>>> #3  0x0000555555562cd2 in support_test_main (argc=1, argv=0x7fffffffd810,
>>>      config=config@entry=0x7fffffffd670) at support_test_main.c:415
>>> #4  0x0000555555559bad in main (argc=<optimized out>, argv=<optimized out>)
>>>      at ../support/test-driver.c:170
>>
>> That looks unrelated but I'll take a look.  I've been testing with the full __bdos patchset and that did not fail.
> 
> Sorry that was tardy of me; that's just the test failing to catch buffer overflows, which ends up corrupting state for later tests and eventually, getenv.  I should have anticipated that.
> 
> I'm basically trying to get the support in early in case the remaining gcc patches take longer and glibc 2.35 freezes over.  Should I xfail these tests for gcc for now and then revert that bit once the rest of the support goes in?  If it's not acceptable to push this change before all of the compiler support is in, then I'll settle for the possibility of having to wait until 2.36 development opens and then backport the change to 2.35.
> 
> What would you suggest?

I am not sure I am following what is happening here, is the test wrong for
_FORTIFY_SOURCE=3, our _FORTIFY_SOURCE=3 lacking some hardening, or missing
upstream gcc support?
Siddhesh Poyarekar Dec. 17, 2021, 7:30 p.m. UTC | #5
On 12/18/21 00:34, Adhemerval Zanella wrote:
> I am not sure I am following what is happening here, is the test wrong for
> _FORTIFY_SOURCE=3, our _FORTIFY_SOURCE=3 lacking some hardening, or missing
> upstream gcc support?

It's missing some support upstream.  Basically the series is this:

https://patchwork.sourceware.org/project/gcc/list/?series=5387&state=*

where I've committed 1/6 and 2/6, thus adding 
__builtin_dynamic_object_size identification to gcc.  However, the 
actual dynamic expression generation is in patches 3-6, which is why the 
tests are failing.

I may not be able to finish updating and pushing all the patches before 
glibc freeze.

Siddhesh
Adhemerval Zanella Dec. 17, 2021, 7:48 p.m. UTC | #6
On 17/12/2021 16:30, Siddhesh Poyarekar wrote:
> On 12/18/21 00:34, Adhemerval Zanella wrote:
>> I am not sure I am following what is happening here, is the test wrong for
>> _FORTIFY_SOURCE=3, our _FORTIFY_SOURCE=3 lacking some hardening, or missing
>> upstream gcc support?
> 
> It's missing some support upstream.  Basically the series is this:
> 
> https://patchwork.sourceware.org/project/gcc/list/?series=5387&state=*
> 
> where I've committed 1/6 and 2/6, thus adding __builtin_dynamic_object_size identification to gcc.  However, the actual dynamic expression generation is in patches 3-6, which is why the tests are failing.
> 
> I may not be able to finish updating and pushing all the patches before glibc freeze.

Right, it this case I am afraid we will need to hold enabling _FORTIFY_SOURCE=3 
until we get proper and complete support from compiler.
Siddhesh Poyarekar Dec. 17, 2021, 7:50 p.m. UTC | #7
On 12/18/21 01:18, Adhemerval Zanella wrote:
> 
> 
> On 17/12/2021 16:30, Siddhesh Poyarekar wrote:
>> On 12/18/21 00:34, Adhemerval Zanella wrote:
>>> I am not sure I am following what is happening here, is the test wrong for
>>> _FORTIFY_SOURCE=3, our _FORTIFY_SOURCE=3 lacking some hardening, or missing
>>> upstream gcc support?
>>
>> It's missing some support upstream.  Basically the series is this:
>>
>> https://patchwork.sourceware.org/project/gcc/list/?series=5387&state=*
>>
>> where I've committed 1/6 and 2/6, thus adding __builtin_dynamic_object_size identification to gcc.  However, the actual dynamic expression generation is in patches 3-6, which is why the tests are failing.
>>
>> I may not be able to finish updating and pushing all the patches before glibc freeze.
> 
> Right, it this case I am afraid we will need to hold enabling _FORTIFY_SOURCE=3
> until we get proper and complete support from compiler.


I agree that's fair.

Thanks,
Siddhesh
Siddhesh Poyarekar Jan. 12, 2022, 4:14 a.m. UTC | #8
On 17/12/2021 18:47, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 17/12/2021 01:07, Siddhesh Poyarekar via Libc-alpha wrote:
>> gcc 12 now has support for the __builtin_dynamic_object_size builtin.
>> Adapt the macro checks to enable _FORTIFY_SOURCE=3 on gcc 12 and above.
>>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
> With gcc version 12.0.0 20211217 (experimental) [master r12-6038-g411ac94611f]
> I am seeing two new failures:
> 
>    FAIL: debug/tst-chk7
>    FAIL: debug/tst-chk8

This should now work on gcc trunk as of r12-6482, tested with:

gcc version 12.0.0 20220112 (experimental) (GCC)

OK to commit?

Thanks,
Siddhesh
Adhemerval Zanella Jan. 12, 2022, 12:52 p.m. UTC | #9
On 12/01/2022 01:14, Siddhesh Poyarekar wrote:
> On 17/12/2021 18:47, Adhemerval Zanella via Libc-alpha wrote:
>>
>>
>> On 17/12/2021 01:07, Siddhesh Poyarekar via Libc-alpha wrote:
>>> gcc 12 now has support for the __builtin_dynamic_object_size builtin.
>>> Adapt the macro checks to enable _FORTIFY_SOURCE=3 on gcc 12 and above.
>>>
>>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>>
>> With gcc version 12.0.0 20211217 (experimental) [master r12-6038-g411ac94611f]
>> I am seeing two new failures:
>>
>>    FAIL: debug/tst-chk7
>>    FAIL: debug/tst-chk8
> 
> This should now work on gcc trunk as of r12-6482, tested with:
> 
> gcc version 12.0.0 20220112 (experimental) (GCC)
> 
> OK to commit?

Ok, I have verified that it now work with gcc 12 current.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.or>
diff mbox series

Patch

diff --git a/include/features.h b/include/features.h
index d974eabfaf..933499bcff 100644
--- a/include/features.h
+++ b/include/features.h
@@ -412,7 +412,9 @@ 
 #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
 # elif !__GNUC_PREREQ (4, 1)
 #  warning _FORTIFY_SOURCE requires GCC 4.1 or later
-# elif _FORTIFY_SOURCE > 2 && __glibc_clang_prereq (9, 0)
+# elif _FORTIFY_SOURCE > 2 && (__glibc_clang_prereq (9, 0)		      \
+			       || __GNUC_PREREQ (12, 0))
+
 #  if _FORTIFY_SOURCE > 3
 #   warning _FORTIFY_SOURCE > 3 is treated like 3 on this platform
 #  endif
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index a05b538579..1875e5c6ed 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -143,7 +143,8 @@ 
 #define __bos0(ptr) __builtin_object_size (ptr, 0)
 
 /* Use __builtin_dynamic_object_size at _FORTIFY_SOURCE=3 when available.  */
-#if __USE_FORTIFY_LEVEL == 3 && __glibc_clang_prereq (9, 0)
+#if __USE_FORTIFY_LEVEL == 3 && (__glibc_clang_prereq (9, 0)		      \
+				 || __GNUC_PREREQ (12, 0))
 # define __glibc_objsize0(__o) __builtin_dynamic_object_size (__o, 0)
 # define __glibc_objsize(__o) __builtin_dynamic_object_size (__o, 1)
 #else