diff mbox series

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

Message ID 20230721121817.1978446-3-adhemerval.zanella@linaro.org
State Committed
Commit 85ac7edcdf67010c223541936243427213ac87a6
Headers
Series Fix some issues with tests when fortify is enabled |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm warning Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 warning Patch failed to apply

Commit Message

Adhemerval Zanella Netto July 21, 2023, 12:18 p.m. UTC 
  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(-)

Comments

Carlos O'Donell July 24, 2023, 12:36 p.m. UTC | #1 
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')
Siddhesh Poyarekar July 24, 2023, 2:24 p.m. UTC | #2 
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')
>
Adhemerval Zanella Netto July 24, 2023, 2:26 p.m. UTC | #3 
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.
Siddhesh Poyarekar July 24, 2023, 2:36 p.m. UTC | #4 
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
diff mbox series

Patch

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')