This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]