diff mbox

detect null sprintf pointers (PR 78519)

Message ID 38cc207d-a4bf-e645-0562-c13a23fee025@gmail.com
State New
Headers show

Commit Message

Martin Sebor Dec. 15, 2016, 4:21 a.m. UTC
>> I suppose setting a range seemed better than giving up.  Then again,
>> since with this patch GCC will warn on null %s pointers there may
>> not be much point in trying to see if there's also some other
>> problem after that, except perhaps in code that deliberately relies
>> on the Glibc feature.  I'd be fine with just stopping at this point
>> if you prefer.
> I think I'd rather just stop at this point.  I don't think we're gaining
> much, if anything and encoding the glibc-ism in here seems like a mistake.

That's fine.  Attached is an updated patch with this change.

Thanks
Martin

Comments

Jeff Law Dec. 15, 2016, 4:22 a.m. UTC | #1
On 12/14/2016 09:21 PM, Martin Sebor wrote:
>>> I suppose setting a range seemed better than giving up.  Then again,
>>> since with this patch GCC will warn on null %s pointers there may
>>> not be much point in trying to see if there's also some other
>>> problem after that, except perhaps in code that deliberately relies
>>> on the Glibc feature.  I'd be fine with just stopping at this point
>>> if you prefer.
>> I think I'd rather just stop at this point.  I don't think we're gaining
>> much, if anything and encoding the glibc-ism in here seems like a
>> mistake.
>
> That's fine.  Attached is an updated patch with this change.
>
> Thanks
> Martin
>
>
> gcc-78519.diff
>
>
> PR middle-end/78519 - missing warning for sprintf %s with null pointer
>
> gcc/ChangeLog:
>
> 	PR middle-end/78519
> 	* gimple-ssa-sprintf.c (format_string): Handle null pointers.
> 	(format_directive): Diagnose null pointer arguments.
> 	(pass_sprintf_length::handle_gimple_call): Diagnose null destination
> 	pointers.  Correct location of null format string in diagnostics.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/78519
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: New test.
OK.
jeff
Jeff Law Dec. 19, 2016, 8:24 p.m. UTC | #2
On 12/14/2016 09:21 PM, Martin Sebor wrote:
>>> I suppose setting a range seemed better than giving up.  Then again,
>>> since with this patch GCC will warn on null %s pointers there may
>>> not be much point in trying to see if there's also some other
>>> problem after that, except perhaps in code that deliberately relies
>>> on the Glibc feature.  I'd be fine with just stopping at this point
>>> if you prefer.
>> I think I'd rather just stop at this point.  I don't think we're gaining
>> much, if anything and encoding the glibc-ism in here seems like a
>> mistake.
>
> That's fine.  Attached is an updated patch with this change.
>
> Thanks
> Martin
>
>
> gcc-78519.diff
>
>
> PR middle-end/78519 - missing warning for sprintf %s with null pointer
>
> gcc/ChangeLog:
>
> 	PR middle-end/78519
> 	* gimple-ssa-sprintf.c (format_string): Handle null pointers.
> 	(format_directive): Diagnose null pointer arguments.
> 	(pass_sprintf_length::handle_gimple_call): Diagnose null destination
> 	pointers.  Correct location of null format string in diagnostics.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/78519
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: New test.
So I think we should defer this given the vigorous discussion around the 
other NULL checks.  This has the same issues that we're discussing in 
the other, rather heated, thread.



Jeff
Jakub Jelinek Dec. 19, 2016, 8:37 p.m. UTC | #3
On Mon, Dec 19, 2016 at 01:24:34PM -0700, Jeff Law wrote:
> > PR middle-end/78519 - missing warning for sprintf %s with null pointer
> > 
> > gcc/ChangeLog:
> > 
> > 	PR middle-end/78519
> > 	* gimple-ssa-sprintf.c (format_string): Handle null pointers.
> > 	(format_directive): Diagnose null pointer arguments.
> > 	(pass_sprintf_length::handle_gimple_call): Diagnose null destination
> > 	pointers.  Correct location of null format string in diagnostics.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR middle-end/78519
> > 	* gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: New test.
> So I think we should defer this given the vigorous discussion around the
> other NULL checks.  This has the same issues that we're discussing in the
> other, rather heated, thread.

If this would be only warned if !fold_return_value, then it wouldn't be
having the same issues.  Though of course, it would diagnose fewer cases.
Conceptually it isn't much dependent on the gimple-ssa-sprintf.c stuff,
so could be warned without too much effort from other pass, perhaps
by using a few helpers from gimple-ssa-sprintf.c.

	Jakub
diff mbox

Patch

PR middle-end/78519 - missing warning for sprintf %s with null pointer

gcc/ChangeLog:

	PR middle-end/78519
	* gimple-ssa-sprintf.c (format_string): Handle null pointers.
	(format_directive): Diagnose null pointer arguments.
	(pass_sprintf_length::handle_gimple_call): Diagnose null destination
	pointers.  Correct location of null format string in diagnostics.

gcc/testsuite/ChangeLog:

	PR middle-end/78519
	* gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: New test.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 5909e05..435c162 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -439,7 +439,7 @@  struct result_range
 struct fmtresult
 {
   fmtresult ()
-  : argmin (), argmax (), knownrange (), bounded (), constant ()
+  : argmin (), argmax (), knownrange (), bounded (), constant (), nullp ()
   {
     range.min = range.max = HOST_WIDE_INT_MAX;
   }
@@ -467,6 +467,9 @@  struct fmtresult
      are also constant (such as determined by constant propagation,
      though not value range propagation).  */
   bool constant;
+
+  /* True when the argument is a null pointer.  */
+  bool nullp;
 };
 
 /* Description of a conversion specification.  */
@@ -1765,6 +1768,16 @@  format_string (const conversion_spec &spec, tree arg)
 	      res.range.min = 0;
 	    }
 	}
+      else if (arg && integer_zerop (arg))
+	{
+	  /* Handle null pointer argument.  */
+
+	  fmtresult res;
+	  res.range.min = 0;
+	  res.range.max = HOST_WIDE_INT_MAX;
+	  res.nullp = true;
+	  return res;
+	}
       else
 	{
 	  /* For a '%s' and '%ls' directive with a non-constant string,
@@ -1910,13 +1923,27 @@  format_directive (const pass_sprintf_length::call_info &info,
 	}
     }
 
+  if (fmtres.nullp)
+    {
+      fmtwarn (dirloc, pargrange, NULL,
+	       OPT_Wformat_length_,
+	       "%<%.*s%> directive argument is null",
+	       (int)cvtlen, cvtbeg);
+
+      /* Don't bother processing the rest of the format string.  */
+      res->warned = true;
+      res->number_chars = HOST_WIDE_INT_M1U;
+      res->number_chars_min = res->number_chars_max = res->number_chars;
+      return;
+    }
+
+  bool warned = res->warned;
+
   /* Compute the number of available bytes in the destination.  There
      must always be at least one byte of space for the terminating
      NUL that's appended after the format string has been processed.  */
   unsigned HOST_WIDE_INT navail = min_bytes_remaining (info.objsize, *res);
 
-  bool warned = res->warned;
-
   if (fmtres.range.min < fmtres.range.max)
     {
       /* The result is a range (i.e., it's inexact).  */
@@ -2871,6 +2898,9 @@  pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
       return;
     }
 
+  /* The first argument is a pointer to the destination. */
+  tree dstptr = gimple_call_arg (info.callstmt, 0);
+
   info.format = gimple_call_arg (info.callstmt, idx_format);
 
   if (idx_dstsize == HOST_WIDE_INT_M1U)
@@ -2878,7 +2908,7 @@  pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
       /* For non-bounded functions like sprintf, determine the size
 	 of the destination from the object or pointer passed to it
 	 as the first argument.  */
-      dstsize = get_destination_size (gimple_call_arg (info.callstmt, 0));
+      dstsize = get_destination_size (dstptr);
     }
   else if (tree size = gimple_call_arg (info.callstmt, idx_dstsize))
     {
@@ -2948,6 +2978,20 @@  pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
     }
   else
     {
+      /* For calls to non-bounded functions or to those of bounded
+	 functions with a non-zero size, warn if the destination
+	 pointer is null.  */
+      if (integer_zerop (dstptr))
+	{
+	  /* This is diagnosed with -Wformat only when the null is a constant
+	     pointer.  The warning here diagnoses instances where the pointer
+	     is not constant.  */
+	  location_t loc = gimple_location (info.callstmt);
+	  warning_at (EXPR_LOC_OR_LOC (dstptr, loc),
+		      OPT_Wformat_length_, "null destination pointer");
+	  return;
+	}
+
       /* Set the object size to the smaller of the two arguments
 	 of both have been specified and they're not equal.  */
       info.objsize = dstsize < objsize ? dstsize : objsize;
@@ -2971,7 +3015,8 @@  pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
       /* This is diagnosed with -Wformat only when the null is a constant
 	 pointer.  The warning here diagnoses instances where the pointer
 	 is not constant.  */
-      warning_at (EXPR_LOC_OR_LOC (info.format, input_location),
+      location_t loc = gimple_location (info.callstmt);
+      warning_at (EXPR_LOC_OR_LOC (info.format, loc),
 		  OPT_Wformat_length_, "null format string");
       return;
     }
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-8.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-8.c
new file mode 100644
index 0000000..2550065
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-8.c
@@ -0,0 +1,93 @@ 
+/* PR middle-end/78519 - missing warning for sprintf %s with null pointer
+   Also exercises null destination pointer and null format string.
+   { dg-do compile }
+   { dg-options "-O2 -Wformat -Wformat-length -Wno-nonnull -ftrack-macro-expansion=0" } */
+
+typedef __builtin_va_list va_list;
+
+#define sprintf   __builtin_sprintf
+#define snprintf  __builtin_snprintf
+#define vsprintf  __builtin_vsprintf
+#define vsnprintf __builtin_vsnprintf
+
+
+static char* null (void)
+{
+  return 0;
+}
+
+
+void sink (int);
+#define T sink
+
+
+/* Verify that calls with a null destination pointer are diagnosed.  */
+
+void test_null_dest (va_list va)
+{
+  char *p = null ();
+  T (sprintf (null (), "%i", 0));   /* { dg-warning "null destination pointer" } */
+  T (sprintf (p, "%i", 0));         /* { dg-warning "null destination pointer" } */
+  T (sprintf (p, "%i abc", 0));     /* { dg-warning "null destination pointer" } */
+
+  T (snprintf (null (), 1, "%i", 0));   /* { dg-warning "null destination pointer" } */
+  T (snprintf (p, 2, "%i", 0));         /* { dg-warning "null destination pointer" } */
+  T (snprintf (p, 3, "%i abc", 0));     /* { dg-warning "null destination pointer" } */
+
+  /* Snprintf with a null pointer and a zero size is a special request
+     to determine the size of output without writing any.  Such calls
+     are valid must not be diagnosed.  */
+  T (snprintf (p, 0, "%i", 0));
+
+  T (vsprintf (null (), "%i", va)); /* { dg-warning "null destination pointer" } */
+  T (vsprintf (p,       "%i", va)); /* { dg-warning "null destination pointer" } */
+
+  T (vsnprintf (null (), 1, "%i", va)); /* { dg-warning "null destination pointer" } */
+  T (vsnprintf (p,       2, "%i", va));       /* { dg-warning "null destination pointer" } */
+  T (vsnprintf (p,       0, "%i", va));
+}
+
+void test_null_format (char *d, va_list va)
+{
+  char *fmt = null ();
+
+  T (sprintf (d, null ()));    /* { dg-warning "null format string" } */
+  T (sprintf (d, fmt));        /* { dg-warning "null format string" } */
+
+  T (snprintf (d, 0, null ()));    /* { dg-warning "null format string" } */
+  T (snprintf (d, 1, fmt));        /* { dg-warning "null format string" } */
+
+  T (vsprintf (d, null (), va));   /* { dg-warning "null format string" } */
+  T (vsprintf (d, fmt, va));       /* { dg-warning "null format string" } */
+
+  T (vsnprintf (d, 0, null (), va));  /* { dg-warning "null format string" } */
+  T (vsnprintf (d, 1, fmt, va));      /* { dg-warning "null format string" } */
+}
+
+void test_null_arg (char *d, const char *s)
+{
+  char *p = null ();
+
+  T (sprintf (d, "%-s", null ()));  /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%-s", p));        /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%s %s", p, s));   /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%s %s", s, p));   /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%s %i", p, 1));   /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%i %s", 1, p));   /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%.0s", p));       /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%1.0s", p));      /* { dg-warning "directive argument is null" } */
+
+  T (snprintf (d, 0, "%-s", null ()));  /* { dg-warning "directive argument is null" } */
+  T (snprintf (d, 1, "%-s", p));        /* { dg-warning "directive argument is null" } */
+
+  T (sprintf (d, "%i %s", 1, null ()));  /* { dg-warning "directive argument is null" } */
+  T (sprintf (d, "%i %s", 2, p));        /* { dg-warning "directive argument is null" } */
+
+  T (snprintf (d, 0, "%i %s", 1, null ()));  /* { dg-warning "directive argument is null" } */
+  T (snprintf (d, 9, "%i %s", 2, p));        /* { dg-warning "directive argument is null" } */
+
+  /* A sanity check that the %p directive doesn't emit a warning
+     with a null pointer.  */
+  T (sprintf (d, "%p", null ()));
+  T (sprintf (d, "%p", p));
+}