Re: [PING] [PATCH] enhance buffer overflow warnings (and c/53562)

[Jeff Law <law at redhat dot com>]

On 11/09/2016 03:49 PM, Martin Sebor wrote:

> gcc-53562.diff
>
>
> PR c/53562 - Add -Werror= support for -D_FORTIFY_SOURCE / __builtin___memcpy_chk
> PR middle-end/77784 - duplicate warning for snprintf when n > object size
> PR middle-end/78149 - missing warning on strncpy buffer overflow due to an excessive bound
> PR middle-end/78138 - missing warnings on buffer overflow with non-constant source length
>
> gcc/c-family/ChangeLog:
> 	PR c/53562
> 	* c.opt (-Wstringop-overflow): New option.
>
> gcc/ChangeLog:
> 	PR c/53562
> 	PR middle-end/77784
> 	PR middle-end/78149
> 	PR middle-end/78138
> 	
> 	* builtins.c (expand_builtin_strcat, expand_builtin_strncat): New
> 	functions.
> 	(compute_dest_size, get_size_range, check_sizes, check_strncat_sizes)
> 	(check_memop_sizes): Same.
> 	(expand_builtin_memcpy): Call check memop_sizes.
> 	(expand_builtin_mempcpy): Same.
> 	(expand_builtin_memset): Same,
> 	(expand_builtin_bzero): Same.
> 	(expand_builtin_memory_chk): Call check_sizes.
> 	(expand_builtin_strcpy): Same.
> 	(expand_builtin_strncpy): Same.
> 	(maybe_emit_sprintf_chk_warning): Same.
> 	(expand_builtin): Handle strcat and strncat.
> 	(fini_object_sizes): Reset pointers.
> 	(compute_object_size): New function.
> 	* gimple-ssa-sprintf.c (pass_sprintf_length::handle_gimple_call):
> 	Avoid issuing warnings also issued during built-in expansion.
> 	* doc/invoke.texi (Warning Options): Document -Wstringop-overflow.
>
> gcc/testsuite/ChangeLog:
> 	PR c/53562
> 	PR middle-end/77784
> 	PR middle-end/78149
> 	PR middle-end/78138
>
> 	* c-c++-common/Wsizeof-pointer-memaccess2.c: Adjust expected diagnostic.
> 	* g++.dg/ext/builtin-object-size3.C (bar): Same.
> 	* g++.dg/ext/strncpy-chk1.C: Same.
> 	* g++.dg/opt/memcpy1.C: Same.
> 	* g++.dg/torture/Wsizeof-pointer-memaccess1.C: Same.
> 	* gcc.c-torture/compile/pr55569.c: Disable -Wstringop-overflow.
> 	* gcc.dg/Wobjsize-1.c: Adjust expected diagnostic.
> 	* gcc.dg/attr-alloc_size.c: Same.
> 	* gcc.dg/builtin-stringop-chk-1.c: Adjust expected diagnostic.
> 	* gcc.dg/builtin-stringop-chk-2.c: Same.
> 	* gcc.dg/builtin-stringop-chk-4.c: New test.
> 	* gcc.dg/builtin-strncat-chk-1.c: Adjust expected diagnostic.
> 	* gcc.dg/memcpy-2.c: Same.
> 	* gcc.dg/pr40340-1.c: Same.
> 	* gcc.dg/pr40340-2.c (main): Same.
> 	* gcc.dg/pr40340-5.c (main): Same.
> 	* gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Same.
> 	* gcc.dg/torture/pr71132.c: Disable -Wstringop-overflow.
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Adjust text of expected
> 	warning.
>
> 	* gcc.dg/fstack-protector-strong.c: Add expected diagnostic.
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index cc711a0..cb7bbaf 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -3011,6 +3013,295 @@ expand_builtin_memcpy_args (tree dest, tree src, tree len, rtx target, tree exp)
>    return dest_addr;
>  }
>
> +/* Fill the 2-element RANGE array with the minimum and maximum values
> +   EXP is known to have and return true, otherwise null and return
> +   false.  */
> +
> +static bool
> +get_size_range (tree exp, tree range[2])
> +{
> +  if (tree_fits_uhwi_p (exp))
> +    {
> +      range[0] = range[1] = exp;
> +      return true;
> +    }
> +
> +  if (TREE_CODE (exp) == SSA_NAME)
> +    {
> +      wide_int min, max;
> +      enum value_range_type range_type = get_range_info (exp, &min, &max);
> +
> +      if (range_type == VR_RANGE)
> +	{
> +	  /* Interpret the bound in the variable's type.  */
> +	  range[0] = wide_int_to_tree (TREE_TYPE (exp), min);
> +	  range[1] = wide_int_to_tree (TREE_TYPE (exp), max);
> +	  return true;
> +	}
> +      else if (range_type == VR_ANTI_RANGE)
> +	{
> +	  /* An anti-range implies the original variable is signed and
> +	     its lower bound is negative and the upper bound positive.
> +	     Since that means that the expression's value could be zero
> +	     nothing interesting can be inferred from this.  */
I'm not sure I agree with this comment.  For example, we often get an 
anti-range like ~[0,0].

I don't mind punting anti-ranges, but I think the comments needs 
updating unless there's something else in this code that ensures that an 
anti range reaching this point fits the property above.


> +/* Try to verify that the sizes and lengths of the arguments to a string
> +   manipulation function given by EXP are within valid bounds and that
> +   the operation does not lead to buffer overflow.  Arguments other than
> +   EXP may be null.  When non-null, the arguments have the following
> +   meaning:
> +   SIZE is the user-supplied size argument to the function (such as in
> +   memcpy(d, s, SIZE) or strncpy(d, s, SIZE).  It specifies the exact
> +   number of bytes to write.
> +   MAXLEN is the user-supplied bound on the length of the source sequence
> +   (such as in strncat(d, s, N).  It specifies the upper limit on the number
> +   of bytes to write.
> +   STR is the source string (such as in strcpy(d, s)) when the epxression
> +   EXP is a string function call (as opposed to a memory call like memcpy).
> +   As an exception, STR can also be an integer denoting the precomputed
> +   length of the source string.
> +   OBJSIZE is the size of the destination object specified by the last
> +   argument to the _chk builtins, typically resulting from the expansion
> +   of __builtin_object_size (such as in __builtin___strcpy_chk(d, s,
> +   OBJSIZE).
> +
> +   When SIZE is null LEN is checked to verify that it doesn't exceed
> +   SIZE_MAX.
> +
> +   If the call is successfully verfified as safe from buffer overflow
> +   the function returns true, otherwise false..  */
s/verfified/verified/


> +
> +  /* There is no way here to determine the length of the string in
> +     the destination to which the SRC string is being appended so
> +     just diagnose cases when the srouce string is longer than
s/srouce/source/


With the comment fixes noted above, this is OK.

jeff