Fail to compile GDB with recent GCC trunk (-Werror=stringop-overflow=, -Werror=stringop-truncation)

Fail to compile GDB with recent GCC trunk (-Werror=stringop-overflow=, -Werror=stringop-truncation)

[Yao Qi]

Hi,
I failed to compile GDB with GCC trunk (8.0.0 20171117) because of some
-Werror=stringop-overflow= and -Werror=stringop-truncation warnings.
Some of them are not necessary to me,

1. ../../binutils-gdb/gdb/python/py-gdb-readline.c:79:15: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation]
       strncpy (q, p, n);
       ~~~~~~~~^~~~~~~~~
../../binutils-gdb/gdb/python/py-gdb-readline.c:73:14: note: length computed here
   n = strlen (p);
       ~~~~~~~^~~

the code is simple,

  n = strlen (p);

  /* Copy the line to Python and return.  */
  q = (char *) PyMem_RawMalloc (n + 2);
  if (q != NULL)
    {
      strncpy (q, p, n);
      q[n] = '\n';
      q[n + 1] = '\0';
    }

I don't see the point of warning here.

2. ../../binutils-gdb/gdb/cp-namespace.c:1071:11: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying 2 bytes from a string of the same length [-Werror=stringop-truncation]
   strncpy (full_name + scope_length, "::", 2);
   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  full_name = (char *) alloca (scope_length + 2 + strlen (name) + 1);
  strncpy (full_name, scope, scope_length);
  strncpy (full_name + scope_length, "::", 2);
  strcpy (full_name + scope_length + 2, name);

the code looks right to me,

Likewise,

../../../binutils-gdb/gdb/gdbserver/remote-utils.c:1204:14: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying 6 bytes from a string of the same length [-Werror=stringop-truncation]
      strncpy (buf, "watch:", 6);
      ~~~~~~~~^~~~~~~~~~~~~~~~~~

            strncpy (buf, "watch:", 6);
            buf += 6;
....
        *buf = '\0';

I can "fix" these warnings by changing GDB code, use strcpy in 1) and
use memcpy in 2).  Do we expect all the users of GCC 8 changing their
correct code because GCC is not happy on the code?  The warning is too
aggressive to me.

-- 
Yao (齐尧)

Re: Fail to compile GDB with recent GCC trunk (-Werror=stringop-overflow=, -Werror=stringop-truncation)

[Eric Gallager]

On 11/20/17, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Hi,
> I failed to compile GDB with GCC trunk (8.0.0 20171117) because of some
> -Werror=stringop-overflow= and -Werror=stringop-truncation warnings.
> Some of them are not necessary to me,
>
> 1. ../../binutils-gdb/gdb/python/py-gdb-readline.c:79:15: error: ‘char*
> strncpy(char*, const char*, size_t)’ output truncated before terminating nul
> copying as many bytes from a string as its length
> [-Werror=stringop-truncation]
>        strncpy (q, p, n);
>        ~~~~~~~~^~~~~~~~~
> ../../binutils-gdb/gdb/python/py-gdb-readline.c:73:14: note: length computed
> here
>    n = strlen (p);
>        ~~~~~~~^~~
>
> the code is simple,
>
>   n = strlen (p);
>
>   /* Copy the line to Python and return.  */
>   q = (char *) PyMem_RawMalloc (n + 2);
>   if (q != NULL)
>     {
>       strncpy (q, p, n);
>       q[n] = '\n';
>       q[n + 1] = '\0';
>     }
>
> I don't see the point of warning here.
>
> 2. ../../binutils-gdb/gdb/cp-namespace.c:1071:11: error: ‘char*
> strncpy(char*, const char*, size_t)’ output truncated before terminating nul
> copying 2 bytes from a string of the same length
> [-Werror=stringop-truncation]
>    strncpy (full_name + scope_length, "::", 2);
>    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   full_name = (char *) alloca (scope_length + 2 + strlen (name) + 1);
>   strncpy (full_name, scope, scope_length);
>   strncpy (full_name + scope_length, "::", 2);
>   strcpy (full_name + scope_length + 2, name);
>
> the code looks right to me,
>
> Likewise,
>
> ../../../binutils-gdb/gdb/gdbserver/remote-utils.c:1204:14: error: ‘char*
> strncpy(char*, const char*, size_t)’ output truncated before terminating nul
> copying 6 bytes from a string of the same length
> [-Werror=stringop-truncation]
>       strncpy (buf, "watch:", 6);
>       ~~~~~~~~^~~~~~~~~~~~~~~~~~
>
>             strncpy (buf, "watch:", 6);
>             buf += 6;
> ....
>         *buf = '\0';
>
> I can "fix" these warnings by changing GDB code, use strcpy in 1) and
> use memcpy in 2).  Do we expect all the users of GCC 8 changing their
> correct code because GCC is not happy on the code?  The warning is too
> aggressive to me.
>
> --
> Yao (齐尧)
>

I thought there was a gcc bug open about this but now I can't seem to
find it; please let me know if you come across the one I was trying to
remember...

Re: Fail to compile GDB with recent GCC trunk (-Werror=stringop-overflow=, -Werror=stringop-truncation)

[Eric Gallager]

On 11/20/17, Eric Gallager <egall@gwmail.gwu.edu> wrote:
> On 11/20/17, Yao Qi <qiyaoltc@gmail.com> wrote:
>>
>> Hi,
>> I failed to compile GDB with GCC trunk (8.0.0 20171117) because of some
>> -Werror=stringop-overflow= and -Werror=stringop-truncation warnings.
>> Some of them are not necessary to me,
>>
>> 1. ../../binutils-gdb/gdb/python/py-gdb-readline.c:79:15: error: ‘char*
>> strncpy(char*, const char*, size_t)’ output truncated before terminating
>> nul
>> copying as many bytes from a string as its length
>> [-Werror=stringop-truncation]
>>        strncpy (q, p, n);
>>        ~~~~~~~~^~~~~~~~~
>> ../../binutils-gdb/gdb/python/py-gdb-readline.c:73:14: note: length
>> computed
>> here
>>    n = strlen (p);
>>        ~~~~~~~^~~
>>
>> the code is simple,
>>
>>   n = strlen (p);
>>
>>   /* Copy the line to Python and return.  */
>>   q = (char *) PyMem_RawMalloc (n + 2);
>>   if (q != NULL)
>>     {
>>       strncpy (q, p, n);
>>       q[n] = '\n';
>>       q[n + 1] = '\0';
>>     }
>>
>> I don't see the point of warning here.
>>
>> 2. ../../binutils-gdb/gdb/cp-namespace.c:1071:11: error: ‘char*
>> strncpy(char*, const char*, size_t)’ output truncated before terminating
>> nul
>> copying 2 bytes from a string of the same length
>> [-Werror=stringop-truncation]
>>    strncpy (full_name + scope_length, "::", 2);
>>    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   full_name = (char *) alloca (scope_length + 2 + strlen (name) + 1);
>>   strncpy (full_name, scope, scope_length);
>>   strncpy (full_name + scope_length, "::", 2);
>>   strcpy (full_name + scope_length + 2, name);
>>
>> the code looks right to me,
>>
>> Likewise,
>>
>> ../../../binutils-gdb/gdb/gdbserver/remote-utils.c:1204:14: error: ‘char*
>> strncpy(char*, const char*, size_t)’ output truncated before terminating
>> nul
>> copying 6 bytes from a string of the same length
>> [-Werror=stringop-truncation]
>>       strncpy (buf, "watch:", 6);
>>       ~~~~~~~~^~~~~~~~~~~~~~~~~~
>>
>>             strncpy (buf, "watch:", 6);
>>             buf += 6;
>> ....
>>         *buf = '\0';
>>
>> I can "fix" these warnings by changing GDB code, use strcpy in 1) and
>> use memcpy in 2).  Do we expect all the users of GCC 8 changing their
>> correct code because GCC is not happy on the code?  The warning is too
>> aggressive to me.
>>
>> --
>> Yao (齐尧)
>>
>
> I thought there was a gcc bug open about this but now I can't seem to
> find it; please let me know if you come across the one I was trying to
> remember...
>

Never mind, I found the bug I was looking for (80354), but it was
about -Wformat-truncation, not -Wstringop-truncation, and it's closed,
not open, and the point I made in it was about sprintf vs. snprintf,
not strcpy vs. strncpy (although it applies equally as well in both
cases): https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80354

Re: Fail to compile GDB with recent GCC trunk (-Werror=stringop-overflow=, -Werror=stringop-truncation)

[Martin Sebor]

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

On 11/20/2017 08:51 AM, Yao Qi wrote:
>
> Hi,
> I failed to compile GDB with GCC trunk (8.0.0 20171117) because of some
> -Werror=stringop-overflow= and -Werror=stringop-truncation warnings.
> Some of them are not necessary to me,

I have the attached patch for two of these but I have been waiting
to submit it until the latest GCC patch has been approved that
adjusts the checker a bit.

>
> 1. ../../binutils-gdb/gdb/python/py-gdb-readline.c:79:15: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation]
>        strncpy (q, p, n);
>        ~~~~~~~~^~~~~~~~~
> ../../binutils-gdb/gdb/python/py-gdb-readline.c:73:14: note: length computed here
>    n = strlen (p);
>        ~~~~~~~^~~
>
> the code is simple,
>
>   n = strlen (p);
>
>   /* Copy the line to Python and return.  */
>   q = (char *) PyMem_RawMalloc (n + 2);
>   if (q != NULL)
>     {
>       strncpy (q, p, n);
>       q[n] = '\n';
>       q[n + 1] = '\0';
>     }
>
> I don't see the point of warning here.

The overall purpose of the warning is to help find likely misuses
of strncpy and strncat.  As with any warning that's based on intent,
it cannot avoid highlighting some safe uses, or missing some unsafe
ones.

The case above is based on a heuristic designed to find bugs where
the bound depends on the length of the source rather the size of
the destination, as in:

   strncpy (d, s, strlen (s));

This is, unfortunately, a common misuse/mistake.  It's often seen
in legacy code that's being updated in response to a security
mandate to replace strcpy with strncpy.

The GDB use case, although safe, is also not how the function is
intended to be used.  The intended use is to specify the size of
the destination, typically a statically allocated array, and have
the function fill it with data (not necessarily a string, and
not necessarily containing a terminating nul).  When the array
is allocated dynamically and sized to store the entire string
it's preferable to use some other function (e.g., memcpy or
strcpy).

>
> 2. ../../binutils-gdb/gdb/cp-namespace.c:1071:11: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying 2 bytes from a string of the same length [-Werror=stringop-truncation]
>    strncpy (full_name + scope_length, "::", 2);
>    ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   full_name = (char *) alloca (scope_length + 2 + strlen (name) + 1);
>   strncpy (full_name, scope, scope_length);
>   strncpy (full_name + scope_length, "::", 2);

This is safe, although also not the intended use of the function.
The call above can be replaced either by memcpy or strcpy.  There
also is no good way to avoid warning on it without compromising
the efficacy of the checker.

>   strcpy (full_name + scope_length + 2, name);
>
> the code looks right to me,
>
> Likewise,
>
> ../../../binutils-gdb/gdb/gdbserver/remote-utils.c:1204:14: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying 6 bytes from a string of the same length [-Werror=stringop-truncation]
>       strncpy (buf, "watch:", 6);
>       ~~~~~~~~^~~~~~~~~~~~~~~~~~
>
>             strncpy (buf, "watch:", 6);
>             buf += 6;
> ....
>         *buf = '\0';

As above, memcpy or strcpy are the preferred alternatives.

Martin

[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 3802 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 836 bytes --]

The latest revision of GCC 8.0 adds a -Wstringop-truncation option
to detect common misuses of the strncpy and strncat functions that
may truncate the copy and leave the result without a terminating
nul character.

In testing the implementation with GDB sources on x86_64 I found
a few instances of the warning that are issued for what's safe
but nevertheless not strictly intended uses of the functions
(i.e., to create "bounded" non-nul-terminated copies of a string).
I adjusted the warning to accept some but not all of these use
cases.  The attached patch shows the two instances of the warning
that I had to suppress in GDB.

In general, even though the checker handles some such cases, to
avoid the warning, it's best to use strncpy only with a bound that
reflects the size of the destination, never that of the source.

Martin


[-- Attachment #2.1.2: gdb-wstringop-trunc.diff --]
[-- Type: text/x-patch, Size: 2055 bytes --]

2017-11-10  Martin Sebor  <msebor@redhat.com>

	* gdb/cli/cli-decode.c (help_list): Use strcpy and memcpy instead
	of strncpy.
	* gdb/cp-namespace.c (cp_lookup_transparent_type_loop): Use strcpy
	instead of strncpy to avoid -Wstringop-truncation.
	* gdb/gdbserver/remote-utils.c (prepare_resume_reply): Use memcpy
	instead of strncpy to avoid -Wstringop-truncation.

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 87ebed5..968779e 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1,4 +1,4 @@
-/* Handle lists of commands, their decoding and documentation, for GDB.
+t/* Handle lists of commands, their decoding and documentation, for GDB.
 
    Copyright (C) 1986-2017 Free Software Foundation, Inc.
 
@@ -1115,9 +1115,8 @@ help_list (struct cmd_list_element *list, const char *cmdtype,
   if (len)
     {
       cmdtype1[0] = ' ';
-      strncpy (cmdtype1 + 1, cmdtype, len - 1);
-      cmdtype1[len] = 0;
-      strncpy (cmdtype2, cmdtype, len - 1);
+      strcpy (cmdtype1 + 1, cmdtype);
+      memcpy (cmdtype2, cmdtype, len - 1);
       strcpy (cmdtype2 + len - 1, " sub");
     }
 
diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 214b7e1..fabe87a 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -1068,7 +1068,7 @@ cp_lookup_transparent_type_loop (const char *name,
 
   full_name = (char *) alloca (scope_length + 2 + strlen (name) + 1);
   strncpy (full_name, scope, scope_length);
-  strncpy (full_name + scope_length, "::", 2);
+  strcpy (full_name + scope_length, "::");
   strcpy (full_name + scope_length + 2, name);
 
   return basic_lookup_transparent_type (full_name);
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 66e0652..e314447 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -1201,7 +1201,7 @@ prepare_resume_reply (char *buf, ptid_t ptid,
 	    CORE_ADDR addr;
 	    int i;
 
-	    strncpy (buf, "watch:", 6);
+	    memcpy (buf, "watch:", 6);
 	    buf += 6;
 
 	    addr = (*the_target->stopped_data_address) ();

Re: Fail to compile GDB with recent GCC trunk (-Werror=stringop-overflow=, -Werror=stringop-truncation)

[Yao Qi]

On 17-11-20 09:33:46, Martin Sebor wrote:
> On 11/20/2017 08:51 AM, Yao Qi wrote:
> >
> >Hi,
> >I failed to compile GDB with GCC trunk (8.0.0 20171117) because of some
> >-Werror=stringop-overflow= and -Werror=stringop-truncation warnings.
> >Some of them are not necessary to me,
> 
> I have the attached patch for two of these but I have been waiting
> to submit it until the latest GCC patch has been approved that
> adjusts the checker a bit.

Hi Martin,
can you post the patch to gdb-patches@sourceware.org?  The patch can
be reviewed there.

> 
> >
> >1. ../../binutils-gdb/gdb/python/py-gdb-readline.c:79:15: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation]
> >       strncpy (q, p, n);
> >       ~~~~~~~~^~~~~~~~~
> >../../binutils-gdb/gdb/python/py-gdb-readline.c:73:14: note: length computed here
> >   n = strlen (p);
> >       ~~~~~~~^~~
> >
> >the code is simple,
> >
> >  n = strlen (p);
> >
> >  /* Copy the line to Python and return.  */
> >  q = (char *) PyMem_RawMalloc (n + 2);
> >  if (q != NULL)
> >    {
> >      strncpy (q, p, n);
> >      q[n] = '\n';
> >      q[n + 1] = '\0';
> >    }
> >
> >I don't see the point of warning here.
> 
> The overall purpose of the warning is to help find likely misuses
> of strncpy and strncat.  As with any warning that's based on intent,
> it cannot avoid highlighting some safe uses, or missing some unsafe
> ones.

As a user, false negative is fine to me, but false positive is too noisy.
If I made stupid mistakes and compiler doesn't find them, that is fine.
The people who wrote such bad code should be blamed, rather than
compiler.  However, if compiler emits many warnings to the correct code,
it is annoying.  Usually, "too many false warnings" == "no warnings".

> 
> The case above is based on a heuristic designed to find bugs where
> the bound depends on the length of the source rather the size of
> the destination, as in:
> 
>   strncpy (d, s, strlen (s));
> 
> This is, unfortunately, a common misuse/mistake.  It's often seen
> in legacy code that's being updated in response to a security
> mandate to replace strcpy with strncpy.
> 
> The GDB use case, although safe, is also not how the function is
> intended to be used.  The intended use is to specify the size of
> the destination, typically a statically allocated array, and have
> the function fill it with data (not necessarily a string, and
> not necessarily containing a terminating nul).  When the array
> is allocated dynamically and sized to store the entire string
> it's preferable to use some other function (e.g., memcpy or
> strcpy).

The real issue here is GCC warning is too aggressive, and even emit
warnings to the correct code.  We fixed glibc, gdb, what is the next?
GCC will be used to build thousands of packages, do you expect "fixing"
all of them?  If I have to fix my correct code, just because gcc
complains about it, it is time I consider switching to other compilers
to compile my code.

> 
> >
> >2. ../../binutils-gdb/gdb/cp-namespace.c:1071:11: error: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying 2 bytes from a string of the same length [-Werror=stringop-truncation]
> >   strncpy (full_name + scope_length, "::", 2);
> >   ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> >  full_name = (char *) alloca (scope_length + 2 + strlen (name) + 1);
> >  strncpy (full_name, scope, scope_length);
> >  strncpy (full_name + scope_length, "::", 2);
> 
> This is safe, although also not the intended use of the function.
> The call above can be replaced either by memcpy or strcpy.  There
> also is no good way to avoid warning on it without compromising
> the efficacy of the checker.
> 

IMO, compiler != static analysis/checker, although they share many
technologies.  They have different responsibilities, compiler is to
generate binary code from source code, while static analysis tool
is to find problems in the code as many as possible.

-- 
Yao (齐尧)