diff mbox

enable -Wformat-length for dynamically allocated buffers (pr 78245)

Message ID 3ce498e7-3c3f-600d-2f2b-cfacea646140@gmail.com
State New
Headers show

Commit Message

Martin Sebor Nov. 30, 2016, 3:22 a.m. UTC 
>>> That said, I defer to you on how to proceed here.  I'm prepared
>>> to do the work(*) but I do worry about jeopardizing the chances
>>> of this patch and the others making it into 7.0.
>> So would it make sense to just init/fini the b_o_s framework in your
>> pass and for builtin expansion?
>
> I think that should work for the sprintf checking.  Let me test it.
> We can deal with the memxxx and strxxx patch (53562) independently
> if you prefer.

Attached is a modified patch that calls {init,fini}_object_sizes()
from the gimple-ssa-sprintf pass instead.

While this works fine, I do like the approach of making the calls
in a single function better because it makes for a more robust API.
Decoupling the init/fini calls from the compute_object_size()
function that depends on them having been made makes the API easier
to accidentally misuse by calling one while forgetting to call one
or both of the other two.

Martin

Comments

Jeff Law Dec. 2, 2016, 9:02 p.m. UTC | #1 
On 11/29/2016 08:22 PM, Martin Sebor wrote:
>>>> That said, I defer to you on how to proceed here.  I'm prepared
>>>> to do the work(*) but I do worry about jeopardizing the chances
>>>> of this patch and the others making it into 7.0.
>>> So would it make sense to just init/fini the b_o_s framework in your
>>> pass and for builtin expansion?
>>
>> I think that should work for the sprintf checking.  Let me test it.
>> We can deal with the memxxx and strxxx patch (53562) independently
>> if you prefer.
>
> Attached is a modified patch that calls {init,fini}_object_sizes()
> from the gimple-ssa-sprintf pass instead.
>
> While this works fine, I do like the approach of making the calls
> in a single function better because it makes for a more robust API.
> Decoupling the init/fini calls from the compute_object_size()
> function that depends on them having been made makes the API easier
> to accidentally misuse by calling one while forgetting to call one
> or both of the other two.
It's not ideal, nor is the prospect of caching and potentially not 
invaliding properly.

I've started tackling these kinds problems by wrapping everything into a 
class with suitable ctors/dtors and methods.  With everything locked 
down inside a class, the only way to access the subsystem is by 
instantiating a suitable object (which obviously gives us control over 
init/fini).  The problem then boils down to not having that instantiated 
object live across passes, which usually isn't a problem in GCC :-)

I suggested it as a possibility, but wasn't going to demand it without 
knowing much more about the code in tree-object-size and how well it 
could be encapsulated.

ANyway, I'll take another look at the patch.  My recollection was that 
the only issue at hand was the init/fini/caching aspects.


jeff
Jeff Law Dec. 2, 2016, 9:18 p.m. UTC | #2 
On 11/29/2016 08:22 PM, Martin Sebor wrote:
>>>> That said, I defer to you on how to proceed here.  I'm prepared
>>>> to do the work(*) but I do worry about jeopardizing the chances
>>>> of this patch and the others making it into 7.0.
>>> So would it make sense to just init/fini the b_o_s framework in your
>>> pass and for builtin expansion?
>>
>> I think that should work for the sprintf checking.  Let me test it.
>> We can deal with the memxxx and strxxx patch (53562) independently
>> if you prefer.
>
> Attached is a modified patch that calls {init,fini}_object_sizes()
> from the gimple-ssa-sprintf pass instead.
>
> While this works fine, I do like the approach of making the calls
> in a single function better because it makes for a more robust API.
> Decoupling the init/fini calls from the compute_object_size()
> function that depends on them having been made makes the API easier
> to accidentally misuse by calling one while forgetting to call one
> or both of the other two.
>
> Martin
>
>
> gcc-78245.diff
>
>
> PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/78245
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.
>
> gcc/ChangeLog:
>
> 	PR middle-end/78245
> 	* gimple-ssa-sprintf.c (get_destination_size): Call
> 	compute_object_size.
> 	* tree-object-size.c (addr_object_size): Adjust.
> 	(pass_through_call): Adjust.
> 	(internal_object_size): New function.
> 	(compute_builtin_object_size): Call internal_object_size.
> 	(pass_object_sizes::execute): Adjust.
> 	* tree-object-size.h (fini_object_sizes): Declare.
>
> @@ -664,6 +665,18 @@ compute_builtin_object_size (tree ptr, int object_size_type,
>    return *psize != unknown[object_size_type];
>  }
>
> +/* Compute __builtin_object_size value for PTR and set *PSIZE to
> +   the resulting value.  OBJECT_SIZE_TYPE is the second argument
> +   to __builtin_object_size.  Return true on success and false
> +   when the object size could not be determined.  */
> +
> +bool
> +compute_builtin_object_size (tree ptr, int object_size_type,
> +			     unsigned HOST_WIDE_INT *psize)
> +{
> +  return internal_object_size (ptr, object_size_type, psize);
> +}
Is this wrapper still necessary now that we've pulled the init/fini 
routines out?  Seems like it shouldn't be.


I think that's the only issue left.  If the wrapper is needed, then the 
patch is fine.  If the wrapper isn't, then a patch with the wrapper 
removed is pre-approved after the usual testing.

jeff
diff mbox

Patch

PR middle-end/78245 - missing -Wformat-length on an overflow of a dynamically allocated buffer

gcc/testsuite/ChangeLog:

	PR middle-end/78245
	* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Add tests.

gcc/ChangeLog:

	PR middle-end/78245
	* gimple-ssa-sprintf.c (get_destination_size): Call
	compute_object_size.
	* tree-object-size.c (addr_object_size): Adjust.
	(pass_through_call): Adjust.
	(internal_object_size): New function.
	(compute_builtin_object_size): Call internal_object_size.
	(pass_object_sizes::execute): Adjust.
	* tree-object-size.h (fini_object_sizes): Declare.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index ead8b0e..34b3723 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2466,6 +2466,9 @@  get_destination_size (tree dest)
      a member array as opposed to the whole enclosing object), otherwise
      use type-zero object size to determine the size of the enclosing
      object (the function fails without optimization in this type).  */
+
+  init_object_sizes ();
+
   int ost = optimize > 0;
   unsigned HOST_WIDE_INT size;
   if (compute_builtin_object_size (dest, ost, &size))
@@ -2800,6 +2803,8 @@  pass_sprintf_length::execute (function *fun)
 	}
     }
 
+  fini_object_sizes ();
+
   return 0;
 }
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
index 8d97fa8..9874332 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
@@ -1,5 +1,10 @@ 
 /* { dg-do compile } */
-/* { dg-options "-std=c99 -O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* { dg-options "-O2 -Wformat -Wformat-length=1 -ftrack-macro-expansion=0" } */
+/* Verify that all sprintf built-ins detect overflow involving directives
+   with non-constant arguments known to be constrained by some range of
+   values, and even when writing into dynamically allocated buffers.
+   -O2 (-ftree-vrp) is necessary for the tests involving ranges to pass,
+   otherwise -O1 is sufficient.  */
 
 #ifndef LINE
 #  define LINE 0
@@ -7,18 +12,26 @@ 
 
 #define bos(x) __builtin_object_size (x, 0)
 
-#define T(bufsize, fmt, ...)						\
-    do {								\
-      if (!LINE || __LINE__ == LINE)					\
-	{								\
-	  char *d = (char *)__builtin_malloc (bufsize);			\
-	  __builtin___sprintf_chk (d, 0, bos (d), fmt, __VA_ARGS__);	\
-	  sink (d);							\
-	}								\
-    } while (0)
+/* Defined (and redefined) to the allocation function to use, either
+   malloc, or alloca, or a VLA.  */
+#define ALLOC(p, n)   (p) = __builtin_malloc (n)
 
-void
-sink (void*);
+/* Defined (and redefined) to the sprintf function to exercise.  */
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)		\
+  __builtin___sprintf_chk (d, 0, objsize, fmt, __VA_ARGS__)
+
+#define T(bufsize, fmt, ...)				\
+  do {							\
+    if (!LINE || __LINE__ == LINE)			\
+      {							\
+	char *d;					\
+	ALLOC (d, bufsize);				\
+	TEST_SPRINTF (d, 0, bos (d), fmt, __VA_ARGS__);	\
+	sink (d);					\
+      }							\
+  } while (0)
+
+void sink (void*);
 
 /* Identity function to verify that the checker figures out the value
    of the operand even when it's not constant (i.e., makes use of
@@ -232,3 +245,88 @@  void test_sprintf_chk_range_sshort (signed short *a, signed short *b)
   T ( 4, "%i",  Ra (998,  999));
   T ( 4, "%i",  Ra (999, 1000)); /* { dg-warning "may write a terminating nul past the end of the destination" } */
 }
+
+/* Exercise ordinary sprintf with malloc.  */
+#undef TEST_SPRINTF
+#define TEST_SPRINTF(d, maxsize, objsize, fmt, ...)	\
+  __builtin_sprintf (d, fmt, __VA_ARGS__)
+
+void test_sprintf_malloc (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with alloca.  */
+#undef ALLOC
+#define ALLOC(p, n) (p) = __builtin_alloca (n)
+
+void test_sprintf_alloca (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
+
+/* Exercise ordinary sprintf with a VLA.  */
+#undef ALLOC
+#define ALLOC(p, n) char vla [i (n)]; (p) = vla
+
+void test_sprintf_vla (const char *s, const char *t)
+{
+#define x x ()
+
+  T (1, "%-s", x ? "" : "1");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : "");       /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : "1");        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? "1" : s);        /* { dg-warning "nul past the end" } */
+  T (1, "%-s", x ? s : t);
+
+  T (2, "%-s", x ? "" : "1");
+  T (2, "%-s", x ? "" : s);
+  T (2, "%-s", x ? "1" : "");
+  T (2, "%-s", x ? s : "");
+  T (2, "%-s", x ? "1" : "2");
+  T (2, "%-s", x ? "" : "12");      /* { dg-warning "nul past the end" } */
+  T (2, "%-s", x ? "12" : "");      /* { dg-warning "nul past the end" } */
+
+  T (2, "%-s", x ? "" : "123");     /* { dg-warning "into a region" } */
+  T (2, "%-s", x ? "123" : "");     /* { dg-warning "into a region" } */
+
+#undef x
+}
diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
index 1317ad7..3a2d54d 100644
--- a/gcc/tree-object-size.c
+++ b/gcc/tree-object-size.c
@@ -50,6 +50,7 @@  static const unsigned HOST_WIDE_INT unknown[4] = {
   0
 };
 
+static bool internal_object_size (tree, int, unsigned HOST_WIDE_INT *);
 static tree compute_object_offset (const_tree, const_tree);
 static bool addr_object_size (struct object_size_info *,
 			      const_tree, int, unsigned HOST_WIDE_INT *);
@@ -187,7 +188,7 @@  addr_object_size (struct object_size_info *osi, const_tree ptr,
       if (!osi || (object_size_type & 1) != 0
 	  || TREE_CODE (TREE_OPERAND (pt_var, 0)) != SSA_NAME)
 	{
-	  compute_builtin_object_size (TREE_OPERAND (pt_var, 0),
+	  internal_object_size (TREE_OPERAND (pt_var, 0),
 				       object_size_type & ~1, &sz);
 	}
       else
@@ -491,14 +492,14 @@  pass_through_call (const gcall *call)
 }
 
 
-/* Compute __builtin_object_size value for PTR and set *PSIZE to
-   the resulting value.  OBJECT_SIZE_TYPE is the second argument
-   to __builtin_object_size.  Return true on success and false
-   when the object size could not be determined.  */
+/* Compute the size of the object pointed to by PTR and set *PSIZE
+   to the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false when
+   the object size could not be determined.  */
 
 bool
-compute_builtin_object_size (tree ptr, int object_size_type,
-			     unsigned HOST_WIDE_INT *psize)
+internal_object_size (tree ptr, int object_size_type,
+		      unsigned HOST_WIDE_INT *psize)
 {
   gcc_assert (object_size_type >= 0 && object_size_type <= 3);
 
@@ -534,7 +535,7 @@  compute_builtin_object_size (tree ptr, int object_size_type,
 	      ptr = gimple_assign_rhs1 (def);
 
 	      if (cst_and_fits_in_hwi (offset)
-		  && compute_builtin_object_size (ptr, object_size_type, psize))
+		  && internal_object_size (ptr, object_size_type, psize))
 		{
 		  /* Return zero when the offset is out of bounds.  */
 		  unsigned HOST_WIDE_INT off = tree_to_shwi (offset);
@@ -664,6 +665,18 @@  compute_builtin_object_size (tree ptr, int object_size_type,
   return *psize != unknown[object_size_type];
 }
 
+/* Compute __builtin_object_size value for PTR and set *PSIZE to
+   the resulting value.  OBJECT_SIZE_TYPE is the second argument
+   to __builtin_object_size.  Return true on success and false
+   when the object size could not be determined.  */
+
+bool
+compute_builtin_object_size (tree ptr, int object_size_type,
+			     unsigned HOST_WIDE_INT *psize)
+{
+  return internal_object_size (ptr, object_size_type, psize);
+}
+
 /* Compute object_sizes for PTR, defined to VALUE, which is not an SSA_NAME.  */
 
 static void
@@ -1230,7 +1243,7 @@  init_object_sizes (void)
 
 /* Destroy data structures after the object size computation.  */
 
-static void
+void
 fini_object_sizes (void)
 {
   int object_size_type;
@@ -1325,7 +1338,7 @@  pass_object_sizes::execute (function *fun)
 		    {
 		      tree type = TREE_TYPE (lhs);
 		      unsigned HOST_WIDE_INT bytes;
-		      if (compute_builtin_object_size (ptr, object_size_type,
+		      if (internal_object_size (ptr, object_size_type,
 						       &bytes)
 			  && wi::fits_to_tree_p (bytes, type))
 			{
diff --git a/gcc/tree-object-size.h b/gcc/tree-object-size.h
index 38c3e07..b656339 100644
--- a/gcc/tree-object-size.h
+++ b/gcc/tree-object-size.h
@@ -21,6 +21,7 @@  along with GCC; see the file COPYING3.  If not see
 #define GCC_TREE_OBJECT_SIZE_H
 
 extern void init_object_sizes (void);
+extern void fini_object_sizes (void);
 extern bool compute_builtin_object_size (tree, int, unsigned HOST_WIDE_INT *);
 
 #endif  // GCC_TREE_OBJECT_SIZE_H