Bug 84468 - [8 Regression] bogus -Wstringop-truncation despite assignment after conditional strncpy
Summary: [8 Regression] bogus -Wstringop-truncation despite assignment after condition...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 8.0.1
: P1 normal
Target Milestone: 8.0
Assignee: Martin Sebor
URL:
Keywords: diagnostic, patch
Depends on:
Blocks: Wstringop-truncation
  Show dependency treegraph
 
Reported: 2018-02-19 15:56 UTC by Romain Geissler
Modified: 2019-01-09 23:46 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-02-19 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Romain Geissler 2018-02-19 15:56:50 UTC
Hi,

I can see some strange behavior for the warning -W=stringop-truncation with -O2 and current master, depending on how I enclose my code in if/else blocks or not, despite that logically speaking, the end result is always the same.

See this code:

EOF
#include <string.h>
        
class A 
{   
    public:
        A();
        A(const char* iCString);
    
        A& operator=(const A& iA);

    private:
        void setCString(const char* iCString);
    
        // Uncommenting the attribute silences the warning
        // but actually we do have a "string", not a "nonstring".
        /* [[gnu::nonstring]] */ char _cstring[3 + 1];
};      
    
A::A()
{   
    _cstring[0] = '\0';
}

A::A(const char* iCString)
{
    setCString(iCString);
}

A& A::operator=(const A& iA)
{
    if (this != &iA) // Commenting this line silences the warning.
    {
        setCString(iA._cstring);
    }

    return *this;
}

void A::setCString(const char* iCString)
{   
    // This version produces warning.
    if (iCString)
    {   
        strncpy(_cstring, iCString, 3);
    }
    
    _cstring[3] = '\0';
    
    /*
    // This version doesn't produce any warning, but has the same logic as above.
    if (iCString)
    {   
        strncpy(_cstring, iCString, 3);
        _cstring[3] = '\0';
    }
    else
    {
        _cstring[3] = '\0';
    }
    */

    /*
    // This version also produces a warning, despite having the same logic as above,
    // just with an extra assigment to _cstring[3] when iCString is not null.
    if (iCString)
    {
        strncpy(_cstring, iCString, 3);
        _cstring[3] = '\0';
    }
    _cstring[3] = '\0';
    */
}

When compiled with -Wall -Werror -Wextra -std=gnu++17 -O2 and current master. I get:
error: ‘char* strncpy(char*, const char*, size_t)’ output may be truncated copying 3 bytes from a string of length 3 [-Werror=stringop-truncation]

First what I find strange is the warning itself. gcc is clever enough to see that source is 3 bytes long, that we copy in a buffer that is 4 bytes long, so why would it complain about a truncation anyway ? Or maybe I misunderstood the wording of the warning itself ?

Second, why are the different variant of A::setCString I commented in the code working differently wrt the warning, while logically speaking they do have the same behavior. If we need to write our if/else block in a given way to make gcc understand it is safe, this starts to be complex for developers.

Final question: do you think it would somehow make sense to introduce a "string" attribute, that would be the contrary of the newly introduced "nonstring". "nonstring" means that maybe the string is not null terminate. "string" would mean that for sure the array is always null terminated, and thus doing things like strncpy(_cstring, anotherA._cstring, 4) (note that I used 4 and not 3 for the total size), would always work ? "nonstring" correctly silences the warning, but has the exact opposite semantic meaning than I have in this particular case.

Cheers,
Romain
Comment 1 Sérgio Basto 2018-02-19 17:09:32 UTC
btw and this warning  [1] make sense ? 

[1]
mongoose.c: In function 'mg_resolve_async_opt':
mongoose.c:10791:3: error: 'strncpy' specified bound 1024 equals destination size [-Werror=stringop-truncation]
   strncpy(req->name, name, sizeof(req->name));
Comment 2 Martin Sebor 2018-02-19 21:46:32 UTC
The warning is designed to look at the next statement just after a call to strncpy() and if it's an assignment to the destination it does not trigger.  Unfortunately, due to a limitation/missing feature in the optimizer (bug 84470), in the test case in comment #0, the 'if (iCString)' test prevents the warning from seeing that the next statement is an assignment.

As you already noted, making the assignment to _cstring[3] in A::setCString() also conditional on iCString being nonnull avoids the warning.  This will become unnecessary once the missing optimization mentioned in bug 84470 is implemented.  I can confirm that the warning is a false positive in this case.  Let me see if there's a way to enhance the warning to handle it until the optimization is implemented.

(In reply to Sérgio Basto from comment #1)
> btw and this warning  [1] make sense ? 
> 
> [1]
> mongoose.c: In function 'mg_resolve_async_opt':
> mongoose.c:10791:3: error: 'strncpy' specified bound 1024 equals destination
> size [-Werror=stringop-truncation]
>    strncpy(req->name, name, sizeof(req->name));

It's a common mistake to assume that strncpy() nul-terminates the copy even when it's called with the size of the destination as the last argument and a source at least as long.  The warning is meant to detect cases when this assumption may be unsafe.  It tries to avoid triggering for provably safe code (by looking for the subsequent assignment, or by tracking string lengths when it can) but the logic isn't perfect so it's best to call the function with a byte count that's less than the destination size and follow the call immediately by an assignment of the nul to the destination.
Comment 3 Romain Geissler 2018-02-19 22:01:39 UTC
Ok there maybe a problem with the optimizer as you describe it in bug 84470. However why does variant 3 from comment #0 yields a warning too ?

    if (iCString)
    {
        strncpy(_cstring, iCString, 3);
        _cstring[3] = '\0';
    }
    _cstring[3] = '\0';

It is because the optimizer finds out that "_cstring[3] = '\0';" is both in the "if branch" and also after, thus eliminates the one from the "if branch", then the strncpy silencing warning mechanism doesn't work properly anymore ? I don't know very well what is the internal gcc IR, but do you think it would be possible to go beyond branches and basic block separation to find out that the next statement in

    if (iCString)
    {
        strncpy(_cstring, iCString, 3);
    }
    _cstring[3] = '\0';

really is "_cstring[3] = '\0';" (ie crossing basic block boundaries) ?
Comment 4 Martin Sebor 2018-02-19 22:06:49 UTC
Yes, the conditional assignment is eliminated in favor of the one in the next basic block.  Considering the first statement in the next basic block (rather than just the current one) is also the solution I'm looking into.
Comment 5 Martin Sebor 2018-02-20 02:51:48 UTC
Patch: https://gcc.gnu.org/ml/gcc-patches/2018-02/msg01141.html
Comment 6 Romain Geissler 2018-02-24 15:19:34 UTC
Hi,

Tried to apply this patch on top of current trunk. During my build process, I bootstrap a complete Linux/binutils/glibc/gcc toolchain following the Linux From Scratch guidelines.

Without the patch, the bootstrap works fine. With the patch, the bootstrap fails when using a newly built gcc 8, I try to build the Linux headers. Here are the logs I have:

+ cd /workdir/src/linux-2.6.32.23
+ make mrproper
+ make INSTALL_HDR_PATH=dest headers_install
  CHK     include/linux/version.h
  UPD     include/linux/version.h
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/basic/docproc
  HOSTCC  scripts/basic/hash
  HOSTCC  scripts/unifdef
In file included from /home/jenkins/workspace/OTF_Toolchain_release_4.0-YQJ6EBM33XTPWHNNQSFSZFEGBOIWUEOX32S2OZLINN43UBSUZTJA/output/build/temporary-system/install/include/string.h:630,
                 from scripts/unifdef.c:72:
scripts/unifdef.c: In function 'Mpass':
scripts/unifdef.c:377:28: internal compiler error: Segmentation fault
 static void Mpass (void) { strncpy(keyword, "if  ", 4); Pelif(); }

                            ^~~~~~~

scripts/unifdef.c:377:28: internal compiler error: Aborted
gcc: internal compiler error: Aborted signal terminated program cc1
Please submit a full bug report,
with preprocessed source if appropriate.
See <https://gcc.gnu.org/bugs/> for instructions.
scripts/Makefile.host:118: recipe for target 'scripts/unifdef' failed
make[1]: *** [scripts/unifdef] Error 4
Makefile:1089: recipe for target '__headers' failed
make: *** [__headers] Error 2


I expect that you would be able to reproduce the same error by downloading the Linux headers (https://www.kernel.org/pub/linux/kernel/v2.6/linux-2.6.32.23.tar.gz) and then:

make mrproper
make INSTALL_HDR_PATH=dest headers_install


This file is actually pretty simple. See https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/scripts/unifdef.c?h=v2.6.32.23#n377

...
static void Pelif (void) { print(); ignoreoff(); state(IS_PASS_MIDDLE); }
...
static void Mpass (void) { strncpy(keyword, "if  ", 4); Pelif(); }
...

Maybe the fact that the instruction that follows strncpy is a function call makes gcc seg faults ?

Cheers,
Romain
Comment 7 Martin Sebor 2018-02-24 15:24:23 UTC
Thanks for the early heads up!  Can you please attach the translation unit for the kernel file that GCC faults on?
Comment 8 Romain Geissler 2018-02-24 15:30:48 UTC
I am currently testing a little variant of your patch (check that "nextbb" if not NULL before trying to use it):

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c   (revision 257796)
+++ gcc/tree-ssa-strlen.c   (working copy)
@@ -1851,8 +1851,21 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
      avoid the truncation warning.  */
   gsi_next_nondebug (&gsi);
   gimple *next_stmt = gsi_stmt (gsi);
+  if (!next_stmt)
+    {
+      /* When there is no statement in the same basic block check
+    the immediate successor block.  */
+      basic_block bb = gimple_bb (stmt);
+      basic_block nextbb
+   = EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL;
+      if (nextbb)
+        {
+          gimple_stmt_iterator it = gsi_start_bb (nextbb);
+          next_stmt = gsi_stmt (it);
+        }
+    }
 
-  if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
+  if (next_stmt && is_gimple_assign (next_stmt))
     {
       tree lhs = gimple_assign_lhs (next_stmt);
       tree_code code = TREE_CODE (lhs);


If it doesn't work, I will provide you with the translation unit.
Comment 9 Romain Geissler 2018-02-24 16:34:29 UTC
Ok I was able to strip down the ICE to this very simple reproducer:

<<EOF
#include <string.h>

static char keyword[4];

static void f (void) { strncpy(keyword, "if  ", 4); }
EOF
Comment 10 Martin Sebor 2018-02-24 19:18:48 UTC
Thanks, I can reproduce it with that test case.  Checking for the basic block being null fixes the SEGV for me.  Let me retest this and post an update for review.

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 257963)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1856,8 +1856,20 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
      avoid the truncation warning.  */
   gsi_next_nondebug (&gsi);
   gimple *next_stmt = gsi_stmt (gsi);
+  if (!next_stmt)
+    {
+      /* When there is no statement in the same basic block check
+	 the immediate successor block.  */
+      if (basic_block bb = gimple_bb (stmt))
+	{
+	  basic_block nextbb
+	    = EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL;
+	  gimple_stmt_iterator it = gsi_start_bb (nextbb);
+	  next_stmt = gsi_stmt (it);
+	}
+    }
 
-  if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
+  if (next_stmt && is_gimple_assign (next_stmt))
     {
       tree lhs = gimple_assign_lhs (next_stmt);
       tree_code code = TREE_CODE (lhs);
Comment 11 Romain Geissler 2018-02-25 20:20:42 UTC
Hi,

Indeed this version of the patch doesn't have any segv. However it seems that it doesn't fix anymore the initial bug report. Does it actually passes the new tests you introduced in your patch ?

Unless I am mistaken (I am pretty sure I have applied the patch), the following code extracted from your tests still emit a warning with -O2

<<EOF
#define strncpy __builtin_strncpy

struct A
{ 
  char a[4];
};

void succ (struct A *p, const struct A *q)
{
  /* Verify that the assignment suppresses the warning for the conditional
     strcnpy call.  The conditional should be folded to true since the
     address of an array can never be null (see bug 84470).  */
  if (q->a)                                       
    strncpy (p->a, q->a, sizeof p->a - 1);    /* { dg-bogus "\\\[-Wstringop-truncation" } */

  p->a[sizeof p->a - 1] = 0;
}
EOF
Comment 12 Martin Sebor 2018-02-25 23:24:47 UTC
Yes, all the relevant tests pass with the patch.  There is no warning for either the test case in comment #0 or the one in comment #11.  The change from v1 of the patch is just the addition of test for null to avoid the ICE.
Comment 13 Romain Geissler 2018-02-26 01:28:47 UTC
Hi,

It looks like that the code in #comment 11 works when you build just with -O2, but not when you add debug symbols: -O2 -g. Do we have a way to ignore debug statements when looking for the next statement in the next basic block ?

Cheers,
Romain
Comment 14 Martin Sebor 2018-02-26 17:20:48 UTC
(In reply to Romain Geissler from comment #13)

Ah, right.  It's not skipping over debug statements.  That's easier to fix than pr84561.  This should do it:

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	(revision 257963)
+++ gcc/tree-ssa-strlen.c	(working copy)
@@ -1856,8 +1856,21 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi
      avoid the truncation warning.  */
   gsi_next_nondebug (&gsi);
   gimple *next_stmt = gsi_stmt (gsi);
+  if (!next_stmt)
+    {
+      /* When there is no statement in the same basic block check
+	 the immediate successor block.  */
+      if (basic_block bb = gimple_bb (stmt))
+	{
+	  basic_block nextbb
+	    = EDGE_COUNT (bb->succs) ? EDGE_SUCC (bb, 0)->dest : NULL;
+	  gimple_stmt_iterator it = gsi_start_bb (nextbb);
+	  gsi_next_nondebug (&it);
+	  next_stmt = gsi_stmt (it);
+	}
+    }
 
-  if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
+  if (next_stmt && is_gimple_assign (next_stmt))
     {
       tree lhs = gimple_assign_lhs (next_stmt);
       tree_code code = TREE_CODE (lhs);
Comment 15 Romain Geissler 2018-02-27 08:24:05 UTC
Hi,

This latest patch seems to fix the occurences I have in my own code. Thanks ;)

Cheers,
Romain
Comment 16 Richard Biener 2018-03-06 15:43:01 UTC
I still say simply walk to the next VDEF...
Comment 17 Martin Sebor 2018-03-07 19:31:17 UTC
Author: msebor
Date: Wed Mar  7 19:30:31 2018
New Revision: 258339

URL: https://gcc.gnu.org/viewcvs?rev=258339&root=gcc&view=rev
Log:
PR tree-optimization/84468 - bogus -Wstringop-truncation despite assignment after conditional strncpy

gcc/ChangeLog:

	PR tree-optimization/84468
	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Consider successor
	basic block when looking for nul assignment.

gcc/testsuite/ChangeLog:

	PR tree-optimization/84468
	* g++.dg/warn/Wstringop-truncation-2.C: New test.
	* gcc.dg/Wstringop-truncation.c: New test.
	* gcc.dg/Wstringop-truncation-2.c: New test.


Added:
    trunk/gcc/testsuite/g++.dg/warn/Wstringop-truncation-2.C
    trunk/gcc/testsuite/gcc.dg/Wstringop-truncation-2.c
    trunk/gcc/testsuite/gcc.dg/Wstringop-truncation.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/warn/Wstringop-truncation-1.C
    trunk/gcc/tree-ssa-strlen.c
Comment 18 Martin Sebor 2018-03-07 19:32:48 UTC
Fixed with r258339 on trunk.  For GCC 9 I'll look into something more sophisticated.