[2/3] string: Fix bug-strncat1 with fortify 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
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
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')
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')
>
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.
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
@@ -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')