Re: [PATCH v2] middle-end/104854: Limit strncmp overread warnings

[Martin Sebor <msebor@redhat.com>]

On 3/14/22 23:31, Siddhesh Poyarekar wrote:
> The size argument in strncmp only describe the maximum length to which
> to compare two strings and is not an indication of sizes of the two
> source strings.  Do not warn if it is larger than the two input strings
> because it is entirely likely that the size argument is a conservative
> maximum to accommodate inputs of different lengths and only a subset is
> reachable through the current code path or that it is some other
> application-specific property completely unrelated to the sizes of the
> input strings.

The strncmp function takes arrays as arguments (not necessarily
strings).  The main purpose of the -Wstringop-overread warning
for calls to it is to detect calls where one of the arrays is
not a nul-terminated string and the bound is larger than the size
of the array.  For example:

   char a[4], b[4];

   int f (void)
   {
     return strncmp (a, b, 8);   // -Wstringop-overread
   }

Such a call is suspect: if one of the arrays isn't nul-terminated
the call is undefined.  Otherwise, if both are nul-terminated there
is no point in calling strncmp with a bound greater than their sizes.

With no evidence that this warning is ever harmful I'd consider
suppressing it a regression.  Since the warning is a deliberate
feature in a released compiler and GCC is now in a regression
fixing stage, this patch is out of scope even if a case where
the warning wasn't helpful did turn up (none has been reported
so far).

> 
> gcc/ChangeLog:
> 
> 	middle-end/104854
> 	* gimple-ssa-warn-access.cc
> 	(pass_waccess::warn_zero_sized_strncmp_inputs): New function.
> 	(pass_waccess::check_strncmp): Use it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	middle-end/104854
> 	* gcc.dg/Wstringop-overread.c (test_strncmp_array): Don't expect
> 	failures for non-zero sizes.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
> ---
> 
> Changes from v1:
> 
> A little better approach, ensuring that it tries to warn on zero length
> inputs if the size of at least one of the two sources is known.
> 
> Also cc'ing Martin so that we can discuss approach on the list instead
> of on the bug.  To summarize the discussion so far, Martin suggests that
> the warning be split into levels but I'm contesting the utility of the
> heuristics as a compiler warning given the looseness of the relationship
> between the size argument and the inputs in the case of these functions.

Thanks for CC'ing me.  The motivating example in pr104854 that we have
been discussing there involves strndup with a string literal.  That's
an entirely different case than the one your patch changes, and I don't
understand in what way you think they are related.

Martin

> 
> 
>   gcc/gimple-ssa-warn-access.cc             | 69 +++++++++--------------
>   gcc/testsuite/gcc.dg/Wstringop-overread.c |  2 +-
>   2 files changed, 28 insertions(+), 43 deletions(-)
> 
> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> index 75297ed7c9e..15299770e29 100644
> --- a/gcc/gimple-ssa-warn-access.cc
> +++ b/gcc/gimple-ssa-warn-access.cc
> @@ -2137,6 +2137,9 @@ private:
>     /* Return true if use follows an invalidating statement.  */
>     bool use_after_inval_p (gimple *, gimple *, bool = false);
>   
> +  /* Emit an overread warning for zero sized inputs to strncmp.  */
> +  void warn_zero_sized_strncmp_inputs (gimple *, tree *, access_data *);
> +
>     /* A pointer_query object to store information about pointers and
>        their targets in.  */
>     pointer_query m_ptr_qry;
> @@ -2619,8 +2622,20 @@ pass_waccess::check_stxncpy (gcall *stmt)
>   		data.mode, &data, m_ptr_qry.rvals);
>   }
>   
> -/* Check a call STMT to stpncpy() or strncpy() for overflow and warn
> -   if it does.  */
> +/* Warn for strncmp on a zero sized source or when an argument isn't
> +   nul-terminated.  */
> +void
> +pass_waccess::warn_zero_sized_strncmp_inputs (gimple *stmt, tree *bndrng,
> +					      access_data *pad)
> +{
> +  tree func = get_callee_fndecl (stmt);
> +  location_t loc = gimple_location (stmt);
> +  maybe_warn_for_bound (OPT_Wstringop_overread, loc, stmt, func, bndrng,
> +			size_zero_node, pad);
> +}
> +
> +/* Check a call STMT to strncmp () for overflow and warn if it does.  This is
> +   limited to checking for NUL terminated arrays for now.  */
>   
>   void
>   pass_waccess::check_strncmp (gcall *stmt)
> @@ -2678,46 +2693,16 @@ pass_waccess::check_strncmp (gcall *stmt)
>     if (!bndrng[0] || integer_zerop (bndrng[0]))
>       return;
>   
> -  if (len1 && tree_int_cst_lt (len1, bndrng[0]))
> -    bndrng[0] = len1;
> -  if (len2 && tree_int_cst_lt (len2, bndrng[0]))
> -    bndrng[0] = len2;
> -
> -  /* compute_objsize almost never fails (and ultimately should never
> -     fail).  Don't bother to handle the rare case when it does.  */
> -  if (!compute_objsize (arg1, stmt, 1, &adata1.src, &m_ptr_qry)
> -      || !compute_objsize (arg2, stmt, 1, &adata2.src, &m_ptr_qry))
> -    return;
> -
> -  /* Compute the size of the remaining space in each array after
> -     subtracting any offset into it.  */
> -  offset_int rem1 = adata1.src.size_remaining ();
> -  offset_int rem2 = adata2.src.size_remaining ();
> -
> -  /* Cap REM1 and REM2 at the other if the other's argument is known
> -     to be an unterminated array, either because there's no space
> -     left in it after adding its offset or because it's constant and
> -     has no nul.  */
> -  if (rem1 == 0 || (rem1 < rem2 && lendata1.decl))
> -    rem2 = rem1;
> -  else if (rem2 == 0 || (rem2 < rem1 && lendata2.decl))
> -    rem1 = rem2;
> -
> -  /* Point PAD at the array to reference in the note if a warning
> -     is issued.  */
> -  access_data *pad = len1 ? &adata2 : &adata1;
> -  offset_int maxrem = wi::max (rem1, rem2, UNSIGNED);
> -  if (lendata1.decl || lendata2.decl
> -      || maxrem < wi::to_offset (bndrng[0]))
> -    {
> -      /* Warn when either argument isn't nul-terminated or the maximum
> -	 remaining space in the two arrays is less than the bound.  */
> -      tree func = get_callee_fndecl (stmt);
> -      location_t loc = gimple_location (stmt);
> -      maybe_warn_for_bound (OPT_Wstringop_overread, loc, stmt, func,
> -			    bndrng, wide_int_to_tree (sizetype, maxrem),
> -			    pad);
> -    }
> +  /* compute_objsize almost never fails (and ultimately should never fail).
> +     Don't bother to handle the rare case when it does.  Warn if either the
> +     source or destination has zero size, since the minimum bound is non-zero,
> +     hence guaranteeing an overread.  */
> +  if (compute_objsize (arg1, stmt, 1, &adata1.src, &m_ptr_qry)
> +      && adata1.src.size_remaining () == 0)
> +    warn_zero_sized_strncmp_inputs (stmt, bndrng, &adata1);
> +  if (compute_objsize (arg2, stmt, 1, &adata2.src, &m_ptr_qry)
> +      && adata2.src.size_remaining () == 0)
> +    warn_zero_sized_strncmp_inputs (stmt, bndrng, &adata2);
>   }
>   
>   /* Determine and check the sizes of the source and the destination
> diff --git a/gcc/testsuite/gcc.dg/Wstringop-overread.c b/gcc/testsuite/gcc.dg/Wstringop-overread.c
> index 7db74029819..fb8e626439d 100644
> --- a/gcc/testsuite/gcc.dg/Wstringop-overread.c
> +++ b/gcc/testsuite/gcc.dg/Wstringop-overread.c
> @@ -431,7 +431,7 @@ void test_strncmp_array (const char *s, int i)
>   
>     T (strncmp (a1, b1, 0));
>     T (strncmp (a1, b1, 1));
> -  T (strncmp (a1, b1, 2));      // { dg-warning "'strncmp' specified bound 2 exceeds source size 1" }
> +  T (strncmp (a1, b1, 2));
>   }
>   
>