gcc-9.2.0 and spurious warnings

gcc-9.2.0 and spurious warnings

[Josef Wolf]

Hello,

I upgraded from gcc-8.2.0 to gcc-9.2.0.

With gcc-9.2.0, I am getting new warnings for old code, which I have not seen
with gcc-8.2.0 and earlier gcc versions.

What's more is, that those warnings disappear when OTHER (totally unrelated)
parts of the code is removed.


Here is one example (some helper functions for parsing). With the code
attached below, I get this warning:

   $ LANG= PATH=$PATH:/usr/local/crossgcc/bin m68k-unknown-elf-gcc -ansi -pedantic -Wall -Wcast-align -Wstrict-prototypes -Wmissing-prototypes -std=c89 -Wnull-dereference -g  -O2 -fno-toplevel-reorder  -mcpu32  -c -o t.o t.c
   t.c: In function 'get_word':
   t.c:53:5: warning: 'strncpy' destination unchanged after copying no bytes
   [-Wstringop-truncation]
      53 |     strncpy (*r, p, q-p);         /* copy */
         |     ^~~~~~~~~~~~~~~~~~~~

I don't understand this warning at all. The freshly allocated memory is big
enough to hold the copied word including the trailing NUL character. And the
NUL character is appended just behind the copied word.

What am I supposed to do to get rid of the warning?

When I remove the next_word() function, which is TOTALLY UNRELATED to the code
in question, the warning disappears!


Here is the code:

#include <ctype.h>
#include <string.h>
#include <stdlib.h>

char *skip_spaces (char *p);
char *skip_word (char *p);
char *next_word (char *p);
char *get_word (char *p, char **r);
void halt_system (int restart, char *fmt, ...) __attribute__((noreturn));

/* skip spaces and tabs */
char *skip_spaces (char *p)
{
    if (!p) return NULL;

    while (((*p==' ') || (*p=='\t')))
        p++;

    return (p);
}

/* skip word (separated by spaces/tabs) */
char *skip_word (char *p)
{
    while (p && *p && (*p!=' ') && (*p!='\t'))
        p++;

    return (p);
}

/* serch next word (separated by spaces/tabs) */
char *next_word (char *p)
{
    return (skip_spaces (skip_word (p)));
}

/* This function will extract a word and store it in freshly allocated memory */
char *get_word (char *p, char **r)
{
    char *q;

    if (!p) return NULL;

    p = skip_spaces (p);  /* look for start of word */
    q = skip_word (p);    /* look for end of word */

    if (!(*r=malloc (q-p+1)))  /* allocate memory */
        halt_system (1, "Out of memory in get_word()");

    /* !!!!!! The warning at the next line is:
       'strncpy' destination unchanged after copying no bytes [-Wstringop-truncation]
       This warning disappears when the next_word() function is removed.
    */
    strncpy (*r, p, q-p);         /* copy */
    (*r)[q-p] = 0;                /* mark the end */

    return (skip_spaces (q));     /* return pointer to next word */
}

-- 
Josef Wolf
jw@raven.inka.de

Re: gcc-9.2.0 and spurious warnings

[Jeff Law]

On Thu, 2019-12-12 at 09:10 +0100, Josef Wolf wrote:
> Hello,
> 
> I upgraded from gcc-8.2.0 to gcc-9.2.0.
> 
> With gcc-9.2.0, I am getting new warnings for old code, which I have not seen
> with gcc-8.2.0 and earlier gcc versions.
> 
> What's more is, that those warnings disappear when OTHER (totally unrelated)
> parts of the code is removed.
> 
> 
> Here is one example (some helper functions for parsing). With the code
> attached below, I get this warning:
> 
>    $ LANG= PATH=$PATH:/usr/local/crossgcc/bin m68k-unknown-elf-gcc -ansi -pedantic -Wall -Wcast-align -Wstrict-prototypes -Wmissing-prototypes -std=c89 -Wnull-dereference -g  -O2 -fno-toplevel-reorder  -mcpu32  -c -o t.o t.c
>    t.c: In function 'get_word':
>    t.c:53:5: warning: 'strncpy' destination unchanged after copying no bytes
>    [-Wstringop-truncation]
>       53 |     strncpy (*r, p, q-p);         /* copy */
>          |     ^~~~~~~~~~~~~~~~~~~~
> 
> I don't understand this warning at all. The freshly allocated memory is big
> enough to hold the copied word including the trailing NUL character. And the
> NUL character is appended just behind the copied word.
I believe it's telling you that the supplied length is zero and that
you haven't changed the destination string.  For that to be true, q
must have the same value as p.  I suspect most of the time a zero
length parameter to strncpy is unintentional.



> 
> 
> When I remove the next_word() function, which is TOTALLY UNRELATED to the code
> in question, the warning disappears!
That probably affects inlining decisions.

jeff
> 

Re: gcc-9.2.0 and spurious warnings

[Josef Wolf]

Thanks for the help, Jeff!

On Thu, Dec 12, 2019 at 01:41:08PM -0700, Jeff Law wrote:
> I believe it's telling you that the supplied length is zero and that
> you haven't changed the destination string.  For that to be true, q
> must have the same value as p.  I suspect most of the time a zero
> length parameter to strncpy is unintentional.

I agree that strncpy() CAN be called with length==0. But this can happen only
when get_word() is passed an empty string.

Is it really evil to deal with empty strings? =8-O

-- 
Josef Wolf
jw@raven.inka.de

Re: gcc-9.2.0 and spurious warnings

[Martin Sebor]

On 12/12/19 1:10 AM, Josef Wolf wrote:
> Hello,
> 
> I upgraded from gcc-8.2.0 to gcc-9.2.0.
> 
> With gcc-9.2.0, I am getting new warnings for old code, which I have not seen
> with gcc-8.2.0 and earlier gcc versions.
> 
> What's more is, that those warnings disappear when OTHER (totally unrelated)
> parts of the code is removed.
> 
> 
> Here is one example (some helper functions for parsing). With the code
> attached below, I get this warning:
> 
>     $ LANG= PATH=$PATH:/usr/local/crossgcc/bin m68k-unknown-elf-gcc -ansi -pedantic -Wall -Wcast-align -Wstrict-prototypes -Wmissing-prototypes -std=c89 -Wnull-dereference -g  -O2 -fno-toplevel-reorder  -mcpu32  -c -o t.o t.c
>     t.c: In function 'get_word':
>     t.c:53:5: warning: 'strncpy' destination unchanged after copying no bytes
>     [-Wstringop-truncation]
>        53 |     strncpy (*r, p, q-p);         /* copy */
>           |     ^~~~~~~~~~~~~~~~~~~~
> 
> I don't understand this warning at all. The freshly allocated memory is big
> enough to hold the copied word including the trailing NUL character. And the
> NUL character is appended just behind the copied word.
> 
> What am I supposed to do to get rid of the warning?
> 
> When I remove the next_word() function, which is TOTALLY UNRELATED to the code
> in question, the warning disappears!

Jeff already explained what the warning means.  I don't see it with
fresh trunk, either natively or with an m68k-unknown-elf cross, so
I can't really say why GCC 9 thinks the two pointers are equal.  You
can tell by looking at the optimization dumps (use -ftree-dump-all
and grep the output for strncpy with a zero argument).

What I can say is that strncpy isn't really meant to be used this
way: to copy exactly as many bytes as the third argument says.
That's what memcpy is for.  The warning was added because strncpy
is frequently misused and a common source of bugs.  The warning
tries to avoid triggering for the safe uses when it can detect
they are safe, but it's imperfect and prone to false alarms.

To avoid it I would suggest using memcpy instead.  It will likely
also be faster.  Alternatively, you can try calling the function
only when the difference is non-zero.

Martin

> 
> Here is the code:
> 
> #include <ctype.h>
> #include <string.h>
> #include <stdlib.h>
> 
> char *skip_spaces (char *p);
> char *skip_word (char *p);
> char *next_word (char *p);
> char *get_word (char *p, char **r);
> void halt_system (int restart, char *fmt, ...) __attribute__((noreturn));
> 
> /* skip spaces and tabs */
> char *skip_spaces (char *p)
> {
>      if (!p) return NULL;
> 
>      while (((*p==' ') || (*p=='\t')))
>          p++;
> 
>      return (p);
> }
> 
> /* skip word (separated by spaces/tabs) */
> char *skip_word (char *p)
> {
>      while (p && *p && (*p!=' ') && (*p!='\t'))
>          p++;
> 
>      return (p);
> }
> 
> /* serch next word (separated by spaces/tabs) */
> char *next_word (char *p)
> {
>      return (skip_spaces (skip_word (p)));
> }
> 
> /* This function will extract a word and store it in freshly allocated memory */
> char *get_word (char *p, char **r)
> {
>      char *q;
> 
>      if (!p) return NULL;
> 
>      p = skip_spaces (p);  /* look for start of word */
>      q = skip_word (p);    /* look for end of word */
> 
>      if (!(*r=malloc (q-p+1)))  /* allocate memory */
>          halt_system (1, "Out of memory in get_word()");
> 
>      /* !!!!!! The warning at the next line is:
>         'strncpy' destination unchanged after copying no bytes [-Wstringop-truncation]
>         This warning disappears when the next_word() function is removed.
>      */
>      strncpy (*r, p, q-p);         /* copy */
>      (*r)[q-p] = 0;                /* mark the end */
> 
>      return (skip_spaces (q));     /* return pointer to next word */
> }
> 

Re: gcc-9.2.0 and spurious warnings

[Josef Wolf]

Thanks for the explanation, Martin!

On Thu, Dec 12, 2019 at 05:20:43PM -0700, Martin Sebor wrote:
> I don't see it with
> fresh trunk, either natively or with an m68k-unknown-elf cross, so
> I can't really say why GCC 9 thinks the two pointers are equal.

Thus there's a chance that the warning will go away with one of the next GCC
releases?

> You
> can tell by looking at the optimization dumps (use -ftree-dump-all
> and grep the output for strncpy with a zero argument).

Hmmm. No -ftree-dump-XXX options here.

> What I can say is that strncpy isn't really meant to be used this
> way: to copy exactly as many bytes as the third argument says.
> That's what memcpy is for.

I tend to use memXXX() family of functions when I am dealing with binary data.

Here, I am dealing with zero terminated strings. Thus, the strXXX() family of
functions should be used. IMHO, of course!

> The warning was added because strncpy
> is frequently misused and a common source of bugs.

I am not sure I understand which kind of misuse you are talking about.

I see there's a pitfall with strncpy() failing to add the terminating NUL
charachter. But this terminating NUL is added explicitly in this case.

> The warning
> tries to avoid triggering for the safe uses when it can detect
> they are safe, but it's imperfect and prone to false alarms.

False alarms teach people to ignore warnings.

> To avoid it I would suggest using memcpy instead.  It will likely
> also be faster.  Alternatively, you can try calling the function
> only when the difference is non-zero.

I agree that memcpy() could do the job and would probably be even faster. But
this is about semantics. This piece of code is dealing with zero terminated
strings.

-- 
Josef Wolf
jw@raven.inka.de

Re: gcc-9.2.0 and spurious warnings

[Segher Boessenkool]

On Fri, Dec 13, 2019 at 07:54:49AM +0100, Josef Wolf wrote:
> > You
> > can tell by looking at the optimization dumps (use -ftree-dump-all
> > and grep the output for strncpy with a zero argument).
> 
> Hmmm. No -ftree-dump-XXX options here.

It's -fdump-tree-XXX instead.


Segher

Re: gcc-9.2.0 and spurious warnings

[Josef Wolf]

On Fri, Dec 13, 2019 at 03:55:29AM -0600, Segher Boessenkool wrote:

> It's -fdump-tree-XXX instead.

Thanks!

I see... Lots of output files...

But I can't make any sense sout ouf it.

Any hints at what I should look at?

-- 
Josef Wolf
jw@raven.inka.de

Re: gcc-9.2.0 and spurious warnings

[Martin Sebor]

On 12/12/19 11:54 PM, Josef Wolf wrote:
> Thanks for the explanation, Martin!
> 
> On Thu, Dec 12, 2019 at 05:20:43PM -0700, Martin Sebor wrote:
>> I don't see it with
>> fresh trunk, either natively or with an m68k-unknown-elf cross, so
>> I can't really say why GCC 9 thinks the two pointers are equal.
> 
> Thus there's a chance that the warning will go away with one of the next GCC
> releases?

I don't see it even with a cross built from 9.x so it's likely still
there, and something is causing my cross to optimize code differently
than yours.  (The warning depends on optimization.)

> 
>> You
>> can tell by looking at the optimization dumps (use -ftree-dump-all
>> and grep the output for strncpy with a zero argument).
> 
> Hmmm. No -ftree-dump-XXX options here.

Sorry, it's -fdump-tree-all.  There will be one file for each
optimization pass.  Of interest in the first one that shows strncpy
being called with a literal zero as the last argument.  The file
name suffix encodes the name of the optimizing pass that either
introduced the call.  It's likely the result of an optimization like
jump threading that runs before then and that introduces an additional
call to strncpy into which the zero is propagated.  So I'd expect
the first relevant dump file to be the first one with two calls to
strncpy.  (This will just help confirm what causes the warning, not
necessarily come up with a fix.)

> 
>> What I can say is that strncpy isn't really meant to be used this
>> way: to copy exactly as many bytes as the third argument says.
>> That's what memcpy is for.
> 
> I tend to use memXXX() family of functions when I am dealing with binary data.
> 
> Here, I am dealing with zero terminated strings. Thus, the strXXX() family of
> functions should be used. IMHO, of course!

Yes, that's a reasonable rule of thumb, and GCC does try to avoid
the warning in this case (it looks for the nul termination after
the call).  But since the implementation is less than perfect it's
prone to false positives.  We'd obviously like to avoid those when
we can, but ideally without missing the real bugs.  It's a tradeoff,
and a judgment call where to draw the line.

The only other string function that comes to mind that avoids
the warning is snprintf:

   sprintf (*r, "%.*s", (int)(q-p), p);

but I mention it just for completeness.  I wouldn't recommend using
it in this case.

If rewriting the code is not an option, individual instances of
warnings can usually be suppressed by #pragma GCC diagnostic:

   #pragma GCC diagnostic push
   #pragma GCC diagnostic ignored "-Wstringop-truncation"
     strncpy (*r, p, q-p);         /* copy */
   #pragma GCC diagnostic pop

(Unfortunately, the pragma doesn't work as well as it should either
for warnings that depend on optimization, due to inlining and code
motion, so you might need to tweak the placement a bit to make it
have the expected effect.  Putting the pragmas before and after
the out-of-line function should work.)

> 
>> The warning was added because strncpy
>> is frequently misused and a common source of bugs.
> 
> I am not sure I understand which kind of misuse you are talking about.
> 
> I see there's a pitfall with strncpy() failing to add the terminating NUL
> charachter. But this terminating NUL is added explicitly in this case.

Using the result of the function as a "bounded string copy", as if
it was guaranteed to be a nul-terminated string, is the most common
misuse.  There are many discussions of the risks online.  A couple
of examples are CWE-170 and the strncpy() and strncat() article on
the US CERT Build Security In site:
https://www.us-cert.gov/bsi/articles/knowledge/coding-practices/strncpy-and-strncat

Martin