[PATCH] handle sprintf(d, "%s", ...) in gimple-ssa-sprintf.c

[Jeff Law]

On 04/21/2017 03:33 PM, Martin Sebor wrote:
> Bug 77671 - missing -Wformat-overflow warning on sprintf overflow
> with "%s", is caused by gimple-fold.c transforming s{,n}printf
> calls with a plain "%s" format string into strcpy regardless of
> whether they are valid well before the -Wformtat-overflow warning
> has had a chance to validate them.
> 
> The attached patch moves the transformation from gimple-fold.c
> into the gimple-ssa-sprintf.c pass, thus allowing the calls to
> be validated.  Only valid calls (i.e., those that do not overflow
> the destination) are transformed.
> 
> Martin
> 
> gcc-77671.diff
> 
> 
> commit 2cd98763984ffb93606ee96ad658733b4c95c1e4
> Author: Martin Sebor<msebor@redhat.com>
> Date:   Wed Apr 12 18:36:26 2017 -0600
> 
>      PR middle-end/77671 - missing -Wformat-overflow warning on sprintf overflow
>      with %s
>      
>      gcc/ChangeLog:
>      
>              PR middle-end/77671
>              * gimple-fold.c (gimple_fold_builtin_sprintf): Make extern.
>              (gimple_fold_builtin_snprintf): Same.
>              * gimple-fold.h (gimple_fold_builtin_sprintf): Declare.
>              (gimple_fold_builtin_snprintf): Same.
>              * gimple-ssa-sprintf.c (get_format_string): Correct the detection
>              of character types.
>              (is_call_safe): New function.
>              (try_substitute_return_value): Call it.
>              (try_simplify_call): New function.
>              (pass_sprintf_length::handle_gimple_call): Call it.
>      
>      gcc/testsuite/ChangeLog:
>      
>              PR middle-end/77671
>              * gcc.dg/tree-ssa/builtin-sprintf-7.c: New test.
>              * gcc.dg/tree-ssa/builtin-sprintf-8.c: New test.
>              * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Adjust.
>              * gcc.dg/tree-ssa/builtin-sprintf-warn-2.c: Adjust.
>              * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Adjust.
I assume this went through the usual regression testing cycle?  I'm a 
bit surprised nothing failed due to moving the transformation later in 
the pipeline.


> 
> diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
> index 2474391..07d6897 100644
> --- a/gcc/gimple-ssa-sprintf.c
> +++ b/gcc/gimple-ssa-sprintf.c
> @@ -373,7 +373,16 @@ get_format_string (tree format, location_t *ploc)
>     if (TREE_CODE (format) != STRING_CST)
>       return NULL;
>   
> -  if (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (format))) != char_type_node)
> +  tree type = TREE_TYPE (format);
> +  if (TREE_CODE (type) == ARRAY_TYPE)
> +    type = TREE_TYPE (type);
> +
> +  /* Can't just test that:
> +       TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (format))) != char_type_node
> +     See bug 79062.  */
> +  if (TREE_CODE (type) != INTEGER_TYPE
> +      || TYPE_MODE (type) != TYPE_MODE (char_type_node)
> +      || TYPE_PRECISION (type) != TYPE_PRECISION (char_type_node))
>       {
>         /* Wide format string.  */
>         return NULL;
In the referenced BZ Richi mentions how fold-const.c checks for this 
case and also hints that you might want t look at tree-ssa-strlen as 
well.  That seems wise.  It also seems wise to factor that check and use 
it from all the identified locations.  I don't like that this uses a 
different check than what's in fold-const.c.

It's also not clear if this change is a requirement to address 77671 or 
not.  If so ISTM that we fix 79062 first, then 77671.  If not we can fix 
them independently.  In both cases the fix for 79062 can be broken out 
into its own patch.



> @@ -3278,31 +3287,21 @@ get_destination_size (tree dest)
>     return HOST_WIDE_INT_M1U;
>   }
>   
> -/* Given a suitable result RES of a call to a formatted output function
> -   described by INFO, substitute the result for the return value of
> -   the call.  The result is suitable if the number of bytes it represents
> -   is known and exact.  A result that isn't suitable for substitution may
> -   have its range set to the range of return values, if that is known.
> -   Return true if the call is removed and gsi_next should not be performed
> -   in the caller.  */
> -
>   static bool
> -try_substitute_return_value (gimple_stmt_iterator *gsi,
> -			     const pass_sprintf_length::call_info &info,
> -			     const format_result &res)
> +is_call_safe (const pass_sprintf_length::call_info &info,
> +	      const format_result &res, bool under4k,
> +	      unsigned HOST_WIDE_INT retval[2])
You need a function comment for is_call_safe.



> @@ -3423,6 +3458,30 @@ try_substitute_return_value (gimple_stmt_iterator *gsi,
>     return removed;
>   }
>   
> +static bool
> +try_simplify_call (gimple_stmt_iterator *gsi,
> +		   const pass_sprintf_length::call_info &info,
> +		   const format_result &res)
Needs a function comment.



So there's the  minor nits to add function comments.  The big question 
is the stuff for 79062 and a confirmation that this went through a full 
regression test cycle.

Jeff