[v2] Fix bogus strncpy source length warning on source bound by constant

Message ID 20180313182258.15201-1-siddhesh@sourceware.org
State New
Headers show
Series
  • [v2] Fix bogus strncpy source length warning on source bound by constant
Related show

Commit Message

Siddhesh Poyarekar March 13, 2018, 6:22 p.m.
Avoid issuing a bogus warning when the source of strncpy is bound by a
constant known to be less than the minimum size of the destination.

Changes from v1:

- Use range-info instead of the MIN_EXPR hack
- Get the minimum size of dst and check for NULL_TREE return

The patch bootstraps successfully and introduces no new regressions in
the testsuite.

gcc/

	* tree-ssa-strlen.c (handle_builtin_stxncpy): Check bounds of
	source length if available.

gcc/testsuite/

	* gcc.dg/builtin-stringop-chk-10.c: New test case.
---
 gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c | 17 +++++++++++++++++
 gcc/tree-ssa-strlen.c                          | 15 +++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c

-- 
2.14.3

Comments

Richard Biener March 14, 2018, 3:10 p.m. | #1
On Tue, Mar 13, 2018 at 7:22 PM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> Avoid issuing a bogus warning when the source of strncpy is bound by a

> constant known to be less than the minimum size of the destination.

>

> Changes from v1:

>

> - Use range-info instead of the MIN_EXPR hack

> - Get the minimum size of dst and check for NULL_TREE return

>

> The patch bootstraps successfully and introduces no new regressions in

> the testsuite.

>

> gcc/

>

>         * tree-ssa-strlen.c (handle_builtin_stxncpy): Check bounds of

>         source length if available.

>

> gcc/testsuite/

>

>         * gcc.dg/builtin-stringop-chk-10.c: New test case.

> ---

>  gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c | 17 +++++++++++++++++

>  gcc/tree-ssa-strlen.c                          | 15 +++++++++++++++

>  2 files changed, 32 insertions(+)

>  create mode 100644 gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c

>

> diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c

> new file mode 100644

> index 00000000000..13e4bd2f049

> --- /dev/null

> +++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c

> @@ -0,0 +1,17 @@

> +/* Bogus -Wstringop-overflow on strncpy when size is based on strlen but is

> +   bound by a constant.

> +   { dg-do compile }

> +   { dg-options "-O2 -Wstringop-overflow" } */

> +

> +char dst[1024];

> +

> +void

> +f1 (const char *src)

> +{

> +  unsigned long limit = 512;

> +  unsigned long len = __builtin_strlen (src);  /* { dg-bogus "length computed here" } */

> +  if (len > limit)

> +    len = limit;

> +

> +  __builtin_strncpy (dst, src, len);   /* { dg-bogus "specified bound depends on the length of the source argument" } */

> +}

> diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c

> index 72f6a17cd32..265f351ea85 100644

> --- a/gcc/tree-ssa-strlen.c

> +++ b/gcc/tree-ssa-strlen.c

> @@ -2125,6 +2125,21 @@ handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi)

>        return;

>      }

>

> +  /* Don't bother about the strlen (SRC) in the LEN computation if the range of

> +     values for LEN ends up within dstsize.  */

> +  if (TREE_CODE (len) == SSA_NAME)

> +    {

> +      wide_int min, max;

> +      enum value_range_type vr = get_range_info (len, &min, &max);

> +      tree dstsize = compute_objsize (dst, 3);

> +      if (vr == VR_RANGE && dstsize)

> +       {

> +         tree len_max = wide_int_to_tree (TREE_TYPE (dstsize), max);

> +         if (tree_int_cst_lt (len_max, dstsize))


Instead of building a tree from max you should use

    if (wi::to_widest (max) < wi::to_widest (wi::to_wide (dstsize)))
      return;

given compute_objsize is somewhat confused about the type it returns
a widest_int compare is required.

Note I'm not too familiar with tree-ssa-strlen.c nor this part of the
warning code
so I'll not approve the patch but after fixing that it looks techincally ok.

Richard.

> +           return;

> +       }

> +    }

> +

>    /* Retrieve the strinfo data for the string S that LEN was computed

>       from as some function F of strlen (S) (i.e., LEN need not be equal

>       to strlen(S)).  */

> --

> 2.14.3

>
Siddhesh Poyarekar March 15, 2018, 6:53 a.m. | #2
On Wednesday 14 March 2018 08:40 PM, Richard Biener wrote:
> Instead of building a tree from max you should use

> 

>     if (wi::to_widest (max) < wi::to_widest (wi::to_wide (dstsize)))

>       return;

> 

> given compute_objsize is somewhat confused about the type it returns

> a widest_int compare is required.

> 

> Note I'm not too familiar with tree-ssa-strlen.c nor this part of the

> warning code

> so I'll not approve the patch but after fixing that it looks techincally ok.


Thanks, I'll post a fixed up version soon.

Siddhesh

Patch

diff --git a/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c
new file mode 100644
index 00000000000..13e4bd2f049
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtin-stringop-chk-10.c
@@ -0,0 +1,17 @@ 
+/* Bogus -Wstringop-overflow on strncpy when size is based on strlen but is
+   bound by a constant.
+   { dg-do compile }
+   { dg-options "-O2 -Wstringop-overflow" } */
+
+char dst[1024];
+
+void
+f1 (const char *src)
+{
+  unsigned long limit = 512;
+  unsigned long len = __builtin_strlen (src);	/* { dg-bogus "length computed here" } */
+  if (len > limit)
+    len = limit;
+
+  __builtin_strncpy (dst, src, len);	/* { dg-bogus "specified bound depends on the length of the source argument" } */
+}
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index 72f6a17cd32..265f351ea85 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -2125,6 +2125,21 @@  handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi)
       return;
     }
 
+  /* Don't bother about the strlen (SRC) in the LEN computation if the range of
+     values for LEN ends up within dstsize.  */
+  if (TREE_CODE (len) == SSA_NAME)
+    {
+      wide_int min, max;
+      enum value_range_type vr = get_range_info (len, &min, &max);
+      tree dstsize = compute_objsize (dst, 3);
+      if (vr == VR_RANGE && dstsize)
+	{
+	  tree len_max = wide_int_to_tree (TREE_TYPE (dstsize), max);
+	  if (tree_int_cst_lt (len_max, dstsize))
+	    return;
+	}
+    }
+
   /* Retrieve the strinfo data for the string S that LEN was computed
      from as some function F of strlen (S) (i.e., LEN need not be equal
      to strlen(S)).  */