[Bug tree-optimization/104746] New: [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[Bug tree-optimization/104746] New: [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

            Bug ID: 104746
           Summary: [12 Regression] False positive for -Wformat-overflow=2
                    since r12-7033-g3c9f762ad02f398c
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: msebor at gcc dot gnu.org
  Target Milestone: ---

I see the following regression reduced from nfs-utils:

$ cat 1.i

unsigned long systemd_escape___trans_tmp_2;
char *systemd_escape_p;
int systemd_escape_len;
char generate_target_pipefs_path;
char *main_path;

char *systemd_escape2()
{
}

char *systemd_escape(char, char *suffix) {
  char *result;
  systemd_escape_p++;
  systemd_escape_len++;
  systemd_escape___trans_tmp_2 = __builtin_strlen(suffix);
  result =__builtin_malloc(systemd_escape_len + systemd_escape___trans_tmp_2 +
1);
  __builtin_sprintf(systemd_escape_p, suffix);
  return result;
}

void main()
{
  char *pipefs_unit = systemd_escape(generate_target_pipefs_path, ".mount");
  char *dirname = systemd_escape2();

  unsigned long len = __builtin_strlen(pipefs_unit) + __builtin_strlen(dirname)
+ 1;
  main_path = __builtin_malloc(len);

  __builtin_sprintf(main_path, "%s/%s", pipefs_unit, dirname);
}

$ gcc -Werror=format-overflow=2 1.i -flto -O2
XXX
1.i: In function ‘main’:
1.i:29:32: error: ‘%s’ directive output between 0 and 2147483653 bytes may
exceed minimum required size of 4095 [-Werror=format-overflow=]
   29 |   __builtin_sprintf(main_path, "%s/%s", pipefs_unit, dirname);
      |                                ^
1.i:29:32: note: assuming directive output of 1 byte
1.i:29:32: note: assuming directive output of 1 byte
lto1: some warnings being treated as errors
lto-wrapper: fatal error: gcc returned 1 exit status
compilation terminated.
/tmp/binutils/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status

Note it's fine w/o LTO:

$ gcc -Werror=format-overflow=2 1.i -O2

[Bug tree-optimization/104746] [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-03-01
     Ever confirmed|0                           |1
   Target Milestone|---                         |12.0

[Bug tree-optimization/104746] [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
The warning certainly looks cryptic but seems to actually point out a real bug
in the code: len is set to 1 less than the number of bytes the sprintf call
writes to the buffer (the two strings plus the slash character plus the
teminating nul byte).

That said, the warning persists even with a buffer of sufficient size, but then
disappears if the empty definition of systemd_escape2() is removed.  Since the
function fails to return a result the test case is invalid, I'm guessing
because it was reduced too far.  Can you provide a valid test case?

FYI, level 2 of -Wformat-overflow is designed to "warn also about calls that
might overflow the destination buffer given an argument of sufficient length or
magnitude" so it will have a higher rate of false positives in cases where some
arguments cannot be determined to be sufficiently constrained to avoid an
overflow.  In addition, there is no support for symbolic constraints involving
multiple arguments (like strlen(a) + strlen (b) < bufsize - 1) so the warning
is susceptible to false positives for calls involving such arguments, as in
sprintf(buf, "%s/%s", a, b).

[Bug tree-optimization/104746] [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|WAITING                     |RESOLVED

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #1)
> The warning certainly looks cryptic but seems to actually point out a real
> bug in the code: len is set to 1 less than the number of bytes the sprintf
> call writes to the buffer (the two strings plus the slash character plus the
> teminating nul byte).

Oh, I overlooked that. Thanks for the explanation, I'm going to report that to
the upstream project.

[Bug tree-optimization/104746] [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |NEW
         Resolution|INVALID                     |---

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
> That said, the warning persists even with a buffer of sufficient size, but
> then disappears if the empty definition of systemd_escape2() is removed. 
> Since the function fails to return a result the test case is invalid, I'm
> guessing because it was reduced too far.  Can you provide a valid test case?
> 

Sure, there's a valid test-case that is completely fine and we emit a warning:

$ cat problem.c
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
  const char *pipefs_path = argv[1];
  const char *dirname = argv[1];
  const char *suffix = ".mount";

  int len = strlen(pipefs_path);
  char *result = malloc(len + strlen(suffix) + 1);
  sprintf(result, "%s", suffix);

  char *path = malloc(strlen(dirname) + strlen(result) + 2);
  sprintf(path, "%s/%s", dirname, result);

  return 0;
}

[Bug tree-optimization/104746] [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

--- Comment #4 from Martin Liška <marxin at gcc dot gnu.org> ---
Btw. upstream applied the following patch:
http://git.linux-nfs.org/?p=steved/nfs-utils.git;a=blobdiff;f=systemd/rpc-pipefs-generator.c;h=7b2bb4f7fe7d27f6339ac7adf9562523b1395aef;hp=c24db567a45d9f544a1a094e0c0fbc5582f80da9;hb=7f8463fe702174bd613df9d308cc899af25ae02e;hpb=475857723dee84a68b675a56ca0ea2e81c2f626e

that seems to me like a papering over.

[Bug tree-optimization/104746] [12 Regression] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #3)

This is an example of the "symbolic constraints involving multiple arguments"
that I mentioned in comment #1.  There is no logic to determine from the
complex relationship between the lengths of the two strings that their sum also
constrains the output of the call to avoid the overflow.  A similar example of
the same problem is below.  The conditional guarantees that each of i and j
produces exactly one digit on output, but the all the warning logic considers
is the range of the arguments, which is [0, INT_MAX].  Unlike in the string
case, I think here Ranger could actually set the range of i and j to be [0, 9]
on the assumption the sum doesn't overflow, but that would still not avoid the
warning unless the code also checked the range of the sum.

$ cat b.c && gcc -O2 -S -Wall -Wformat-overflow=2 b.c

char a[3];

void f (int i, int j)
{
  if (i < 0 || j < 0 || i + j > 9)
    return;

  __builtin_sprintf (a, "%u%u", i, j);
}
b.c: In function ‘f’:
b.c:8:26: warning: ‘%u’ directive writing between 1 and 10 bytes into a region
of size 4 [-Wformat-overflow=]
    8 |   __builtin_sprintf (a, "%u%u", i, j);
      |                          ^~
b.c:8:25: note: using the range [0, 4294967295] for directive argument
    8 |   __builtin_sprintf (a, "%u%u", i, j);
      |                         ^~~~~~
b.c:8:25: note: using the range [0, 4294967295] for directive argument
b.c:8:3: note: ‘__builtin_sprintf’ output between 3 and 21 bytes into a
destination of size 4
    8 |   __builtin_sprintf (a, "%u%u", i, j);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12 Regression] False       |False positive for
                   |positive for                |-Wformat-overflow=2 since
                   |-Wformat-overflow=2 since   |r12-7033-g3c9f762ad02f398c
                   |r12-7033-g3c9f762ad02f398c  |

--- Comment #6 from Martin Sebor <msebor at gcc dot gnu.org> ---
None of these "false positives" is due to a bug in the warning code.  The
warning has been designed and documented to work this way.  What triggers more
instances of these warnings in GCC 12 is the more accurate range info courtesy
of Ranger.  Prior to GCC 12, the ranges were less accurate and sometimes
unavailable at all, and the warning is designed to avoid triggering in the
absence of any range info at all.

So I don't consider this a regression.

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[amacleod at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Andrew Macleod <amacleod at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com

--- Comment #7 from Andrew Macleod <amacleod at redhat dot com> ---

(In reply to Martin Sebor from comment #6)
> None of these "false positives" is due to a bug in the warning code.  The
> warning has been designed and documented to work this way.  What triggers
> more instances of these warnings in GCC 12 is the more accurate range info
> courtesy of Ranger.  Prior to GCC 12, the ranges were less accurate and
> sometimes unavailable at all, and the warning is designed to avoid
> triggering in the absence of any range info at all.
> 
> So I don't consider this a regression.

"Regression" is defined as didn't cause a problem before, but does now. Making
this a regression.

Besides, according to the warning:

size 4 [-Wformat-overflow=]
    8 |   __builtin_sprintf (a, "%u%u", i, j);
      |                          ^~
b.c:8:25: note: using the range [0, 4294967295] for directive argument
    8 |   __builtin_sprintf (a, "%u%u", i, j);
      |                         ^~~~~~
b.c:8:25: note: using the range [0, 4294967295] for directive argument
b.c:8:3: note: ‘__builtin_sprintf’ output between 3 and 21 bytes into a
destination of size 4
    8 |   __builtin_sprintf (a, "%u%u", i, j);

its using [0, 4294967295] as the range, which is [0, 0xFFFFFFFF] or varying..
so there isn't any new precision of ranges from ranger causing this? TVRYING
implies there is no range at all known.

Wouldnt we be seeing [0,9] if you were getting more precise ranges?

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
Andrew, quoting from the documentation for the warning:

  Unknown string arguments whose length cannot be assumed to be bounded either
by the directive’s precision, or by a finite set of string literals they may
evaluate to, or the character array they may point to, are assumed to be 1
character long. 

The length of the second string argument to the second sprintf call is assumed
to be bounded by the size of the array allocated by the first call to malloc. 
The malloc argument is at most 2147483654 (INT_MAX + strlen (".mount") + 1), so
the maximum length of the string is 2147483653.  That's also what the warning
prints.  The ability to determine and use the maximum length was added in
r12-7033 to avoid the warning reported in PR 104119.  Because GCC 11 doesn't
have this ability, it assumes the length of the string argument is 1, and so
the warning doesn't trigger there.  That could be considered a bug or
limitation in GCC 11.

The instance of the warning in the test case in comment #3 is designed point
out directives whose output may exceed the environmental limit of 4095 bytes
(the minimum the C standard requires implementations to support).  It's working
as designed and as intended.  If you don't like the design and want to propose
a change I suggest you submit a concrete proposal and we can discuss it.

As for my integer test from comment #7, you're right that if each argument was
in the range [0, 9] that the warning would be avoided.  I didn't get the limits
quite right.  A test case that should better illustrate the point I was making
about the constraints derived from relationships might go something like this:

char a[4];

void f (int i, int j)
{
  if (i < 0 || j < 0 || i + j > 19)
    return;

  __builtin_sprintf (a, "%u%u", i, j);
}

Here, setting the range of each of i and j on its own to [0, 19] isn't enough
to rule out a possible overflow; we also need to capture the constraint that if
one is two digits the other must be just one.  At the moment there is no logic
to determine that.  I think the corresponding test case for a possible
optimization is this:

void f (int i, int j)
{
  if (i < 0 || j < 0 || i + j > 19)
    return;

  if (i * j > 90)   // fold to false
    __builtin_abort ();
}

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

--- Comment #9 from Martin Sebor <msebor at gcc dot gnu.org> ---
Martin, since the warning is working correctly (even if it's arguably not as
clear as it could be), I'd like us to close this.  If you agree, can you please
go ahead and mark this as resolved (either invalid or wontfix)?

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #10 from Martin Liška <marxin at gcc dot gnu.org> ---
Won't be fixed.

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |NEW
         Resolution|WONTFIX                     |---

--- Comment #11 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #9)
> Martin, since the warning is working correctly (even if it's arguably not as
> clear as it could be), I'd like us to close this.  If you agree, can you
> please go ahead and mark this as resolved (either invalid or wontfix)?

All right, so I changed my mind. The warning is maybe working according to
documentation, but I think the constraints it assumes are basically wrong. If
it's unclear about a range of an expression, then it should not emit a warning
based on that.
Such types of warnings should be part of the static analyzer, where the level
of certainty is lower.
Leaving the issue here for future improvement.

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|NEW                         |RESOLVED

--- Comment #12 from Martin Sebor <msebor at gcc dot gnu.org> ---
The documented purpose of -Wformat-overflow=2 is to point out potential
problems, including those where an argument is not known to be sufficiently
constrained.  (Level 1 behaves close to what you expect.)  Level 2 is not
enabled in -Wall or -Wextra and must be explicitly enabled.  Different designe
choices are of course possible but since some projects are using it as is it's
useful as designed.  This is not a bug.

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[amacleod at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Andrew Macleod <amacleod at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |---
             Status|RESOLVED                    |REOPENED

--- Comment #13 from Andrew Macleod <amacleod at redhat dot com> ---
Why can't we leave this open? There have been VRP bugs open since 2005. There
was no intent to fix them back then, but they identify opportunities for
improvement.

I think this is a good placeholder for improvements in analyzing the parameters
of the warning. We can make more effort when there is more than one parameter
to see if the information can be further refined.  

  _4 = i_6(D) + j_7(D);
  if (_4 > 19)
    goto <bb 4>; [INV]
  else
    goto <bb 5>; [INV]

  <bb 5> :
  __builtin_sprintf (&a, "%u%u", i_6(D), j_7(D));


i_6 and j_7 are both understood to be [0,19], and it wouldn't take much more
work to find if there is a relationship between them, and refine the values.

They appear together in one statement _4 = i_6 + j_7, and _4 is known to be
[0,19] at the warning location. 

so [0,19] = i_6 + j_7

we can ask what the range of i_6 or j_7 is if the other one is refined.

[10,99] will create 2 characters of output, you can ask if i_6 is [10,19],
whats the range of j_7, and it will give you [0,9].   And then the warning
doesn't need to trigger. Likewise for i_6 computed from j_7 = [10, 19]. 

This could be generalized using [100,999] for 3 character outputs, [1000, 9999]
is 4 characters, etc.  And doing a bit more work trying to analyze the
parameters.

Those bits are all there now. This could be in the next release.


I'd even argue we should probably split this into 2 reports. The earlier
warning on:

  char *path = malloc(strlen(dirname) + strlen(result) + 2);
  sprintf(path, "%s/%s", dirname, result);

Seems like a different opportunity in which we could track/associate strlen
results with the string. ie

  result_17 = malloc (_5);
  sprintf (result_17, "%s", suffix_14);

  _6 = strlen (dirname_13);
  _7 = strlen (result_17);
  _8 = _6 + _7;
  _9 = _8 + 2;
  path_20 = malloc (_9);
  sprintf (path_20, "%s/%s", dirname_13, result_17);

_6 is length of dirname_13, and _7 is length of result_17,  Then we malloc an
object that is _6 + _7 + 2 and copy those 2 strings into it. 

With the appropriate pointer/string/strlen/malloc  associations it seems
deterministically knowable that we can avoid that warning too.

Any false positive we can potentially eliminate with some additional analysis
is worth tracking and considering IMO.  This PR is on the list of PRs I track
for possible future improvements.

[Bug tree-optimization/104746] False positive for -Wformat-overflow=2 since r12-7033-g3c9f762ad02f398c

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104746

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #14 from Martin Sebor <msebor at gcc dot gnu.org> ---
Andrew, I agree with tracking the improvement we discussed.  Because it's not
directly related to the warning for which this bug was opened (the 4095 limit),
and implementing it won't prevent this warning.  I've raised pr104922 to track
it.

I'm also open to revisiting the design behind this instance of the warning (the
4095 limit), reconsidering whether it's still useful (it was motivated by some
old sprintf implementations failing for large amounts of output -- see for
example https://bugzilla.redhat.com/show_bug.cgi?id=441945), or perhaps
exposing it under a target hook.  But to avoid confusion I'd prefer to do that
separately of this bug report.  It doesn't illustrate a false positive or
reflect a bug in the warning.