[PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

Bug 78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation
of -9223372036854775808 cannot be represented in type 'long int'
points out an integer overflow bug in the pass caught by ubsan.
The bug was due to negating a number without checking for equality
to INT_MIN.

In addition, my recent change to fix 78521 introduced a call to
abs() that broke the Solaris bootstrap:

   https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00161.html

While fixing these two problems I noticed that the rest of the pass
wasn't handling the corner case of a width with the value of INT_MIN
specified via an argument to the asterisk, such as in:

   int n = snprintf(0, 0, "%*i", INT_MIN, 0);

This is undefined behavior because negative width is supposed to be
treated as the left justification flag followed by a positive width
(thus resulting in INT_MAX + 1 bytes).  This problem affected all
integer and floating point directives.

Finally, while there, I decided to include in information messages
a bit of detail about ranges of floating point values that was
missing.  I did this to help answer questions like those raised
earlier this week by Gerald here ("where does the 317 come from?):

   https://gcc.gnu.org/ml/gcc/2016-11/msg00102.html

The attached patch adjusts the pass to handle these problems.

Martin

[-- Attachment #2: gcc-78608.diff --]
[-- Type: text/x-patch, Size: 22769 bytes --]

PR tree-optimization/78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation of -9223372036854775808 cannot be represented in type 'long int'

gcc/ChangeLog:

	PR tree-optimization/78608
	* gimple-ssa-sprintf.c (tree_digits): Avoid negating a minimum signed
	value.
	(get_width_and_precision): Same.
	(format_integer): Use HOST_WIDE_INT for expected sprintf return value
	to allow for width being the absolte value of INT_MIN.
	(format_floating): Use HOST_WIDE_INT for width and precision.  Store
	argument type for use in diagnostics.  Use target INT_MAX rather than
	the host constant.
	(format_floating): Reuse get_width_and_precision instead of hadcoding
	the same.
	(maybe_get_value_range_from_type): New function.
	(format_directive): Treat INT_MAX + 1 as a valid (if excessive) byte
	count.  Call maybe_get_value_range_from_type.

gcc/testsuite/ChangeLog:

	PR tree-optimization/78608
	* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Add test cases.

Index: gcc/gimple-ssa-sprintf.c
===================================================================
--- gcc/gimple-ssa-sprintf.c	(revision 243196)
+++ gcc/gimple-ssa-sprintf.c	(working copy)
@@ -565,8 +565,14 @@ tree_digits (tree x, int base, bool plus, bool pre
       if (tree_fits_shwi_p (x))
 	{
 	  HOST_WIDE_INT i = tree_to_shwi (x);
-	  if (i < 0)
+	  if (HOST_WIDE_INT_MIN == i)
 	    {
+	      /* Avoid undefined behavior due to negating a minimum.  */
+	      absval = HOST_WIDE_INT_MAX;
+	      res = 1;
+	    }
+	  else if (i < 0)
+	    {
 	      absval = -i;
 	      res = 1;
 	    }
@@ -774,7 +780,23 @@ get_width_and_precision (const conversion_spec &sp
   if (spec.star_width)
     {
       if (TREE_CODE (spec.star_width) == INTEGER_CST)
-	width = abs (tree_to_shwi (spec.star_width));
+	{
+	  width = tree_to_shwi (spec.star_width);
+	  if (width < 0)
+	    {
+	      if (width == HOST_WIDE_INT_MIN)
+		{
+		  /* Avoid undefined behavior due to negating a minimum.
+		     This case will be diagnosed since it will result in
+		     more than INT_MAX bytes on output, either by the
+		     directive itself (when INT_MAX < HOST_WIDE_INT_MAX)
+		     or by the format function itself.  */
+		  width = HOST_WIDE_INT_MAX;
+		}
+	      else
+		width = -width;
+	    }
+	}
       else
 	width = HOST_WIDE_INT_MIN;
     }
@@ -913,7 +935,7 @@ format_integer (const conversion_spec &spec, tree
 	  gcc_unreachable ();
 	}
 
-      int len;
+      HOST_WIDE_INT len;
 
       if ((prec == HOST_WIDE_INT_MIN || prec == 0) && integer_zerop (arg))
 	{
@@ -1118,8 +1140,8 @@ format_integer (const conversion_spec &spec, tree
   return res;
 }
 
-/* Return the number of bytes to format using the format specifier
-   SPEC the largest value in the real floating TYPE.  */
+/* Return the number of bytes to format, using the format specifier SPEC
+   and precision PREC, the largest value in the real floating TYPE.  */
 
 static int
 format_floating_max (tree type, char spec, int prec = -1)
@@ -1170,7 +1192,8 @@ format_floating_max (tree type, char spec, int pre
    is used when the directive argument or its value isn't known.  */
 
 static fmtresult
-format_floating (const conversion_spec &spec, int width, int prec)
+format_floating (const conversion_spec &spec,
+		 HOST_WIDE_INT width, HOST_WIDE_INT prec)
 {
   tree type;
   bool ldbl = false;
@@ -1214,6 +1237,13 @@ static fmtresult
       logexpdigs = ilog (expdigs, 10);
     }
 
+  /* Store only the type in RES.ARGMIN.  If a diagnostic needs to be
+     issued for the directive the range of values used to determine
+     the number of bytes for it will be computed at that time.  This
+     avoids building the real constants corresponding to the range
+     for every directive even if no diagnostic is issued.  */
+  res.argmin = type;
+
   switch (spec.specifier)
     {
     case 'A':
@@ -1221,13 +1251,15 @@ static fmtresult
       {
 	/* The minimum output is "0x.p+0".  */
 	res.range.min = 6 + (prec > 0 ? prec : 0);
-	res.range.max = (width == INT_MIN
-			 ? HOST_WIDE_INT_MAX
+	/* The maximum is the greater of widh and the number of bytes
+	   produced by formatting -TYPE_MAX.  When width is unknown,
+	   the maximum is -INT_MIN (this is undefined and diagnosed).  */
+	res.range.max = (width == HOST_WIDE_INT_MIN
+			 ? target_int_max () + 1
 			 : format_floating_max (type, 'a', prec));
-
 	/* The output of "%a" is fully specified only when precision
 	   is explicitly specified and width isn't unknown.  */
-	res.bounded = INT_MIN != width && -1 < prec;
+	res.bounded = HOST_WIDE_INT_MIN != width && -1 < prec;
 	break;
       }
 
@@ -1243,13 +1275,13 @@ static fmtresult
 	/* Unless width is uknown the maximum output is the minimum plus
 	   sign (unless already included), plus the difference between
 	   the minimum exponent of 2 and the maximum exponent for the type.  */
-	res.range.max = (width == INT_MIN
-			 ? HOST_WIDE_INT_M1U
+	res.range.max = (width == HOST_WIDE_INT_MIN
+			 ? target_int_max () + 1
 			 : res.range.min + !sign + logexpdigs - 2);
 
 	/* "%e" is fully specified and the range of bytes is bounded
 	   unless width is unknown.  */
-	res.bounded = INT_MIN != width;
+	res.bounded = HOST_WIDE_INT_MIN != width;
 	break;
       }
 
@@ -1259,17 +1291,18 @@ static fmtresult
 	/* The minimum output is "1.234567" regardless of the value
 	   of the actual argument.  */
 	res.range.min = 2 + (prec < 0 ? 6 : prec);
-
-	/* Compute the maximum just once.  */
+	
+	/* As an optimization, compute the maximum just once.  */
 	static const int f_max[] = {
 	  format_floating_max (double_type_node, 'f'),
 	  format_floating_max (long_double_type_node, 'f')
 	};
-	res.range.max = width == INT_MIN ? HOST_WIDE_INT_MAX : f_max [ldbl];
+	res.range.max = (width == HOST_WIDE_INT_MIN
+			 ? target_int_max () + 1 : f_max[ldbl]);
 
 	/* "%f" is fully specified and the range of bytes is bounded
 	   unless width is unknown.  */
-	res.bounded = INT_MIN != width;
+	res.bounded = HOST_WIDE_INT_MIN != width;
 	break;
       }
     case 'G':
@@ -1278,16 +1311,17 @@ static fmtresult
 	/* The minimum is the same as for '%F'.  */
 	res.range.min = 2 + (prec < 0 ? 6 : prec);
 
-	/* Compute the maximum just once.  */
+	/* As an optimization, compute the maximum just once.  */
 	static const int g_max[] = {
 	  format_floating_max (double_type_node, 'g'),
 	  format_floating_max (long_double_type_node, 'g')
 	};
-	res.range.max = width == INT_MIN ? HOST_WIDE_INT_MAX : g_max [ldbl];
+	res.range.max = (width == HOST_WIDE_INT_MIN
+			 ? HOST_WIDE_INT_MAX : g_max[ldbl]);
 
 	/* "%g" is fully specified and the range of bytes is bounded
 	   unless width is unknown.  */
-	res.bounded = INT_MIN != width;
+	res.bounded = HOST_WIDE_INT_MIN != width;
 	break;
       }
 
@@ -1316,42 +1350,23 @@ format_floating (const conversion_spec &spec, tree
   /* Set WIDTH to -1 when it's not specified, to INT_MIN when it is
      specified by the asterisk to an unknown value, and otherwise to
      a non-negative value corresponding to the specified width.  */
-  int width = -1;
-  int prec = -1;
+  HOST_WIDE_INT width;
+  HOST_WIDE_INT prec;
+  get_width_and_precision (spec, &width, &prec);
 
+  /* FIXME: Handle specified precision with an unknown value.  */
+  if (prec == HOST_WIDE_INT_MIN)
+    return fmtresult ();
+
   /* The minimum and maximum number of bytes produced by the directive.  */
   fmtresult res;
   res.constant = arg && TREE_CODE (arg) == REAL_CST;
 
-  if (spec.have_width)
-    width = spec.width;
-  else if (spec.star_width)
+  if (res.constant
+      && !spec.have_precision
+      && !spec.star_precision
+      && TOUPPER (spec.specifier) != 'A')
     {
-      if (TREE_CODE (spec.star_width) == INTEGER_CST)
-	{
-	  width = tree_to_shwi (spec.star_width);
-	  if (width < 0)
-	    width = -width;
-	}
-      else
-	width = INT_MIN;
-    }
-
-  if (spec.have_precision)
-    prec = spec.precision;
-  else if (spec.star_precision)
-    {
-      if (TREE_CODE (spec.star_precision) == INTEGER_CST)
-	prec = tree_to_shwi (spec.star_precision);
-      else
-	{
-	  /* FIXME: Handle non-constant precision.  */
-	  res.range.min = res.range.max = HOST_WIDE_INT_M1U;
-	  return res;
-	}
-    }
-  else if (res.constant && TOUPPER (spec.specifier) != 'A')
-    {
       /* Specify the precision explicitly since mpfr_sprintf defaults
 	 to zero.  */
       prec = 6;
@@ -1384,11 +1399,9 @@ format_floating (const conversion_spec &spec, tree
 	if (spec.get_flag (*pf))
 	  *pfmt++ = *pf;
 
-      /* Append width when specified and precision.  */
-      if (-1 < width)
-	pfmt += sprintf (pfmt, "%i", width);
+      /* Append precision but not width.  */
       if (-1 < prec)
-	pfmt += sprintf (pfmt, ".%i", prec);
+	pfmt += sprintf (pfmt, ".%i", (int)prec);
 
       /* Append the MPFR 'R' floating type specifier (no length modifier
 	 is necessary or allowed by MPFR for mpfr_t values).  */
@@ -1412,10 +1425,14 @@ format_floating (const conversion_spec &spec, tree
 	  /* Format it and store the result in the corresponding
 	     member of the result struct.  */
 	  *minmax[i] = mpfr_snprintf (NULL, 0, fmtstr, mpfrval);
+
+	  /* Adjust the count if it's less than the specified width.  */
+	  if (0 < width && *minmax[i] < (unsigned HOST_WIDE_INT)width)
+	    *minmax[i] = width;
 	}
 
       /* The range of output is known even if the result isn't bounded.  */
-      if (width == INT_MIN)
+      if (width == HOST_WIDE_INT_MIN)
 	{
 	  res.knownrange = false;
 	  res.range.max = HOST_WIDE_INT_MAX;
@@ -1684,6 +1701,44 @@ format_string (const conversion_spec &spec, tree a
   return res;
 }
 
+/* Helper to construct tree nodes corresponding to the minimum and
+   maxium values for a type.  When *ARGMIN is a real floating type
+   set [*ARGMIN, *ARGMAX] to the range of minimum and maximum values
+   of the type.  Otherwise do nothing.  */
+
+static void
+maybe_get_value_range_from_type (tree *argmin, tree *argmax)
+{
+  if (TREE_CODE (*argmin) != REAL_TYPE)
+    return;
+
+  /* *ARGMIN is actually a type.  */
+  tree type = *argmin;
+
+  machine_mode mode = TYPE_MODE (type);
+
+  /* IBM Extended mode.  */
+  if (MODE_COMPOSITE_P (mode))
+    mode = DFmode;
+
+  /* Get the real type format desription for the target.  */
+  const real_format *rfmt = REAL_MODE_FORMAT (mode);
+  REAL_VALUE_TYPE rv;
+
+  {
+    /* Create a TYPE_MAX.  */
+    char buf[256];
+    get_max_float (rfmt, buf, sizeof buf);
+    real_from_string (&rv, buf);
+  }
+
+  *argmax = build_real (type,
+			real_value_from_int_cst (type, integer_zero_node));
+
+  /* Create a TYPE_MIN by negating TYPE_MAX.  */
+  *argmin = fold_build1 (NEGATE_EXPR, type, build_real (type, rv));
+}
+
 /* Compute the length of the output resulting from the conversion
    specification SPEC with the argument ARG in a call described by INFO
    and update the overall result of the call in *RES.  The format directive
@@ -1738,9 +1793,12 @@ format_directive (const pass_sprintf_length::call_
   if (!fmtres.knownrange)
     {
       /* Only when the range is known, check it against the host value
-	 of INT_MAX.  Otherwise the range doesn't correspond to known
+	 of INT_MAX + 1.  The plus one allows for the corner case of
+	 width being specified by asterisk as INT_MIN.  Larger values
+	 are excessive and indication of some error expected to disable
+	 the counter(s).  Otherwise the range doesn't correspond to known
 	 values of the argument.  */
-      if (fmtres.range.max >= target_int_max ())
+      if (fmtres.range.max > target_int_max () + 1)
 	{
 	  /* Normalize the MAX counter to avoid having to deal with it
 	     later.  The counter can be less than HOST_WIDE_INT_M1U
@@ -1754,7 +1812,7 @@ format_directive (const pass_sprintf_length::call_
 	  res->number_chars = HOST_WIDE_INT_M1U;
 	}
 
-      if (fmtres.range.min >= target_int_max ())
+      if (fmtres.range.min > target_int_max () + 1)
 	{
 	  /* Disable exact length checking after a failure to determine
 	     even the minimum number of characters (it shouldn't happen
@@ -1794,7 +1852,7 @@ format_directive (const pass_sprintf_length::call_
 				    (int)cvtlen, cvtbeg, fmtres.range.min,
 				    navail);
 		}
-	      else if (fmtres.range.max < HOST_WIDE_INT_MAX)
+	      else if (fmtres.range.max < target_int_max ())
 		{
 		  const char* fmtstr
 		    = (info.bounded
@@ -1866,6 +1924,13 @@ format_directive (const pass_sprintf_length::call_
 
 	  if (warned && fmtres.argmin)
 	    {
+	      /* As an optimization when fmtres.argmin is a type, get
+		 the range of values [TYPE_MIN, TYPE_MAX] from the type.
+		 This makes it possible to avoid computing these for
+		 every numerical argument of every directive even when
+		 there is no diagnostic.  */
+	      maybe_get_value_range_from_type (&fmtres.argmin, &fmtres.argmax);
+
 	      if (fmtres.argmin == fmtres.argmax)
 		inform (info.fmtloc, "directive argument %qE", fmtres.argmin);
 	      else if (fmtres.bounded)
Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c	(revision 243196)
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c	(working copy)
@@ -10,6 +10,7 @@
 #endif
 
 #define INT_MAX __INT_MAX__
+#define INT_MIN (-INT_MAX - 1)
 
 char buffer [256];
 extern char *ptr;
@@ -97,6 +98,9 @@ void test_sprintf_c_const (void)
   T (-1, "%*cX", INT_MAX - 2, '1');
   T (-1, "%*cX", INT_MAX - 1, '1');
   T (-1, "%*cX", INT_MAX,     '1'); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*c",  INT_MIN + 1, '1');
+  T (-1, "%*c",  INT_MIN,     '1'); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
 /* Verify that no warning is issued for calls that write into a flexible
@@ -263,6 +267,9 @@ void test_sprintf_chk_s_const (void)
   T (-1, "%*s",  INT_MAX,     "");
   T (-1, "X%*s", INT_MAX,     ""); /* { dg-warning "directive output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 
+   T (-1, "%-*s", INT_MIN + 1,  "");
+   T (-1, "%-*s", INT_MIN,  ""); /* { dg-warning "directive output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
   /* Multiple directives.  */
 
   T (1, "%s%s", "", "");
@@ -455,6 +462,14 @@ void test_sprintf_chk_hh_const (void)
   T (5, "%0*hhd %0*hhi", 1, 12, 1, 123); /* { dg-warning ".%0\\*hhi. directive writing 3 bytes into a region of size 2" } */
   T (5, "%0*hhd %0*hhi", 2, 12, 3, 123); /* { dg-warning ".%0\\*hhi. directive writing 3 bytes into a region of size 2" } */
 
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*hhi",  INT_MAX - 1, 2);
+  T (-1, "%*hhi",  INT_MAX,     3);
+  T (-1, "%*hhiX", INT_MAX,     4); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*hhi",  INT_MIN + 1, 5);
+  T (-1, "%*hhi",  INT_MIN,     6); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
   /* FIXME: Move the boundary test cases into a file of their own that's
      exercised only on targets with the matching type limits (otherwise
      they'll fail).  */
@@ -560,6 +575,14 @@ void test_sprintf_chk_h_const (void)
   T (1, "%.0hX",        0);
   T (1, "%#.0hX",       0);
 
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*hi",  INT_MAX - 1, 2);
+  T (-1, "%*ho",  INT_MAX,     3);
+  T (-1, "%*hoX", INT_MAX,     3); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*hu",  INT_MIN + 1, 4);
+  T (-1, "%*hx",  INT_MIN,     5); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
 #undef MAX
 #define MAX   65535
 
@@ -643,6 +666,14 @@ void test_sprintf_chk_integer_const (void)
   T (1, "%.0X",         0);
   T (1, "%#.0X",        0);
 
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*i",   INT_MAX - 1, 2);
+  T (-1, "%*i",   INT_MAX,     2);
+  T (-1, "%*oX",  INT_MAX,     3); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*u",  INT_MIN + 1, 4);
+  T (-1, "%*x",  INT_MIN,     5); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
   T ( 7, "%1$i%2$i%3$i",     1, 23, 456);
   T ( 8, "%1$i%2$i%3$i%1$i", 1, 23, 456);
   T ( 8, "%1$i%2$i%3$i%2$i", 1, 23, 456);   /* { dg-warning "nul past the end" } */
@@ -964,6 +995,14 @@ void test_sprintf_chk_a_const (void)
 
   T (7, "%.a",    0.0);
   T (7, "%.0a",   0.0);
+
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*a",  INT_MAX - 1, 2.0);
+  T (-1, "%*a",  INT_MAX,     2.0);
+  T (-1, "%*aX", INT_MAX,     3.0); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*a",  INT_MIN + 1, 4.0);
+  T (-1, "%*a",  INT_MIN,     5.0); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
 void test_sprintf_chk_e_const (void)
@@ -1021,6 +1060,14 @@ void test_sprintf_chk_e_const (void)
   T (12, "%Le", 9.9999997e+99L);/* { dg-warning "terminating nul" } */
   T (12, "%Le", 9.9999998e+99L);/* { dg-warning "terminating nul" } */
   T (12, "%Le", 9.9999999e+99L);/* { dg-warning "terminating nul" } */
+
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*e",  INT_MAX - 2, 1.0);
+  T (-1, "%*e",  INT_MAX - 1, 2.0);
+  T (-1, "%*eX", INT_MAX,     3.0); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*e",  INT_MIN + 1, 4.0);
+  T (-1, "%*e",  INT_MIN,     5.0); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
 /* At -Wformat-length level 1 unknown numbers are assumed to have
@@ -1200,6 +1247,14 @@ void test_sprintf_chk_hh_nonconst (int w, int p, i
   T (2, "%*.1hhi",  w, 123);    /* { dg-warning "into a region" } */
   T (2, "%*.2hhi",  w, 123);    /* { dg-warning "into a region" } */
   T (2, "%*.3hhi",  w, 123);    /* { dg-warning "into a region" } */
+
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*hhi",  INT_MAX - 1, a);
+  T (-1, "%*hhi",  INT_MAX,     a);
+  T (-1, "%*hhiX", INT_MAX,     a); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*hhi",  INT_MIN + 1, a);
+  T (-1, "%*hhi",  INT_MIN,     a); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
 /* Exercise the h length modifier with all integer specifiers and
@@ -1246,6 +1301,14 @@ void test_sprintf_chk_h_nonconst (int a)
   T (3, "%2ho",         a);
   T (3, "%2hu",         a);
   T (3, "%2hx",         a);
+
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*hd",  INT_MAX - 1, a);
+  T (-1, "%*hi",  INT_MAX,     a);
+  T (-1, "%*hiX", INT_MAX,     a); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*hi",  INT_MIN + 1, a);
+  T (-1, "%*hi",  INT_MIN,     a); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
 /* Exercise all integer specifiers with no modifier and a non-constant
@@ -1294,6 +1357,14 @@ void test_sprintf_chk_int_nonconst (int w, int p,
   T (3, "%2x",          a);
 
   T (1, "%.*d",      p, a);
+
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*d",  INT_MAX - 1, a);
+  T (-1, "%*i",  INT_MAX,     a);
+  T (-1, "%*oX", INT_MAX,     a); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*u",  INT_MIN + 1, a);
+  T (-1, "%*x",  INT_MIN,     a); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
 void test_sprintf_chk_e_nonconst (int w, int p, double d)
@@ -1336,14 +1407,31 @@ void test_sprintf_chk_e_nonconst (int w, int p, do
   T ( 8, "%.1e",        d);
 
   T ( 0, "%*e",      0, d);           /* { dg-warning "writing between 12 and 14 bytes into a region of size 0" } */
+
+  /* The number of bytes written by the directive below is actually between
+     12 and INT_MAX + 1 but mentioning the upper bound would be confusing
+     because it could only happen if w were INT_MIN, which is both highly
+     unlikely and undefined.  */
   T ( 0, "%*e",      w, d);           /* { dg-warning "writing 12 or more bytes into a region of size 0" } */
+
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*e",  INT_MAX - 1, d);
+  T (-1, "%*e",  INT_MAX,     d);
+  T (-1, "%*eX", INT_MAX,     d); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*e",  INT_MIN + 1, d);
+  T (-1, "%*e",  INT_MIN,     d); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
-void test_sprintf_chk_f_nonconst (double d)
+void test_sprintf_chk_f_nonconst (int w, int p, double d)
 {
-  T (-1, "%F",          d);
-  T (-1, "%lF",         d);
+  T (-1, "%F",           d);
+  T (-1, "%lF",          d);
 
+  T (-1, "%*f",       w, d);
+  T (-1, "%.*f",      p, d);
+  T (-1, "%*.*f", w,  p, d);
+
   T ( 0, "%F",          d);           /* { dg-warning "into a region" } */
   T ( 0, "%f",          d);           /* { dg-warning "into a region" } */
   T ( 1, "%F",          d);           /* { dg-warning "into a region" } */
@@ -1360,10 +1448,29 @@ void test_sprintf_chk_e_nonconst (int w, int p, do
   T ( 6, "%f",          d);           /* { dg-warning "into a region" } */
   T ( 7, "%F",          d);           /* { dg-warning "into a region" } */
   T ( 7, "%f",          d);           /* { dg-warning "into a region" } */
+  T ( 7, "%*f",      w, d);           /* { dg-warning "into a region" } */
+
+  /* The unknown precision below could reduce the number of bytes to less
+     than 7 so no warning is expected.  */
+  T ( 7, "%.*f",     p, d);
+  T ( 7, "%*.*f", w, p, d);
+
   T ( 8, "%F",          d);           /* { dg-warning "nul past the end" } */
   T ( 8, "%f",          d);           /* { dg-warning "nul past the end" } */
   T ( 9, "%F",          d);
   T ( 9, "%f",          d);
+
+  T (-1, "%*f",       w, d);
+  T (-1, "%.*f",      p, d);
+  T (-1, "%*.*f", w,  p, d);
+
+  /* Verify the edge case of excessive width specified by asterisk.  */
+  T (-1, "%*f",  INT_MAX - 1, d);
+  T (-1, "%*f",  INT_MAX,     d);
+  T (-1, "%*fX", INT_MAX,     d); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
+
+  T (-1, "%*f",  INT_MIN + 1, d);
+  T (-1, "%*f",  INT_MIN,     d); /* { dg-warning "output of \[0-9\]+ bytes causes result to exceed .INT_MAX." } */
 }
 
 /* Tests for __builtin_vsprintf_chk are the same as those for

Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Martin Sebor]

Ping: https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html

(I would have almost forgotten about this if it weren't for bug
78786.  While working on a fix for it I keep thinking that some
of the changes I'm making look like they should have already been
made.)

Thanks
Martin

On 12/02/2016 05:36 PM, Martin Sebor wrote:
> Bug 78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation
> of -9223372036854775808 cannot be represented in type 'long int'
> points out an integer overflow bug in the pass caught by ubsan.
> The bug was due to negating a number without checking for equality
> to INT_MIN.
>
> In addition, my recent change to fix 78521 introduced a call to
> abs() that broke the Solaris bootstrap:
>
>   https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00161.html
>
> While fixing these two problems I noticed that the rest of the pass
> wasn't handling the corner case of a width with the value of INT_MIN
> specified via an argument to the asterisk, such as in:
>
>   int n = snprintf(0, 0, "%*i", INT_MIN, 0);
>
> This is undefined behavior because negative width is supposed to be
> treated as the left justification flag followed by a positive width
> (thus resulting in INT_MAX + 1 bytes).  This problem affected all
> integer and floating point directives.
>
> Finally, while there, I decided to include in information messages
> a bit of detail about ranges of floating point values that was
> missing.  I did this to help answer questions like those raised
> earlier this week by Gerald here ("where does the 317 come from?):
>
>   https://gcc.gnu.org/ml/gcc/2016-11/msg00102.html
>
> The attached patch adjusts the pass to handle these problems.
>
> Martin

Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Jeff Law]

On 12/13/2016 04:49 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html
>
> (I would have almost forgotten about this if it weren't for bug
> 78786.  While working on a fix for it I keep thinking that some
> of the changes I'm making look like they should have already been
> made.)
It's not forgotten on my end :-)


jeff

Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Jeff Law]

On 12/02/2016 05:36 PM, Martin Sebor wrote:
> Bug 78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation
> of -9223372036854775808 cannot be represented in type 'long int'
> points out an integer overflow bug in the pass caught by ubsan.
> The bug was due to negating a number without checking for equality
> to INT_MIN.
>
> In addition, my recent change to fix 78521 introduced a call to
> abs() that broke the Solaris bootstrap:
>
>   https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00161.html
>
> While fixing these two problems I noticed that the rest of the pass
> wasn't handling the corner case of a width with the value of INT_MIN
> specified via an argument to the asterisk, such as in:
>
>   int n = snprintf(0, 0, "%*i", INT_MIN, 0);
>
> This is undefined behavior because negative width is supposed to be
> treated as the left justification flag followed by a positive width
> (thus resulting in INT_MAX + 1 bytes).  This problem affected all
> integer and floating point directives.
>
> Finally, while there, I decided to include in information messages
> a bit of detail about ranges of floating point values that was
> missing.  I did this to help answer questions like those raised
> earlier this week by Gerald here ("where does the 317 come from?):
>
>   https://gcc.gnu.org/ml/gcc/2016-11/msg00102.html
>
> The attached patch adjusts the pass to handle these problems.
>
> Martin
>
> gcc-78608.diff
>
>
> PR tree-optimization/78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation of -9223372036854775808 cannot be represented in type 'long int'
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/78608
> 	* gimple-ssa-sprintf.c (tree_digits): Avoid negating a minimum signed
> 	value.
> 	(get_width_and_precision): Same.
> 	(format_integer): Use HOST_WIDE_INT for expected sprintf return value
> 	to allow for width being the absolte value of INT_MIN.
> 	(format_floating): Use HOST_WIDE_INT for width and precision.  Store
> 	argument type for use in diagnostics.  Use target INT_MAX rather than
> 	the host constant.
> 	(format_floating): Reuse get_width_and_precision instead of hadcoding
s/hadcoding/hardcoding

> 	the same.
> 	(maybe_get_value_range_from_type): New function.
> 	(format_directive): Treat INT_MAX + 1 as a valid (if excessive) byte
> 	count.  Call maybe_get_value_range_from_type.
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/78608
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Add test cases.
So I've been going back and forth on whether or not to suggest a slight 
change in how we're working.

Specifically the practice of lumping multiple fixes into a single patch 
-- I realize that it's usually the case that you're finding related 
issues so there's a desire to address them as a group.  Doing things 
that way also tends to avoid interdependent patches.

The problem is that makes reviewing more difficult and thus I'm much 
less likely to knock it out as soon as it flys by my inbox.   And I 
think I've seen several trivial to review, but important fixes inside 
larger, harder to review changes.

Let's try to keep patches to addressing one issue for the future.  If 
you have multiple related issues, you should consider a series of patches.

Hopefully that will cut down on the number of things are getting stalled.

>
> Index: gcc/gimple-ssa-sprintf.c
> ===================================================================
> --- gcc/gimple-ssa-sprintf.c	(revision 243196)
> +++ gcc/gimple-ssa-sprintf.c	(working copy)
> @@ -565,8 +565,14 @@ tree_digits (tree x, int base, bool plus, bool pre
>        if (tree_fits_shwi_p (x))
>  	{
>  	  HOST_WIDE_INT i = tree_to_shwi (x);
> -	  if (i < 0)
> +	  if (HOST_WIDE_INT_MIN == i)
nit.  I think most folks would probably prefer this as
if (i == HOST_WIDE_INT_MIN).

HOST_WIDE_INT_MIN is a constant and when we can write an expression in 
either order variable OP const is the preferred order.

You seem to be going back and forth between both styles.  Let' try to 
stick with variable OP const.  I don't think you need to go back and fix 
all of the existing const OP variable instances right now, but we may in 
the future.


> @@ -1221,13 +1251,15 @@ static fmtresult
>        {
>  	/* The minimum output is "0x.p+0".  */
>  	res.range.min = 6 + (prec > 0 ? prec : 0);
> -	res.range.max = (width == INT_MIN
> -			 ? HOST_WIDE_INT_MAX
> +	/* The maximum is the greater of widh and the number of bytes
s/widh/width/


> @@ -1316,42 +1350,23 @@ format_floating (const conversion_spec &spec, tree
>    /* Set WIDTH to -1 when it's not specified, to INT_MIN when it is
>       specified by the asterisk to an unknown value, and otherwise to
>       a non-negative value corresponding to the specified width.  */
> -  int width = -1;
> -  int prec = -1;
> +  HOST_WIDE_INT width;
> +  HOST_WIDE_INT prec;
> +  get_width_and_precision (spec, &width, &prec);
>
> +  /* FIXME: Handle specified precision with an unknown value.  */
> +  if (prec == HOST_WIDE_INT_MIN)
> +    return fmtresult ();
Is this something that needs to be fixed now or was this a marker for 
future?

> +	  /* Adjust the count if it's less than the specified width.  */
> +	  if (0 < width && *minmax[i] < (unsigned HOST_WIDE_INT)width)
> +	    *minmax[i] = width;
width > 0 && ...



> @@ -1684,6 +1701,44 @@ format_string (const conversion_spec &spec, tree a
>    return res;
>  }
>
> +/* Helper to construct tree nodes corresponding to the minimum and
> +   maxium values for a type.  When *ARGMIN is a real floating type
s/maxium/maximum/


> +
> +  /* Get the real type format desription for the target.  */
s/desription/description/

If the FIXME was a future thing, then this is OK with the nits fixed. 
If the FIXME was a marker for something you intended to address now and 
just forgot, then we either need another iteration or a follow-up patch 
depending on the severity of the FIXME in your  mind.

jeff

Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Martin Sebor]

> So I've been going back and forth on whether or not to suggest a slight
> change in how we're working.
>
> Specifically the practice of lumping multiple fixes into a single patch
> -- I realize that it's usually the case that you're finding related
> issues so there's a desire to address them as a group.  Doing things
> that way also tends to avoid interdependent patches.
>
> The problem is that makes reviewing more difficult and thus I'm much
> less likely to knock it out as soon as it flys by my inbox.   And I
> think I've seen several trivial to review, but important fixes inside
> larger, harder to review changes.
>
> Let's try to keep patches to addressing one issue for the future.  If
> you have multiple related issues, you should consider a series of patches.
>
> Hopefully that will cut down on the number of things are getting stalled.

Thanks for the review and the suggestion(s).  I also prefer smaller
patches.  I've only gotten into the habit of grouping them together
because of the cost of testing each on it own, and to streamline code
reviews (for everyone involved).  But I agree that the process you'd
like to follow makes the changes more tractable.

>> -      if (i < 0)
>> +      if (HOST_WIDE_INT_MIN == i)
> nit.  I think most folks would probably prefer this as
> if (i == HOST_WIDE_INT_MIN).
>
> HOST_WIDE_INT_MIN is a constant and when we can write an expression in
> either order variable OP const is the preferred order.
>
> You seem to be going back and forth between both styles.  Let' try to
> stick with variable OP const.  I don't think you need to go back and fix
> all of the existing const OP variable instances right now, but we may in
> the future.

I learned the first style (const OP variable, and also using a less
than in favor of other relational operators) many years ago and I'm
trying to unlearn it for GCC.  It's a hard habit to break.

FWIW, the const OP variable style used to be recommended to avoid
subtle bugs due to typos like 'if (var = CST)'  But I realize that
with -Wparentheses warning on this there is no need for the style
when using GCC.

The 'if (X < Y)' style comes from years of writing C++ template code
that couldn't assume that any other operators were defined for the
user-defined type the template might be instantiated on.  So instead
of writing 'if (X >= Y)' we'd write 'if (!(X < Y)' everywhere.  All
other relational operators were taboo.

>> +  if (prec == HOST_WIDE_INT_MIN)
>> +    return fmtresult ();
> Is this something that needs to be fixed now or was this a marker for
> future?

I think the patch for bug 78696 resolves the FIXME (but see below).

> If the FIXME was a future thing, then this is OK with the nits fixed. If
> the FIXME was a marker for something you intended to address now and
> just forgot, then we either need another iteration or a follow-up patch
> depending on the severity of the FIXME in your  mind.

The patch for bug 78696 resolves the FIXME but there will need to
be another change here to improve the handling of unknown precisions
and widths:

1) Use get_range_info to constrain non-constant width and precision,
    and if that fails...

2) ...use some reasonable default (e.g., based on the precision of
    the type of the directive).

Without these changes sprintf (d, "%.*f", p, 0.0) causes

   warning: writing a terminating nul past the end of the destination

even at -Wformat-length=1 with no good way to suppress it.  At
-Wformat-length=2, sprintf(d, "%.*i", 0) also causes a similar:

   warning: ‘%.*i’ directive writing 0 or more bytes into a region...

also with no way to suppress it. (The two warnings should also be
worded the same.)

I've started to work on a general fix for both of these in my patch
for bug 78703 - -fprintf-return-value floating point handling
incorrect in locales with a mulltibyte decimal point.  These fit
well together because they both separate the heuristic-based warning
byte counters from the actual counters good for optimization (which
are based on what GCC knows for certain).

Martin

Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Jeff Law]

On 12/14/2016 09:41 AM, Martin Sebor wrote:
>
>>> -      if (i < 0)
>>> +      if (HOST_WIDE_INT_MIN == i)
>> nit.  I think most folks would probably prefer this as
>> if (i == HOST_WIDE_INT_MIN).
>>
>> HOST_WIDE_INT_MIN is a constant and when we can write an expression in
>> either order variable OP const is the preferred order.
>>
>> You seem to be going back and forth between both styles.  Let' try to
>> stick with variable OP const.  I don't think you need to go back and fix
>> all of the existing const OP variable instances right now, but we may in
>> the future.
>
> I learned the first style (const OP variable, and also using a less
> than in favor of other relational operators) many years ago and I'm
> trying to unlearn it for GCC.  It's a hard habit to break.
>
> FWIW, the const OP variable style used to be recommended to avoid
> subtle bugs due to typos like 'if (var = CST)'  But I realize that
> with -Wparentheses warning on this there is no need for the style
> when using GCC.
Yea.   And I can see some argument to use the CST == var style.   But 
I'm not up to pushing on that in our coding standards.  I realize it'll 
take time to unlearn :-)


>
> I think the patch for bug 78696 resolves the FIXME (but see below).
>
>> If the FIXME was a future thing, then this is OK with the nits fixed. If
>> the FIXME was a marker for something you intended to address now and
>> just forgot, then we either need another iteration or a follow-up patch
>> depending on the severity of the FIXME in your  mind.
>
> The patch for bug 78696 resolves the FIXME but there will need to
> be another change here to improve the handling of unknown precisions
> and widths:
>
> 1) Use get_range_info to constrain non-constant width and precision,
>    and if that fails...
>
> 2) ...use some reasonable default (e.g., based on the precision of
>    the type of the directive).
>
> Without these changes sprintf (d, "%.*f", p, 0.0) causes
>
>   warning: writing a terminating nul past the end of the destination
>
> even at -Wformat-length=1 with no good way to suppress it.  At
> -Wformat-length=2, sprintf(d, "%.*i", 0) also causes a similar:
>
>   warning: ‘%.*i’ directive writing 0 or more bytes into a region...
>
> also with no way to suppress it. (The two warnings should also be
> worded the same.)
>
> I've started to work on a general fix for both of these in my patch
> for bug 78703 - -fprintf-return-value floating point handling
> incorrect in locales with a mulltibyte decimal point.  These fit
> well together because they both separate the heuristic-based warning
> byte counters from the actual counters good for optimization (which
> are based on what GCC knows for certain).
Understood.  Thanks for the update.

jeff

Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Martin Sebor]

> If the FIXME was a future thing, then this is OK with the nits fixed. If
> the FIXME was a marker for something you intended to address now and
> just forgot, then we either need another iteration or a follow-up patch
> depending on the severity of the FIXME in your  mind.

As we discussed privately, I went ahead and committed just a minimal
patch to resolve the bug in r244511.  Most of the rest of the changes
in the patch have either already been made to resolve other bugs or
are a part of the bigger patch for bug 78703 that's under review.

I wanted to resolve the bug without making merging the bigger patch
harder than it needs to be.

Thanks
Martin