[PATCH 0/3] Fix some issues with tests when fortify is enabled

[PATCH 0/3] Fix some issues with tests when fortify is enabled

[Adhemerval Zanella]

While working on the a3090c2c98facb I saw some issues while building
some tests when fortify is enabled.  While the test-errno shows only
with old gcc 8, both bug-strncat1 and tester shows with gcc 13.1.1.

I also checked with version from 7 to 13 with different ABIs (aarch64,
arm, mips, powerpc, powerpc64, and x86_64) and they following are the
oly missing fixes.

Adhemerval Zanella (3):
  posix: Fix test-errno build with fortify enable
  string: Fix bug-strncat1 with fortify enabled
  string: Fix tester with fortify enabled

 posix/test-errno.c    |  5 ++++-
 string/bug-strncat1.c | 14 ++++++++------
 string/tester.c       | 12 ++++++++----
 3 files changed, 20 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH 1/3] posix: Fix test-errno build with fortify enable

[Adhemerval Zanella]

With gcc 11.3.1, building with -D_FORTIFY_SOURCE=2 shows:

In function ‘getgroups’,
    inlined from ‘do_test’ at test-errno.c:129:12:
../misc/sys/cdefs.h:195:6: error: argument 1 value -1 is negative
[-Werror=stringop-overflow=]
  195 |    ? __ ## f ## _alias (__VA_ARGS__)
      \
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../posix/bits/unistd.h:115:10: note: in expansion of macro
‘__glibc_fortify’
  115 |   return __glibc_fortify (getgroups, __size, sizeof (__gid_t),
      |          ^~~~~~~~~~~~~~~
../posix/bits/unistd.h: In function ‘do_test’:
../posix/bits/unistd-decl.h:135:28: note: in a call to function
‘__getgroups_alias’ declared with attribute ‘access (write_only, 2, 1)’
  135 | extern int __REDIRECT_NTH (__getgroups_alias, (int __size,
      __gid_t __list[]),
      |                            ^~~~~~~~~~~~~~~~~
../misc/sys/cdefs.h:264:6: note: in definition of macro ‘__REDIRECT_NTH’
  264 |      name proto __asm__ (__ASMNAME (#alias)) __THROW

It builds fine with gcc 12 and gcc 13.

Checked on x86_64-linux-gnu.
---
 posix/test-errno.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/posix/test-errno.c b/posix/test-errno.c
index 305bc42938..3ef711bb41 100644
--- a/posix/test-errno.c
+++ b/posix/test-errno.c
@@ -17,6 +17,10 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <libc-diag.h>
+/* Triggered by getgroup fortify wrapper.  */
+DIAG_IGNORE_NEEDS_COMMENT (6, "-Wstringop-overflow");
+
 #include <errno.h>
 #include <limits.h>
 #include <grp.h>
@@ -34,7 +38,6 @@
 #include <sys/uio.h>
 #include <unistd.h>
 #include <netinet/in.h>
-#include <libc-diag.h>
 
 /* This is not an exhaustive test: only system calls that can be
    persuaded to fail with a consistent error code and no side effects
-- 
2.34.1

[PATCH 2/3] string: Fix bug-strncat1 with fortify enabled

[Adhemerval Zanella]

If fortify is enabled, the truncated output warning is issued by
the wrapper itself:

bug-strncat1.c: In function ‘main’:
bug-strncat1.c:14:3: error: ‘__builtin___strncat_chk’ output truncated
copying 1 byte from a string of length 2 [-Werror=stringop-truncation]
   14 |   strncat (d, "\5\6", 1);
      |   ^

Checked on x86_64-linux-gnu.
---
 string/bug-strncat1.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/string/bug-strncat1.c b/string/bug-strncat1.c
index 65a7ed58c2..cdd2141191 100644
--- a/string/bug-strncat1.c
+++ b/string/bug-strncat1.c
@@ -1,9 +1,16 @@
 #undef __USE_STRING_INLINES
 #define __USE_STRING_INLINES
+#include <sys/cdefs.h>
+#include <libc-diag.h>
+#if __GNUC_PREREQ (8, 0)
+/* GCC warns about strncat truncating output; this is deliberately
+   tested here.  If fortify is enabled, it is also triggered by the
+   wrappers. */
+DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
+#endif
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <libc-diag.h>
 
 char d[3] = "\0\1\2";
 
@@ -11,11 +18,6 @@ int
 main (void)
 {
   DIAG_PUSH_NEEDS_COMMENT;
-#if __GNUC_PREREQ (8, 0)
-  /* GCC 8 warns about strncat truncating output; this is deliberately
-     tested here.  */
-  DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
-#endif
   strncat (d, "\5\6", 1);
   DIAG_POP_NEEDS_COMMENT;
   if (d[0] != '\5')
-- 
2.34.1

[PATCH 3/3] string: Fix tester with fortify enabled

[Adhemerval Zanella]

If fortify is enabled, the truncated output warning is issued by
the wrapper itself:

In function ‘strncpy’,
    inlined from ‘test_strncpy’ at tester.c:505:10:
../string/bits/string_fortified.h:95:10: error: ‘__builtin_strncpy’
destination unchanged after copying no bytes from a string of length 3
[-Werror=stringop-truncation]
   95 |   return __builtin___strncpy_chk (__dest, __src, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   96 |                                   __glibc_objsize (__dest));
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/bits/string_fortified.h:1,
                 from ../string/string.h:548,
                 from ../include/string.h:60,
                 from tester.c:33,
                 from inl-tester.c:6:
In function ‘strncpy’,
    inlined from ‘test_strncpy’ at tester.c:505:10:

Checked on x86_64-linux-gnu.
---
 string/tester.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/string/tester.c b/string/tester.c
index 8de70ad3ce..da42c72141 100644
--- a/string/tester.c
+++ b/string/tester.c
@@ -26,6 +26,14 @@
 #undef __USE_STRING_INLINES
 #endif
 
+#include <sys/cdefs.h>
+#include <libc-diag.h>
+
+/* Triggered by strncpy fortify wrapper when it is enabled.  */
+#if __GNUC_PREREQ (8, 0)
+DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
+#endif
+
 #include <errno.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -33,7 +41,6 @@
 #include <string.h>
 #include <strings.h>
 #include <fcntl.h>
-#include <libc-diag.h>
 
 /* This file tests a range of corner cases of string functions,
    including cases where truncation occurs or where sizes specified
@@ -45,9 +52,6 @@ DIAG_IGNORE_NEEDS_COMMENT (5.0, "-Wmemset-transposed-args");
 DIAG_IGNORE_NEEDS_COMMENT (9, "-Wrestrict");
 DIAG_IGNORE_NEEDS_COMMENT (7, "-Wstringop-overflow=");
 #endif
-#if __GNUC_PREREQ (8, 0)
-DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
-#endif
 #if __GNUC_PREREQ (11, 0)
 DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overread");
 #endif
-- 
2.34.1

Re: [PATCH 1/3] posix: Fix test-errno build with fortify enable

[Carlos O'Donell]

On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
> With gcc 11.3.1, building with -D_FORTIFY_SOURCE=2 shows:
> 
> In function ‘getgroups’,
>     inlined from ‘do_test’ at test-errno.c:129:12:
> ../misc/sys/cdefs.h:195:6: error: argument 1 value -1 is negative
> [-Werror=stringop-overflow=]
>   195 |    ? __ ## f ## _alias (__VA_ARGS__)
>       \
>       |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../posix/bits/unistd.h:115:10: note: in expansion of macro
> ‘__glibc_fortify’
>   115 |   return __glibc_fortify (getgroups, __size, sizeof (__gid_t),
>       |          ^~~~~~~~~~~~~~~
> ../posix/bits/unistd.h: In function ‘do_test’:
> ../posix/bits/unistd-decl.h:135:28: note: in a call to function
> ‘__getgroups_alias’ declared with attribute ‘access (write_only, 2, 1)’
>   135 | extern int __REDIRECT_NTH (__getgroups_alias, (int __size,
>       __gid_t __list[]),
>       |                            ^~~~~~~~~~~~~~~~~
> ../misc/sys/cdefs.h:264:6: note: in definition of macro ‘__REDIRECT_NTH’
>   264 |      name proto __asm__ (__ASMNAME (#alias)) __THROW
> 
> It builds fine with gcc 12 and gcc 13.
> 
> Checked on x86_64-linux-gnu.
> ---
>  posix/test-errno.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/posix/test-errno.c b/posix/test-errno.c
> index 305bc42938..3ef711bb41 100644
> --- a/posix/test-errno.c
> +++ b/posix/test-errno.c
> @@ -17,6 +17,10 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <libc-diag.h>
> +/* Triggered by getgroup fortify wrapper.  */
> +DIAG_IGNORE_NEEDS_COMMENT (6, "-Wstringop-overflow");

Why "6"? This should be the most recent version of gcc which the warnings
appears in. Shouldn't this be 11.3?

We also have some of this logic:

124 #if __GNUC_PREREQ (7, 0)
125   DIAG_PUSH_NEEDS_COMMENT;
126   /* Avoid warnings about the second (size) argument being negative.  */
127   DIAG_IGNORE_NEEDS_COMMENT (10.1, "-Wstringop-overflow");
128 #endif
129   fails |= test_wrp (EINVAL, getgroups, -1, 0);
130 #if __GNUC_PREREQ (7, 0)
131   DIAG_POP_NEEDS_COMMENT;
132 #endif

Is it the same code triggering?


> +
>  #include <errno.h>
>  #include <limits.h>
>  #include <grp.h>
> @@ -34,7 +38,6 @@
>  #include <sys/uio.h>
>  #include <unistd.h>
>  #include <netinet/in.h>
> -#include <libc-diag.h>
>  
>  /* This is not an exhaustive test: only system calls that can be
>     persuaded to fail with a consistent error code and no side effects

-- 
Cheers,
Carlos.

Re: [PATCH 2/3] string: Fix bug-strncat1 with fortify enabled

[Carlos O'Donell]

On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
> If fortify is enabled, the truncated output warning is issued by
> the wrapper itself:
> 
> bug-strncat1.c: In function ‘main’:
> bug-strncat1.c:14:3: error: ‘__builtin___strncat_chk’ output truncated
> copying 1 byte from a string of length 2 [-Werror=stringop-truncation]
>    14 |   strncat (d, "\5\6", 1);
>       |   ^
> 

I find it suspect that this triggers on line 14, but given that the whole
test is intended to test truncated output, we should disable this warning
for the whole file.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> Checked on x86_64-linux-gnu.
> ---
>  string/bug-strncat1.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/string/bug-strncat1.c b/string/bug-strncat1.c
> index 65a7ed58c2..cdd2141191 100644
> --- a/string/bug-strncat1.c
> +++ b/string/bug-strncat1.c
> @@ -1,9 +1,16 @@
>  #undef __USE_STRING_INLINES
>  #define __USE_STRING_INLINES
> +#include <sys/cdefs.h>
> +#include <libc-diag.h>
> +#if __GNUC_PREREQ (8, 0)
> +/* GCC warns about strncat truncating output; this is deliberately
> +   tested here.  If fortify is enabled, it is also triggered by the
> +   wrappers. */
> +DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");

OK. Agreed. IMO all the diagnostic control should be in the test case with
good comments like this one which talk about the issues related to the code.
I dislike these being in the Makefile because it's harder to keep both in sync.

> +#endif
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <libc-diag.h>

OK.

>  
>  char d[3] = "\0\1\2";
>  
> @@ -11,11 +18,6 @@ int
>  main (void)
>  {
>    DIAG_PUSH_NEEDS_COMMENT;
> -#if __GNUC_PREREQ (8, 0)
> -  /* GCC 8 warns about strncat truncating output; this is deliberately
> -     tested here.  */
> -  DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
> -#endif

OK.

>    strncat (d, "\5\6", 1);
>    DIAG_POP_NEEDS_COMMENT;
>    if (d[0] != '\5')

-- 
Cheers,
Carlos.

Re: [PATCH 3/3] string: Fix tester with fortify enabled

[Carlos O'Donell]

On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
> If fortify is enabled, the truncated output warning is issued by
> the wrapper itself:
> 
> In function ‘strncpy’,
>     inlined from ‘test_strncpy’ at tester.c:505:10:
> ../string/bits/string_fortified.h:95:10: error: ‘__builtin_strncpy’
> destination unchanged after copying no bytes from a string of length 3
> [-Werror=stringop-truncation]
>    95 |   return __builtin___strncpy_chk (__dest, __src, __len,
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    96 |                                   __glibc_objsize (__dest));
>       |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../include/bits/string_fortified.h:1,
>                  from ../string/string.h:548,
>                  from ../include/string.h:60,
>                  from tester.c:33,
>                  from inl-tester.c:6:
> In function ‘strncpy’,
>     inlined from ‘test_strncpy’ at tester.c:505:10:
> 
> Checked on x86_64-linux-gnu.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  string/tester.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/string/tester.c b/string/tester.c
> index 8de70ad3ce..da42c72141 100644
> --- a/string/tester.c
> +++ b/string/tester.c
> @@ -26,6 +26,14 @@
>  #undef __USE_STRING_INLINES
>  #endif
>  
> +#include <sys/cdefs.h>
> +#include <libc-diag.h>
> +
> +/* Triggered by strncpy fortify wrapper when it is enabled.  */

OK. Comment and disable.

> +#if __GNUC_PREREQ (8, 0)

OK. GCC 8 is the latest version at which the behaviour starts (noted in your cover letter).

> +DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
> +#endif
> +
>  #include <errno.h>
>  #include <stdint.h>
>  #include <stdio.h>
> @@ -33,7 +41,6 @@
>  #include <string.h>
>  #include <strings.h>
>  #include <fcntl.h>
> -#include <libc-diag.h>
>  
>  /* This file tests a range of corner cases of string functions,
>     including cases where truncation occurs or where sizes specified
> @@ -45,9 +52,6 @@ DIAG_IGNORE_NEEDS_COMMENT (5.0, "-Wmemset-transposed-args");
>  DIAG_IGNORE_NEEDS_COMMENT (9, "-Wrestrict");
>  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wstringop-overflow=");
>  #endif
> -#if __GNUC_PREREQ (8, 0)
> -DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
> -#endif

OK.

>  #if __GNUC_PREREQ (11, 0)
>  DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overread");
>  #endif

-- 
Cheers,
Carlos.

Re: [PATCH 0/3] Fix some issues with tests when fortify is enabled

[Carlos O'Donell]

On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
> While working on the a3090c2c98facb I saw some issues while building
> some tests when fortify is enabled.  While the test-errno shows only
> with old gcc 8, both bug-strncat1 and tester shows with gcc 13.1.1.
> 
> I also checked with version from 7 to 13 with different ABIs (aarch64,
> arm, mips, powerpc, powerpc64, and x86_64) and they following are the
> oly missing fixes.

Thank you very much for the thorough testing!


> Adhemerval Zanella (3):
>   posix: Fix test-errno build with fortify enable

This one looks good but there was a mismatch between the gcc version
you said triggered the failure and the one commented in the code.
If you clarify that we can get this committed.

>   string: Fix bug-strncat1 with fortify enabled
>   string: Fix tester with fortify enabled

These two look good to me.

> 
>  posix/test-errno.c    |  5 ++++-
>  string/bug-strncat1.c | 14 ++++++++------
>  string/tester.c       | 12 ++++++++----
>  3 files changed, 20 insertions(+), 11 deletions(-)
> 

-- 
Cheers,
Carlos.

Re: [PATCH 1/3] posix: Fix test-errno build with fortify enable

[Adhemerval Zanella Netto]

On 24/07/23 09:16, Carlos O'Donell wrote:
> On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
>> With gcc 11.3.1, building with -D_FORTIFY_SOURCE=2 shows:
>>
>> In function ‘getgroups’,
>>     inlined from ‘do_test’ at test-errno.c:129:12:
>> ../misc/sys/cdefs.h:195:6: error: argument 1 value -1 is negative
>> [-Werror=stringop-overflow=]
>>   195 |    ? __ ## f ## _alias (__VA_ARGS__)
>>       \
>>       |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../posix/bits/unistd.h:115:10: note: in expansion of macro
>> ‘__glibc_fortify’
>>   115 |   return __glibc_fortify (getgroups, __size, sizeof (__gid_t),
>>       |          ^~~~~~~~~~~~~~~
>> ../posix/bits/unistd.h: In function ‘do_test’:
>> ../posix/bits/unistd-decl.h:135:28: note: in a call to function
>> ‘__getgroups_alias’ declared with attribute ‘access (write_only, 2, 1)’
>>   135 | extern int __REDIRECT_NTH (__getgroups_alias, (int __size,
>>       __gid_t __list[]),
>>       |                            ^~~~~~~~~~~~~~~~~
>> ../misc/sys/cdefs.h:264:6: note: in definition of macro ‘__REDIRECT_NTH’
>>   264 |      name proto __asm__ (__ASMNAME (#alias)) __THROW
>>
>> It builds fine with gcc 12 and gcc 13.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>>  posix/test-errno.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/posix/test-errno.c b/posix/test-errno.c
>> index 305bc42938..3ef711bb41 100644
>> --- a/posix/test-errno.c
>> +++ b/posix/test-errno.c
>> @@ -17,6 +17,10 @@
>>     License along with the GNU C Library; if not, see
>>     <https://www.gnu.org/licenses/>.  */
>>  
>> +#include <libc-diag.h>
>> +/* Triggered by getgroup fortify wrapper.  */
>> +DIAG_IGNORE_NEEDS_COMMENT (6, "-Wstringop-overflow");
> 
> Why "6"? This should be the most recent version of gcc which the warnings
> appears in. Shouldn't this be 11.3?

I am checking the current glibc build status with old gcc, and in fact this should
'7' since it is the first version -Wstringop-overflow was introduced and I did
see the failure on this version.

> 
> We also have some of this logic:
> 
> 124 #if __GNUC_PREREQ (7, 0)
> 125   DIAG_PUSH_NEEDS_COMMENT;
> 126   /* Avoid warnings about the second (size) argument being negative.  */
> 127   DIAG_IGNORE_NEEDS_COMMENT (10.1, "-Wstringop-overflow");
> 128 #endif
> 129   fails |= test_wrp (EINVAL, getgroups, -1, 0);
> 130 #if __GNUC_PREREQ (7, 0)
> 131   DIAG_POP_NEEDS_COMMENT;
> 132 #endif
> 
> Is it the same code triggering?
> 
> 
>> +
>>  #include <errno.h>
>>  #include <limits.h>
>>  #include <grp.h>
>> @@ -34,7 +38,6 @@
>>  #include <sys/uio.h>
>>  #include <unistd.h>
>>  #include <netinet/in.h>
>> -#include <libc-diag.h>
>>  
>>  /* This is not an exhaustive test: only system calls that can be
>>     persuaded to fail with a consistent error code and no side effects
> 

Re: [PATCH 2/3] string: Fix bug-strncat1 with fortify enabled

[Siddhesh Poyarekar]

On 2023-07-24 08:36, Carlos O'Donell via Libc-alpha wrote:
> On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
>> If fortify is enabled, the truncated output warning is issued by
>> the wrapper itself:
>>
>> bug-strncat1.c: In function ‘main’:
>> bug-strncat1.c:14:3: error: ‘__builtin___strncat_chk’ output truncated
>> copying 1 byte from a string of length 2 [-Werror=stringop-truncation]
>>     14 |   strncat (d, "\5\6", 1);
>>        |   ^
>>
> 
> I find it suspect that this triggers on line 14, but given that the whole
> test is intended to test truncated output, we should disable this warning
> for the whole file.
> 
> LGTM.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

In that case, wouldn't it be better to patch the Makefile instead?

Thanks,
Sid

> 
>> Checked on x86_64-linux-gnu.
>> ---
>>   string/bug-strncat1.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/string/bug-strncat1.c b/string/bug-strncat1.c
>> index 65a7ed58c2..cdd2141191 100644
>> --- a/string/bug-strncat1.c
>> +++ b/string/bug-strncat1.c
>> @@ -1,9 +1,16 @@
>>   #undef __USE_STRING_INLINES
>>   #define __USE_STRING_INLINES
>> +#include <sys/cdefs.h>
>> +#include <libc-diag.h>
>> +#if __GNUC_PREREQ (8, 0)
>> +/* GCC warns about strncat truncating output; this is deliberately
>> +   tested here.  If fortify is enabled, it is also triggered by the
>> +   wrappers. */
>> +DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
> 
> OK. Agreed. IMO all the diagnostic control should be in the test case with
> good comments like this one which talk about the issues related to the code.
> I dislike these being in the Makefile because it's harder to keep both in sync.
> 
>> +#endif
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <string.h>
>> -#include <libc-diag.h>
> 
> OK.
> 
>>   
>>   char d[3] = "\0\1\2";
>>   
>> @@ -11,11 +18,6 @@ int
>>   main (void)
>>   {
>>     DIAG_PUSH_NEEDS_COMMENT;
>> -#if __GNUC_PREREQ (8, 0)
>> -  /* GCC 8 warns about strncat truncating output; this is deliberately
>> -     tested here.  */
>> -  DIAG_IGNORE_NEEDS_COMMENT (8, "-Wstringop-truncation");
>> -#endif
> 
> OK.
> 
>>     strncat (d, "\5\6", 1);
>>     DIAG_POP_NEEDS_COMMENT;
>>     if (d[0] != '\5')
> 

Re: [PATCH 2/3] string: Fix bug-strncat1 with fortify enabled

[Adhemerval Zanella Netto]

On 24/07/23 11:24, Siddhesh Poyarekar wrote:
> On 2023-07-24 08:36, Carlos O'Donell via Libc-alpha wrote:
>> On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
>>> If fortify is enabled, the truncated output warning is issued by
>>> the wrapper itself:
>>>
>>> bug-strncat1.c: In function ‘main’:
>>> bug-strncat1.c:14:3: error: ‘__builtin___strncat_chk’ output truncated
>>> copying 1 byte from a string of length 2 [-Werror=stringop-truncation]
>>>     14 |   strncat (d, "\5\6", 1);
>>>        |   ^
>>>
>>
>> I find it suspect that this triggers on line 14, but given that the whole
>> test is intended to test truncated output, we should disable this warning
>> for the whole file.
>>
>> LGTM.
>>
>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
> In that case, wouldn't it be better to patch the Makefile instead?

My understanding of using libc-diag.h is a having an explicit mark with
additional comment so we can safely remove the suppression if/when we
move the minimum supported compiler.

Re: [PATCH 2/3] string: Fix bug-strncat1 with fortify enabled

[Siddhesh Poyarekar]

On 2023-07-24 10:26, Adhemerval Zanella Netto wrote:
> 
> 
> On 24/07/23 11:24, Siddhesh Poyarekar wrote:
>> On 2023-07-24 08:36, Carlos O'Donell via Libc-alpha wrote:
>>> On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
>>>> If fortify is enabled, the truncated output warning is issued by
>>>> the wrapper itself:
>>>>
>>>> bug-strncat1.c: In function ‘main’:
>>>> bug-strncat1.c:14:3: error: ‘__builtin___strncat_chk’ output truncated
>>>> copying 1 byte from a string of length 2 [-Werror=stringop-truncation]
>>>>      14 |   strncat (d, "\5\6", 1);
>>>>         |   ^
>>>>
>>>
>>> I find it suspect that this triggers on line 14, but given that the whole
>>> test is intended to test truncated output, we should disable this warning
>>> for the whole file.
>>>
>>> LGTM.
>>>
>>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>>
>> In that case, wouldn't it be better to patch the Makefile instead?
> 
> My understanding of using libc-diag.h is a having an explicit mark with
> additional comment so we can safely remove the suppression if/when we
> move the minimum supported compiler.
> 

OK fair enough.

Thanks,
Sid

Re: [PATCH 0/3] Fix some issues with tests when fortify is enabled

[Adhemerval Zanella Netto]

On 24/07/23 09:39, Carlos O'Donell wrote:
> On 7/21/23 08:18, Adhemerval Zanella via Libc-alpha wrote:
>> While working on the a3090c2c98facb I saw some issues while building
>> some tests when fortify is enabled.  While the test-errno shows only
>> with old gcc 8, both bug-strncat1 and tester shows with gcc 13.1.1.
>>
>> I also checked with version from 7 to 13 with different ABIs (aarch64,
>> arm, mips, powerpc, powerpc64, and x86_64) and they following are the
>> oly missing fixes.
> 
> Thank you very much for the thorough testing!
> 
> 
>> Adhemerval Zanella (3):
>>   posix: Fix test-errno build with fortify enable
> 
> This one looks good but there was a mismatch between the gcc version
> you said triggered the failure and the one commented in the code.
> If you clarify that we can get this committed.
> 
>>   string: Fix bug-strncat1 with fortify enabled
>>   string: Fix tester with fortify enabled
> 
> These two look good to me.

Thanks for the review. Do we want these for 2.38 or should we wait for 2.39?

Re: [PATCH 0/3] Fix some issues with tests when fortify is enabled

[Andreas K. Huettel]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Am Montag, 24. Juli 2023, 20:42:42 CEST schrieb Adhemerval Zanella Netto:
> > 
> >> Adhemerval Zanella (3):
> >>   posix: Fix test-errno build with fortify enable
> > 
> > This one looks good but there was a mismatch between the gcc version
> > you said triggered the failure and the one commented in the code.
> > If you clarify that we can get this committed.
> > 
> >>   string: Fix bug-strncat1 with fortify enabled
> >>   string: Fix tester with fortify enabled
> > 
> > These two look good to me.
> 
> Thanks for the review. Do we want these for 2.38 or should we wait for 2.39?
> 

Should be fine imho for 2.38

-- 
Andreas K. Hüttel
dilfridge@gentoo.org
Gentoo Linux developer
(council, toolchain, base-system, perl, libreoffice)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 981 bytes --]