[PATCH] avoid folding snprintf calls with bounds > INT_MAX (PR 87096)

[PATCH] avoid folding snprintf calls with bounds > INT_MAX (PR 87096)

[Martin Sebor]

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

POSIX requires snprintf to fail with EOVERFLOW when the specified
bound exceeds INT_MAX.  This requirement conflicts with the C
requirements on valid calls to the function and isn't universally
implemented (e.g., Glibc doesn't seem to follow it, and GCC has
historically not paid heed to it either).  Nevertheless, there
are implementations that do respect it (Solaris being one of
them), and it seems that GCC should make a tricky situation
even more treacherous for programmers by returning different
values from some calls to the function than the library would.
This is also what bug 87096 requests.  The patch also adds
a warning that was missing from a subset of these troublesome
calls.

The attached patch disables the snprintf constant folding and
range optimization for calls to it and other related bounded
functions when the bound is not known not to exceed INT_MAX.

Tested on x86_64-linux.

Martin

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

PR tree-optimization/87096 - Optimised snprintf is not POSIX conformant

gcc/ChangeLog:

	PR rtl-optimization/87096
	* gimple-ssa-sprintf.c (sprintf_dom_walker::handle_gimple_call): Avoid
	folding calls whose bound may exceed INT_MAX.  Diagnose bound ranges
	that exceed the limit.

gcc/testsuite/ChangeLog:

	PR tree-optimization/87096
	* gcc.dg/tree-ssa/builtin-snprintf-4.c: New test.

Index: gcc/gimple-ssa-sprintf.c
===================================================================
--- gcc/gimple-ssa-sprintf.c	(revision 267063)
+++ gcc/gimple-ssa-sprintf.c	(working copy)
@@ -3990,6 +3990,7 @@ sprintf_dom_walker::handle_gimple_call (gimple_stm
   /* True when the destination size is constant as opposed to the lower
      or upper bound of a range.  */
   bool dstsize_cst_p = true;
+  bool posunder4k = true;
 
   if (idx_dstsize == UINT_MAX)
     {
@@ -4022,11 +4023,20 @@ sprintf_dom_walker::handle_gimple_call (gimple_stm
 			    "specified bound %wu exceeds maximum object size "
 			    "%wu",
 			    dstsize, target_size_max () / 2);
+	      /* POSIX requires snprintf to fail if DSTSIZE is greater
+		 than INT_MAX.  Even though not all POSIX implementations
+		 conform to the requirement, avoid folding in this case.  */
+	      posunder4k = false;
 	    }
 	  else if (dstsize > target_int_max ())
-	    warning_at (gimple_location (info.callstmt), info.warnopt (),
-			"specified bound %wu exceeds %<INT_MAX%>",
-			dstsize);
+	    {
+	      warning_at (gimple_location (info.callstmt), info.warnopt (),
+			  "specified bound %wu exceeds %<INT_MAX%>",
+			  dstsize);
+	      /* POSIX requires snprintf to fail if DSTSIZE is greater
+		 than INT_MAX.  Avoid folding in that case.  */
+	      posunder4k = false;
+	    }
 	}
       else if (TREE_CODE (size) == SSA_NAME)
 	{
@@ -4035,10 +4045,30 @@ sprintf_dom_walker::handle_gimple_call (gimple_stm
 	     of them at level 2.  */
 	  value_range *vr = evrp_range_analyzer.get_value_range (size);
 	  if (range_int_cst_p (vr))
-	    dstsize = (warn_level < 2
-		       ? TREE_INT_CST_LOW (vr->max ())
-		       : TREE_INT_CST_LOW (vr->min ()));
+	    {
+	      unsigned HOST_WIDE_INT minsize = TREE_INT_CST_LOW (vr->min ());
+	      unsigned HOST_WIDE_INT maxsize = TREE_INT_CST_LOW (vr->max ());
+	      dstsize = warn_level < 2 ? maxsize : minsize;
 
+	      if (minsize > target_int_max ())
+		warning_at (gimple_location (info.callstmt), info.warnopt (),
+			    "specified bound range [%wu, %wu] exceeds "
+			    "%<INT_MAX%>",
+			    minsize, maxsize);
+
+	      /* POSIX requires snprintf to fail if DSTSIZE is greater
+		 than INT_MAX.  Avoid folding if that's possible.  */
+	      if (maxsize > target_int_max ())
+		posunder4k = false;
+	    }
+	  else if (vr->varying_p ())
+	    {
+	      /* POSIX requires snprintf to fail if DSTSIZE is greater
+		 than INT_MAX.  Since SIZE's range is unknown, avoid
+		 folding.  */
+	      posunder4k = false;
+	    }
+
 	  /* The destination size is not constant.  If the function is
 	     bounded (e.g., snprintf) a lower bound of zero doesn't
 	     necessarily imply it can be eliminated.  */
@@ -4122,7 +4152,7 @@ sprintf_dom_walker::handle_gimple_call (gimple_stm
      directive.  Clear POSUNDER4K for the former set of functions and set
      it to true for the latter (it can only be cleared later, but it is
      never set to true again).  */
-  res.posunder4k = dstptr;
+  res.posunder4k = posunder4k && dstptr;
 
   bool success = compute_format_length (info, &res);
   if (res.warned)
Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-4.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-4.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-snprintf-4.c	(working copy)
@@ -0,0 +1,156 @@
+/* PR tree-optimization/87096 - "Optimised" snprintf is not POSIX conformant
+   Verify that calls to snprintf with size in excess of INT_MAX are not
+   treated as successful.
+   It would be valid for GCC to fold some of these calls to a negative
+   value provided it also arranged to set errno to EOVERFLOW.  If that
+   is ever implemented this test will need to be adjusted.
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized -ftrack-macro-expansion=0" } */
+
+#include "../range.h"
+
+typedef __builtin_va_list va_list;
+
+extern int snprintf (char*, size_t, const char*, ...);
+extern int vsnprintf (char*, size_t, const char*, va_list);
+
+#define CAT(x, y) x ## y
+#define CONCAT(x, y) CAT (x, y)
+#define FAILNAME(name) CONCAT (call_ ## name ##_on_line_, __LINE__)
+
+#define FAIL(name) do {				\
+    extern void FAILNAME (name) (void);		\
+    FAILNAME (name)();				\
+  } while (0)
+
+/* Macro to emit a call to function named
+     call_in_true_branch_not_eliminated_on_line_NNN()
+   for each expression that's expected to fold to false but that
+   GCC does not fold.  The dg-final scan-tree-dump-time directive
+   at the bottom of the test verifies that no such call appears
+   in output.  */
+#define ELIM(expr)							\
+  if ((expr)) FAIL (in_true_branch_not_eliminated); else (void)0
+
+/* Macro to emit a call to a function named
+     call_made_in_{true,false}_branch_on_line_NNN()
+   for each call that's expected to be retained.  The dg-final
+   scan-tree-dump-time directive at the bottom of the test verifies
+   that the expected number of both kinds of calls appears in output
+   (a pair for each line with the invocation of the KEEP() macro.  */
+#define KEEP(expr)				\
+  if (expr)					\
+    FAIL (made_in_true_branch);			\
+  else						\
+    FAIL (made_in_false_branch)
+
+extern void sink (int, ...);
+#define sink(...) sink (0, __VA_ARGS__)
+
+#define WARN(N, expr)				\
+  do {						\
+    char a[N];					\
+    expr;					\
+    sink (a);					\
+  } while (0)
+
+
+static const size_t imax = __INT_MAX__;
+static const size_t imaxp1 = imax + 1;
+
+static const size_t dmax = __PTRDIFF_MAX__;
+static const size_t dmaxp1 = dmax + 1;
+
+static const size_t szmax = __SIZE_MAX__;
+static const size_t szmaxm1 = __SIZE_MAX__ - 1;
+
+
+void test_size_cst (char **d)
+{
+  ELIM (0 > snprintf (*d++, imax, "%s", ""));
+
+  KEEP (0 > snprintf (*d++, imaxp1, "%s", ""));   /* { dg-warning "\\\[-Wformat-truncation=]" } */
+
+  KEEP (0 > snprintf (*d++, dmax, "%s", ""));     /* { dg-warning "\\\[-Wformat-truncation=]" } */
+  KEEP (0 > snprintf (*d++, dmaxp1, "%s", ""));   /* { dg-warning "\\\[-Wformat-truncation=]" } */
+  KEEP (0 > snprintf (*d++, szmaxm1, "%s", ""));  /* { dg-warning "\\\[-Wformat-truncation=]" } */
+  KEEP (0 > snprintf (*d++, szmax, "%s", ""));    /* { dg-warning "\\\[-Wformat-truncation=]" } */
+}
+
+
+void test_size_cst_va (char **d, va_list va)
+{
+  ELIM (0 > vsnprintf (*d++, imax, " ", va));
+
+  KEEP (0 > vsnprintf (*d++, imaxp1, " ", va));   /* { dg-warning "\\\[-Wformat-truncation=]" } */
+
+  KEEP (0 > vsnprintf (*d++, dmax, " ", va));     /* { dg-warning "\\\[-Wformat-truncation=]" } */
+  KEEP (0 > vsnprintf (*d++, dmaxp1, " ", va));   /* { dg-warning "\\\[-Wformat-truncation=]" } */
+  KEEP (0 > vsnprintf (*d++, szmaxm1, " ", va));  /* { dg-warning "\\\[-Wformat-truncation=]" } */
+  KEEP (0 > vsnprintf (*d++, szmax, " ", va));    /* { dg-warning "\\\[-Wformat-truncation=]" } */
+}
+
+
+void test_size_range (char **d)
+{
+  size_t r = UR (imax - 1, imax);
+  ELIM (0 > snprintf (*d++, r, "%s", ""));
+
+  r = UR (imax, imax + 1);
+  KEEP (0 > snprintf (*d++, r, "%s", ""));
+
+  r = UR (imaxp1, imaxp1 + 1);
+  KEEP (0 > snprintf (*d++, r, "%s", ""));        /* { dg-warning "specified bound range \\\[\[0-9\]+, \[0-9\]+] exceeds .INT_MAX." } */
+
+  r = UR (dmax, dmaxp1);
+  KEEP (0 > snprintf (*d++, r, "%s", ""));        /* { dg-warning "\\\[-Wformat-truncation=]" } */
+
+  r = UR (dmaxp1, dmaxp1 + 1);
+  KEEP (0 > snprintf (*d++, r, "%s", ""));        /* { dg-warning "\\\[-Wformat-truncation=]" } */
+
+  r = UR (szmaxm1, szmax);
+  KEEP (0 > snprintf (*d++, r, "%s", ""));        /* { dg-warning "\\\[-Wformat-truncation=]" } */
+}
+
+
+void test_size_range_va (char **d, va_list va)
+{
+  size_t r = UR (imax - 1, imax);
+  ELIM (0 > vsnprintf (*d++, r, " ", va));
+
+  r = UR (imax, imax + 1);
+  KEEP (0 > vsnprintf (*d++, r, " ", va));
+
+  r = UR (imaxp1, imaxp1 + 1);
+  KEEP (0 > vsnprintf (*d++, r, " ", va));        /* { dg-warning "specified bound range \\\[\[0-9\]+, \[0-9\]+] exceeds .INT_MAX." } */
+
+  r = UR (dmax, dmaxp1);
+  KEEP (0 > vsnprintf (*d++, r, " ", va));        /* { dg-warning "\\\[-Wformat-truncation=]" } */
+
+  r = UR (dmaxp1, dmaxp1 + 1);
+  KEEP (0 > vsnprintf (*d++, r, " ", va));        /* { dg-warning "\\\[-Wformat-truncation=]" } */
+
+  r = UR (szmaxm1, szmax);
+  KEEP (0 > vsnprintf (*d++, r, " ", va));        /* { dg-warning "\\\[-Wformat-truncation=]" } */
+}
+
+
+void test_size_varying (char **d, size_t n)
+{
+  KEEP (0 > snprintf (*d++, n, "%s", ""));
+
+  n += 1;
+  KEEP (0 > snprintf (*d++, n, "%s", ""));
+}
+
+
+void test_size_varying_va (char **d, size_t n, va_list va)
+{
+  KEEP (0 > vsnprintf (*d++, n, " ", va));
+
+  n += 1;
+  KEEP (0 > vsnprintf (*d++, n, " ", va));
+}
+
+/* { dg-final { scan-tree-dump-times " = snprintf" 12 "optimized"} }
+   { dg-final { scan-tree-dump-times " = vsnprintf" 12 "optimized"} } */

Re: [PATCH] avoid folding snprintf calls with bounds > INT_MAX (PR 87096)

[Jeff Law]

On 12/12/18 4:18 PM, Martin Sebor wrote:
> POSIX requires snprintf to fail with EOVERFLOW when the specified
> bound exceeds INT_MAX.  This requirement conflicts with the C
> requirements on valid calls to the function and isn't universally
> implemented (e.g., Glibc doesn't seem to follow it, and GCC has
> historically not paid heed to it either).  Nevertheless, there
> are implementations that do respect it (Solaris being one of
> them), and it seems that GCC should make a tricky situation
> even more treacherous for programmers by returning different
> values from some calls to the function than the library would.
> This is also what bug 87096 requests.  The patch also adds
> a warning that was missing from a subset of these troublesome
> calls.
> 
> The attached patch disables the snprintf constant folding and
> range optimization for calls to it and other related bounded
> functions when the bound is not known not to exceed INT_MAX.
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> gcc-87096.diff
> 
> PR tree-optimization/87096 - Optimised snprintf is not POSIX conformant
> 
> gcc/ChangeLog:
> 
> 	PR rtl-optimization/87096
> 	* gimple-ssa-sprintf.c (sprintf_dom_walker::handle_gimple_call): Avoid
> 	folding calls whose bound may exceed INT_MAX.  Diagnose bound ranges
> 	that exceed the limit.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/87096
> 	* gcc.dg/tree-ssa/builtin-snprintf-4.c: New test.
OK
jeff

Re: [PATCH] avoid folding snprintf calls with bounds > INT_MAX (PR 87096)

[Christophe Lyon]

Hi,

On Thu, 13 Dec 2018 at 19:14, Jeff Law <law@redhat.com> wrote:
>
> On 12/12/18 4:18 PM, Martin Sebor wrote:
> > POSIX requires snprintf to fail with EOVERFLOW when the specified
> > bound exceeds INT_MAX.  This requirement conflicts with the C
> > requirements on valid calls to the function and isn't universally
> > implemented (e.g., Glibc doesn't seem to follow it, and GCC has
> > historically not paid heed to it either).  Nevertheless, there
> > are implementations that do respect it (Solaris being one of
> > them), and it seems that GCC should make a tricky situation
> > even more treacherous for programmers by returning different
> > values from some calls to the function than the library would.
> > This is also what bug 87096 requests.  The patch also adds
> > a warning that was missing from a subset of these troublesome
> > calls.
> >
> > The attached patch disables the snprintf constant folding and
> > range optimization for calls to it and other related bounded
> > functions when the bound is not known not to exceed INT_MAX.
> >
> > Tested on x86_64-linux.
> >
> > Martin
> >
> > gcc-87096.diff
> >
> > PR tree-optimization/87096 - Optimised snprintf is not POSIX conformant
> >
> > gcc/ChangeLog:
> >
> >       PR rtl-optimization/87096
> >       * gimple-ssa-sprintf.c (sprintf_dom_walker::handle_gimple_call): Avoid
> >       folding calls whose bound may exceed INT_MAX.  Diagnose bound ranges
> >       that exceed the limit.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR tree-optimization/87096
> >       * gcc.dg/tree-ssa/builtin-snprintf-4.c: New test.

This new test fails on arm (and other 32 bits targets according to
gcc-testresults)
FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 106)
FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 128)
FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 74)
FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 87)
FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c scan-tree-dump-times
optimized " = snprintf" 12
FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c scan-tree-dump-times
optimized " = vsnprintf" 12

Christophe

> OK
> jeff

Re: [PATCH] avoid folding snprintf calls with bounds > INT_MAX (PR 87096)

[Martin Sebor]

On 12/17/18 2:23 AM, Christophe Lyon wrote:
> Hi,
> 
> On Thu, 13 Dec 2018 at 19:14, Jeff Law <law@redhat.com> wrote:
>>
>> On 12/12/18 4:18 PM, Martin Sebor wrote:
>>> POSIX requires snprintf to fail with EOVERFLOW when the specified
>>> bound exceeds INT_MAX.  This requirement conflicts with the C
>>> requirements on valid calls to the function and isn't universally
>>> implemented (e.g., Glibc doesn't seem to follow it, and GCC has
>>> historically not paid heed to it either).  Nevertheless, there
>>> are implementations that do respect it (Solaris being one of
>>> them), and it seems that GCC should make a tricky situation
>>> even more treacherous for programmers by returning different
>>> values from some calls to the function than the library would.
>>> This is also what bug 87096 requests.  The patch also adds
>>> a warning that was missing from a subset of these troublesome
>>> calls.
>>>
>>> The attached patch disables the snprintf constant folding and
>>> range optimization for calls to it and other related bounded
>>> functions when the bound is not known not to exceed INT_MAX.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin
>>>
>>> gcc-87096.diff
>>>
>>> PR tree-optimization/87096 - Optimised snprintf is not POSIX conformant
>>>
>>> gcc/ChangeLog:
>>>
>>>        PR rtl-optimization/87096
>>>        * gimple-ssa-sprintf.c (sprintf_dom_walker::handle_gimple_call): Avoid
>>>        folding calls whose bound may exceed INT_MAX.  Diagnose bound ranges
>>>        that exceed the limit.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>        PR tree-optimization/87096
>>>        * gcc.dg/tree-ssa/builtin-snprintf-4.c: New test.
> 
> This new test fails on arm (and other 32 bits targets according to
> gcc-testresults)
> FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 106)
> FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 128)
> FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 74)
> FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c  (test for warnings, line 87)
> FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c scan-tree-dump-times
> optimized " = snprintf" 12
> FAIL:    gcc.dg/tree-ssa/builtin-snprintf-4.c scan-tree-dump-times
> optimized " = vsnprintf" 12

The test assumed that PTRDIFF_MAX > INT_MAX.  I adjusted it in
r267206 to avoid that assumption.

Thanks
Martin