User account creation filtered due to spam.

Bug 79257 - spurious -Wformat-overflow=1 warning with -O2 and sanitizer
Summary: spurious -Wformat-overflow=1 warning with -O2 and sanitizer
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 7.0.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2017-01-27 15:24 UTC by Vincent Lefèvre
Modified: 2017-07-05 01:49 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-01-28 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent Lefèvre 2017-01-27 15:24:59 UTC
With r244974, on the following C code:

#include <stdio.h>

void a (void);

int main (void)
{
  char buffer[2];
  int i;

  for (i = 0; i < 2; i++)
    {
      if (i == 0)
        a ();
      sprintf (buffer, "%d", i);
    }
  return 0;
}

I get:

cventin:~> =gcc -O2 -Wformat-overflow=1 -fsanitize=undefined -c tst.c
tst.c: In function ‘main’:
tst.c:14:25: warning: ‘%d’ directive writing between 1 and 10 bytes into a region of size 2 [-Wformat-overflow=]
       sprintf (buffer, "%d", i);
                         ^~
tst.c:14:24: note: directive argument in the range [0, 2147483646]
       sprintf (buffer, "%d", i);
                        ^~~~
tst.c:14:7: note: ‘sprintf’ output between 2 and 11 bytes into a destination of size 2
       sprintf (buffer, "%d", i);
       ^~~~~~~~~~~~~~~~~~~~~~~~~
tst.c:14:25: warning: ‘%d’ directive writing between 1 and 10 bytes into a region of size 2 [-Wformat-overflow=]
       sprintf (buffer, "%d", i);
                         ^~
tst.c:14:24: note: directive argument in the range [0, 2147483646]
       sprintf (buffer, "%d", i);
                        ^~~~
tst.c:14:7: note: ‘sprintf’ output between 2 and 11 bytes into a destination of size 2
       sprintf (buffer, "%d", i);
       ^~~~~~~~~~~~~~~~~~~~~~~~~

If I remove any of -O2, -Wformat-overflow=1 or -fsanitize=undefined, I no longer get a warning. Ditto if I remove the test on i or the call to a().

Note: I've found this bug when building the MPFR tests (tl2b.c).
Comment 1 Jakub Jelinek 2017-01-27 19:15:44 UTC
I think the reason is that the printf-return-length2 pass is scheduled very long after vrp1 and shortly before vrp2, optimizations in between those usually (ok, ccp is exception if it figured out something from non-zero bits) just preserve or invalidate the range info, but don't really create it.  Here in particular ivopts pass creates new IVs which don't really have range info then.
What is the reason for not putting the printf-return-length2 pass right after vrp2 or shortly after it, instead of shortly before it?
Otherwise, we'd need to have some lightweight VRP pass (without assertion expression handling, without looping or only with limited looping, without other expensive stuff) scheduled in more places.
Comment 2 Martin Sebor 2017-01-28 01:02:42 UTC
The null pointer instrumentation added by the sanitizer causes GCC trouble.  We've seen it with the original implementation of the -Wnonnull warning, it still causes null warnings out of builtin-ssa-sprintf.c in bootstrap-ubsan, and it looks like it's also (indirectly) behind this warning.  In this case I see no reason why the sanitizer adds the null pointer check for buffer when there's no way for it to be null.  There should be a way to avoid it in the basic case, though I suspect the problem might be more general in nature (and why Richard argues for merging some of these passes with VRP).

  <bb 5> [0.00%]:
  if (&buffer == 0B)
    goto <bb 9>; [0.04%]
  else
    goto <bb 8>; [99.96%]

  <bb 9> [0.00%]:
  __builtin___ubsan_handle_nonnull_arg (&*.Lubsan_data0);

I think the sprintf-return-value pass is before vrp2 because it sets range info that vrp2 then makes use of.  The test below (extracted from the test suite) fails when the pass is moved after vrp2.

void f (const char *s)
{
  int n = __builtin_snprintf (0, 0, "%.*s%08x", 1, s, 1);
  if (7 < n && n < 10) return;
  __builtin_abort ();
}
Comment 3 Martin Sebor 2017-01-28 18:14:20 UTC
A few more comments:

1) moving the gimple-ssa-sprintf pass after vrp2 suppresses just one of the two warnings (the argument in the second call to sprintf is still in the same range)
2) it's not the null pointer sanitizer that's causing the warnings but rather the integer overflow sanitizer
3) the warnings don't seem related to r244974 (i.e., the range of the argument is the same and the same warnings are still issued)
4) similar warnings are issued prior to the recent changes committed to resolve bug 78703 so it's not a recent change or regression
5) the warning is issued on the basis that the range of the argument values is a subrange of its type (as opposed to one whose value is completely unbounded, such a function argument); i.e., an attempt has been made to constrain the argument, presumably to make it fit in the buffer, but the attempt doesn't go far enough

It's worth noting that this problem isn't specific to the -Wformat-overflow warning.  It affects all get_range_info clients outside VRP that use the upper bound of the range (I think that's just -Walloca-larger-than in GCC 7, but the list is likely to grow in the future).

Ideally, the range info would be as accurate outside VRP as within it.  Barring that, the range info consumers might need to be more closely integrated with the VRP pass in some way as Richard suggests.

I'm not sure what's the appropriate solution for GCC 7.  Relaxing the logic in the -Wformat-{overflow,truncation} checker to avoid this instance of the warning will cause false negatives and won't do anything for the other warning(s).  Disabling the warning altogether when -fsanitize=undefined is specified also doesn't seem like a good fix.

I would tend to defer the decision until more code has been exposed to the warning (the Fedora mass rebuild).  If this turns out to be a pervasive problem it's easy to implement one of the changes above.  It it's rare living with it would seem like an acceptable tradeoff.  In GCC 8 I'd like to look into improving the warning to avoid this case as well as a number of others we know about.
Comment 4 Martin Sebor 2017-01-28 19:14:59 UTC
One thing I noticed that suggests another possible (and, IMO, the ideal for this specific case) solution: the warning goes away and GCC emits far better code when the controlling expression of the loop is (i != 2) as opposed to (i < 2).  Contrast the optimal code emitted for foo() below to that for bar():

$ cat c.c && gcc -O2 -c -Wall -fdump-tree-optimized=/dev/stdout -fsanitize=undefined c.c

void a (void);

char buffer[2];

void foo (void)
{
  for (int i = 0; i != 2; i++)
    {
      if (i == 0)
        a ();
      __builtin_sprintf (buffer, "%d", i);
    }
}

void bar (void)
{
  for (int i = 0; i < 2; i++)
    {
      if (i == 0)
        a ();
      __builtin_sprintf (buffer, "%d", i);
    }
}


;; Function foo (foo, funcdef_no=0, decl_uid=2120, cgraph_uid=0, symbol_order=1)

foo ()
{
  <bb 2> [28.07%]:
  a ();
  __builtin_sprintf (&buffer, "%d", 0);
  __builtin_sprintf (&buffer, "%d", 1); [tail call]
  return;

}


c.c: In function ‘bar’:
c.c:21:35: warning: ‘%d’ directive writing between 1 and 10 bytes into a region of size 2 [-Wformat-overflow=]
       __builtin_sprintf (buffer, "%d", i);
                                   ^~
c.c:21:34: note: directive argument in the range [0, 2147483646]
       __builtin_sprintf (buffer, "%d", i);
                                  ^~~~
c.c:21:7: note: ‘__builtin_sprintf’ output between 2 and 11 bytes into a destination of size 2
       __builtin_sprintf (buffer, "%d", i);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

;; Function bar (bar, funcdef_no=1, decl_uid=2127, cgraph_uid=1, symbol_order=2)

Removing basic block 8
bar ()
{
  unsigned long ivtmp.7;
  int i;
  unsigned int _1;
  int _8;
  unsigned int _10;

  <bb 2> [15.00%]:
  goto <bb 4>; [100.00%]

  <bb 3> [70.00%]:
  ivtmp.7_7 = ivtmp.7_3 + 1;

  <bb 4> [85.00%]:
  # ivtmp.7_3 = PHI <ivtmp.7_7(3), 0(2)>
  i_13 = (int) ivtmp.7_3;
  if (i_13 == 0)
    goto <bb 5>; [33.00%]
  else
    goto <bb 6>; [67.00%]

  <bb 5> [28.05%]:
  a ();
  __builtin_sprintf (&buffer, "%d", 0);
  goto <bb 3>; [100.00%]

  <bb 6> [56.95%]:
  __builtin_sprintf (&buffer, "%d", i_13);
  _1 = (unsigned int) ivtmp.7_3;
  _10 = _1 + 1;
  _8 = (int) _10;
  if (_8 <= 1)
    goto <bb 3>; [73.66%]
  else
    goto <bb 7>; [26.34%]

  <bb 7> [15.00%]:
  return;
}
Comment 5 Pierre Chapuis 2017-07-04 19:04:34 UTC
I think I can reproduce something similar *without* the sanitizer.

Using GCC 7.1.1, with:


#include <stdio.h>
int main () {
    int i; char obuf[3];
    int start = 0x00;
    for (i = start; i <= 0xff; ++i) {
        sprintf(obuf, "%02x", i);
    }
    return 0;
}


I get:


demo.c: In function ‘main’:
demo.c:6:23: warning: ‘sprintf’ may write a terminating nul past the end of the destination [-Wformat-overflow=]
         sprintf(obuf, "%02x", i);
                       ^~~~~~
demo.c:6:9: note: ‘sprintf’ output between 3 and 4 bytes into a destination of size 3
         sprintf(obuf, "%02x", i);
         ^~~~~~~~~~~~~~~~~~~~~~~~


At O1 I don't get the warning.

If I set `start` to `0xfb` or `0xfc`, I get the same warning. If I set it to `0xfe` or `0xff` I don't get a warning. If I set it to `0xfd` I get the warning *twice*:


demo.c: In function ‘main’:
demo.c:6:23: warning: ‘sprintf’ may write a terminating nul past the end of the destination [-Wformat-overflow=]
         sprintf(obuf, "%02x", i);
                       ^~~~~~
demo.c:6:9: note: ‘sprintf’ output between 3 and 4 bytes into a destination of size 3
         sprintf(obuf, "%02x", i);
         ^~~~~~~~~~~~~~~~~~~~~~~~
demo.c:6:23: warning: ‘sprintf’ may write a terminating nul past the end of the destination [-Wformat-overflow=]
         sprintf(obuf, "%02x", i);
                       ^~~~~~
demo.c:6:9: note: ‘sprintf’ output between 3 and 4 bytes into a destination of size 3
         sprintf(obuf, "%02x", i);
         ^~~~~~~~~~~~~~~~~~~~~~~~
Comment 6 Pierre Chapuis 2017-07-04 19:09:17 UTC
(Note this is my first time reporting something to GCC, if you think I should have opened a new ticked because it may be a slightly different bug please tell me.)
Comment 7 Vincent Lefèvre 2017-07-04 23:36:12 UTC
(In reply to Pierre Chapuis from comment #5)
> I think I can reproduce something similar *without* the sanitizer.
> 
> Using GCC 7.1.1, with:
> 
> #include <stdio.h>
> int main () {
>     int i; char obuf[3];
>     int start = 0x00;
>     for (i = start; i <= 0xff; ++i) {
>         sprintf(obuf, "%02x", i);
>     }
>     return 0;
> }

If 0xff is replaced by 0xfe, the warning disappears. Is the issue due to the fact that i can be 0x100 in the loop (thus with one additional character), but GCC doesn't notice that this value will not be printed?

Similarly, I get the warning with

  for (i = start; i <= 0xfe; i += 2)

but not with

  for (i = start; i <= 0xfd; i += 2)

> I get:
> 
> demo.c: In function ‘main’:
> demo.c:6:23: warning: ‘sprintf’ may write a terminating nul past the end of
> the destination [-Wformat-overflow=]
>          sprintf(obuf, "%02x", i);
>                        ^~~~~~
> demo.c:6:9: note: ‘sprintf’ output between 3 and 4 bytes into a destination
> of size 3
>          sprintf(obuf, "%02x", i);
>          ^~~~~~~~~~~~~~~~~~~~~~~~

IMHO, this seems to be a different bug, otherwise instead of "4 bytes", you would get the number of bytes corresponding to INT_MAX-1 in the message, as in comment 0.

> If I set `start` to `0xfb` or `0xfc`, I get the same warning. If I set it to
> `0xfe` or `0xff` I don't get a warning. If I set it to `0xfd` I get the
> warning *twice*:

This might be due to loop unrolling. You can see with -S that the generated asm code has a loop for start = 0xfc, but not for start = 0xfd, 0xfe or 0xff. With start = 0xfd, 3 sprintf are generated, and perhaps there's a warning for 2 of them. Then I wonder why start = 0xfe no longer triggers the warning.
Comment 8 Martin Sebor 2017-07-05 01:49:54 UTC
(In reply to Pierre Chapuis from comment #5)

That's a different bug, the same one as the root cause behind the false positive in bug 78969, comment 4.  The range information available outside the VRP pass via get_range_info() is available (unlike in the test case in comment #0) but it's off by one.  It probably deserves a separate bug report, distinct from both this bug and pr78969, and this one can be resolved as a duplicate of the latter.