Re: [PATCH] Fix bogus strncpy source length warning on source bound by constant

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, Mar 11, 2018 at 4:22 PM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> Avoid issuing a bogus warning when the source of strncpy is bound by a
> constant and is known to be less than the size of the destination.
>
> Testsuite run is underway (not complete yet, but no new errors so far)
> and a bootstrap is also underway, I'll report status once they're both
> done.
>
> gcc/
>
>         * 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                          | 24 ++++++++++++++++++++++++
>  2 files changed, 41 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..49a31a551f5 100644
> --- a/gcc/tree-ssa-strlen.c
> +++ b/gcc/tree-ssa-strlen.c
> @@ -2125,6 +2125,30 @@ handle_builtin_stxncpy (built_in_function, gimple_stmt_iterator *gsi)
>        return;
>      }
>
> +  /* When LEN is MIN_EXPR of strlen and a constant, then the copy is bound by
> +     that constant.  If the destination size is also constant then compare with
> +     it to avoid a bogus warning.  */
> +  if (TREE_CODE (len) == SSA_NAME)
> +    {
> +      gimple *def_stmt = SSA_NAME_DEF_STMT (len);
> +
> +      if (is_gimple_assign (def_stmt)
> +         && gimple_assign_rhs_code (def_stmt) == MIN_EXPR)
> +       {
> +         /* RHS1 is the strlen, so check if RHS2 and DSTSIZE are constant.  */
> +         tree rhs2 = gimple_assign_rhs2 (def_stmt);
> +         tree dstsize = compute_objsize (dst, 1);
> +
> +         if (TREE_CODE (rhs2) == INTEGER_CST
> +             && TREE_CODE (dstsize) == INTEGER_CST
> +             && int_cst_value (rhs2) < int_cst_value (dstsize))

Please use tree_int_cst_lt (rhs1, dstsize) instead.

> +           {
> +             gimple_set_no_warning (stmt, true);

Why this?  There's only a single bit -- where do we warn from if you
don't do this here?

I also wonder why this code doesn't use range-info given the
INTEGER_CST constraints
range-info should tell us more than just handling MIN_EXPR?

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
>