[Bug middle-end/49905] Better sanity checking on sprintf src & dest to produce warning for dodgy code ?

["msebor at gcc dot gnu.org" <gcc-bugzilla at gcc dot gnu dot org>]

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

--- Comment #13 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to David Binderman from comment #9)
> I tried a build of the gcc fortran compiler and I found this warning:
> 
> ../../../src/trunk/libgfortran/intrinsics/date_and_time.c:173:33: warning:
> ‘%+03d’ directive output truncated while writing ‘9’ bytes into a region of
> size ‘6’ [-Wformat-length=]
> 
> Source code is
> 
>     snprintf (zone, ZONE_LEN + 1, "%+03d%02d",
>           values[3] / 60, abs (values[3] % 60));
> 
> and
> 
> #define ZONE_LEN 5
> 
>   char zone[ZONE_LEN + 1];
> 
> While it is clear that the region size of 6 is fine, I can't see where the
> value 
> of 9 bytes is coming from.

Thanks for the testing!  I reproduced the warning the above with -O2 in a small
test case:

  extern int values [4];
  inline int abs(int x) { return x < 0 ? -x : x; }

  #define ZONE_LEN 5

  char zone[ZONE_LEN + 1];

  void f (void)
  {
    __builtin_snprintf (zone, ZONE_LEN + 1, "%+03d%02d",
                        values[3] / 60, abs (values[3] % 60));
  }

GCC doesn't know what values[3] evaluates to but since its type is int, the
Value Range Propagation (VRP) determines that the range of (values[3] / 60) is
[-35791394, 35791394], and the checker sees that as many as 9 bytes of space
for the output of any value in that range are required.

The checker prints two general kinds of diagnostics for individual directives:

1) When it knows the space in the destination is insufficient to format either
an exact value or a range of values bounded by some limit imposed by the
program is uses the words "writing" or "truncating."

2) When it doesn't know the exact value and there is no known range of values
beyond the limits of the type of the argument it uses the term "may write" or
"may truncate."

The case above falls into (1) but it could be changed to fall into (2) instead,
or to use a different phrase.  In either case, the diagnostic could also
mention the range of values it thinks the argument can take on.

(In reply to David Binderman from comment #9)
> I'd be happy for the code to compute minimum only and have maximum
> postponed for the future. One step at a time.

The checker has two levels:

1) At level one, it considers the least amount of space needed for the argument
of each directive (is assumes unknown integers have the value 1 and strings are
empty).  This corresponds to the minimum.  I would expect this to be
immediately useful in all existing code since it should have no (or few) false
positives.

2) At level two, it uses the maximum.  Unknown singed integers or type T have
the value T_MIN, unknown integers of unsigned type T_MAX.  That corresponds to
the maximum.  I envision this to be useful mainly in new code and for sanity
checking in existing code.

At both levels, the checker treats known values as exact (because it knows
exactly how many bytes of output each produces).  For ranges of values obtained
from VRP it uses the upper bound of the range.  Due to weaknesses in the range
information exposed by VRP we might want to tweak it this strategy or clarify
the diagnostic.