[PATCH] detect null sprintf pointers (PR 78519)

[Jeff Law]

On 12/04/2016 04:55 PM, Martin Sebor wrote:
> Bug 78519 points out that while the -Wformat warning flags a small
> subset of sprintf calls with a null pointer argument to a %s directive
> (those where the pointer is a constant) it misses the much bigger
> set where the pointer is not a constant but instead is determined
> to be null as a result of optimization.  This is because -Wformat
> runs too early, before any of the optimization passes that make it
> possible to detect that non-constant pointers are null.
>
> With the -Wformat-length warning running much later than -Wformat,
> it's trivial to detect and diagnose these types of bugs with it.
> The attached patch adds this warning, along with the ability to
> detect a null destination pointer when it's required to be non-null
> (this is in all of the {v,}sprintf functions and in {v,}snprintf
> when the size argument is not zero).
>
> Ultimately, the destination pointer argument (but not the format
> string) to the {v,}sprintf functions needs to be declared nonnull
> (pursuant to bug 78673) and the null-checking moved elsewhere.
> I'm testing a follow-on patch that does just that but I post this
> fix in the meantime since its main focus is the null %s argument.
>
> 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.
>
> diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
> index e86c4dc..7004f09 100644
> --- a/gcc/gimple-ssa-sprintf.c
> +++ b/gcc/gimple-ssa-sprintf.c
> @@ -433,7 +433,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;
>    }
> @@ -461,6 +461,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.  */
> @@ -1624,6 +1627,20 @@ format_string (const conversion_spec &spec, tree arg)
>  	      res.range.min = 0;
>  	    }
>  	}
> +      else if (arg && integer_zerop (arg))
> +	{
> +	  /* Handle null pointer argument.  */
> +
> +	  fmtresult res;
> +	  /* Set the range based on Glibc "(null)" output but leave
> +	     all other members at default to indicate that the range
> +	     isn't trustworthy.  This allows the rest of the format
> +	     string to be checked for problems.  */
By not trustworthy, I guess you mean it's only used to issue "may be" 
style warnings, right?  What benefit do you gain by encoding the 
glib-ism vs using HOST_WIDE_INT_MAX?  Presumably once you use 
HOST_WIDE_INT_MAX nothing else is going to be checked?

Jeff