[PATCH 3/4] enhance overflow and truncation detection in strncpy and strncat (PR 81117)

Thu Aug 10 07:39:00 GMT 2017

On August 10, 2017 7:26:33 AM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>On 08/06/2017 02:07 PM, Martin Sebor wrote:
>> Part 3 of the series contains the meat of the patch: the new
>> -Wstringop-truncation option, and enhancements to -Wstringop-
>> overflow, and -Wpointer-sizeof-memaccess to detect misuses of
>> strncpy and strncat.
>> 
>> Martin
>> 
>> gcc-81117-3.diff
>> 
>> 
>> PR c/81117 - Improve buffer overflow checking in strncpy
>> 
>> gcc/ChangeLog:
>> 
>> 	PR c/81117
>> 	* builtins.c (compute_objsize): Handle arrays that
>> 	compute_builtin_object_size likes to fail for.  Make extern.
>> 	* builtins.h (compute_objsize): Declare.
>> 	(check_strncpy_sizes): New function.
>> 	(expand_builtin_strncpy): Call check_strncpy_sizes.
>> 	* gimple-fold.c (gimple_fold_builtin_strncpy): Implement
>> 	-Wstringop-truncation.
>> 	(gimple_fold_builtin_strncat): Same.
>> 	* gimple.c (gimple_build_call_from_tree): Set call location.
>> 	* tree-ssa-strlen.c (strlen_to_stridx): New global variable.
>> 	(maybe_diag_bound_equal_length, is_strlen_related_p): New functions.
>> 	(handle_builtin_stxncpy, handle_builtin_strncat): Same.
>> 	(handle_builtin_strlen): Use strlen_to_stridx.
>> 	(strlen_optimize_stmt): Handle flavors of strncat, strncpy, and
>> 	stpncpy.
>> 	Use strlen_to_stridx.
>> 	(pass_strlen::execute): Release strlen_to_stridx.
>> 	* doc/invoke.texi (-Wsizeof-pointer-memaccess): Document
>enhancement.
>> 	(-Wstringop-truncation): Document new option.
>> 
>> gcc/c-family/ChangeLog:
>> 
>> 	PR c/81117
>> 	* c-warn.c (sizeof_pointer_memaccess_warning): Handle arrays.
>> 	* c.opt (-Wstriingop-truncation): New option.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 	PR c/81117
>> 	* c-c++-common/Wsizeof-pointer-memaccess3.c: New test.
>> 	* c-c++-common/Wstringop-overflow.c: Same.
>> 	* c-c++-common/Wstringop-truncation.c: Same.
>> 	* c-c++-common/Wsizeof-pointer-memaccess2.c: Adjust.
>> 	* c-c++-common/attr-nonstring-2.c: New test.
>> 	* g++.dg/torture/Wsizeof-pointer-memaccess1.C: Adjust.
>> 	* g++.dg/torture/Wsizeof-pointer-memaccess2.C: Same.
>> 	* gcc.dg/torture/pr63554.c: Same.
>> 	* gcc.dg/Walloca-1.c: Disable macro tracking.
>> 
>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>> index 016f68d..1aa9e22 100644
>> --- a/gcc/builtins.c
>> +++ b/gcc/builtins.c
>[ ... ]
>> +
>> +  if (TREE_CODE (type) == ARRAY_TYPE)
>> +    {
>> +      /* Return the constant size unless it's zero (that's a
>zero-length
>> +	 array likely at the end of a struct).  */
>> +      tree size = TYPE_SIZE_UNIT (type);
>> +      if (size && TREE_CODE (size) == INTEGER_CST
>> +	  && !integer_zerop (size))
>> +	return size;
>> +    }
>Q. Do we have a canonical test for the trailing array idiom?  

Array_at_struct_end_p

 In some
>contexts isn't it size 1?  ISTM This test needs slight improvement.
>Ideally we'd use some canonical test for detect the trailing array
>idiom
>rather than open-coding it here.  You might look at the array index
>warnings in tree-vrp.c to see if it's got a canonical test you can call
>or factor and use.
>
>
>
>
>> @@ -3883,6 +3920,30 @@ expand_builtin_strncat (tree exp, rtx)
>>    return NULL_RTX;
>>  }
>>  
>> +/* Helper to check the sizes of sequences and the destination of
>calls
>> +   to __builtin_strncpy (DST, SRC, LEN) and __builtin___strncpy_chk.
>> +   Returns true on success (no overflow warning), false otherwise. 
>*/
>> +
>> +static bool
>> +check_strncpy_sizes (tree exp, tree dst, tree src, tree len)
>> +{
>> +  tree dstsize = compute_objsize (dst, warn_stringop_overflow - 1);
>> +
>> +  if (!check_sizes (OPT_Wstringop_overflow_,
>> +		    exp, len, /*maxlen=*/NULL_TREE, src, dstsize))
>> +    return false;
>> +
>> +  if (!dstsize || TREE_CODE (len) != INTEGER_CST)
>> +    return true;
>> +
>> +  if (tree_int_cst_lt (dstsize, len))
>> +    warning_at (EXPR_LOCATION (exp), OPT_Wstringop_truncation,
>> +		"%K%qD specified bound %E exceeds destination size %E",
>> +		exp, get_callee_fndecl (exp), len, dstsize);
>> +
>> +  return true;
>So in the case where you issue the warning, what should the return
>value
>be?  According to the comment it should be false.  It looks like you
>got
>the wrong return value for the tree_int_cst_lt (dstsize, len) test.
>
>
>
>
>
>>  
>>    return false;
>> diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
>> index b0563fe..ac6503f 100644
>> --- a/gcc/tree-ssa-strlen.c
>> +++ b/gcc/tree-ssa-strlen.c
>
>> +
>> +/* A helper of handle_builtin_stxncpy.  Check to see if the
>specified
>> +   bound is a) equal to the size of the destination DST and if so,
>b)
>> +   if it's immediately followed by DST[LEN - 1] = '\0'.  If a) holds
>> +   and b) does not, warn.  Otherwise, do nothing.  Return true if
>> +   diagnostic has been issued.
>> +
>> +   The purpose is to diagnose calls to strncpy and stpncpy that do
>> +   not nul-terminate the copy while allowing for the idiom where
>> +   such a call is immediately followed by setting the last element
>> +   to nul, as in:
>> +     char a[32];
>> +     strncpy (a, s, sizeof a);
>> +     a[sizeof a - 1] = '\0';
>> +*/
>So using gsi_next to find the next statement could make the heuristic
>fail to find the a[sizeof a - 1] = '\0'; statement when debugging is
>enabled.
>
>gsi_next_nondebug would be better as it would skip over any debug
>insns.
>
>What might be even better would be to use the immediate uses of the
>memory tag.  For your case there should be only one immediate use and
>it
>should point to the statement which NUL terminates the destination.  Or
>maybe that would be worse in that you only want to allow this exception
>when the statements are consecutive.
>
>
>> +
>> +  /* Look for dst[i] = '\0'; after the stxncpy() call and if found
>> +     avoid the truncation warning.  */
>> +  gsi_next (&gsi);
>> +  gimple *next_stmt = gsi_stmt (gsi);
>Here's the gsi_next I'm referring to.
>
>
>> +  else
>> +    {
>> +      /* The source length is uknown.  Try to determine the
>destination
>s/uknown/unknown/
>
>
>
>>  /* Handle a memcpy-like ({mem{,p}cpy,__mem{,p}cpy_chk}) call.
>>     If strlen of the second argument is known and length of the third
>argument
>>     is that plus one, strlen of the first argument is the same after
>this
>> @@ -2512,6 +2789,19 @@ strlen_optimize_stmt (gimple_stmt_iterator
>*gsi)
>You still need to rename strlen_optimize_stmt since after your changes
>it does both optimizations and warnings.
>
>I think we're going to need one more iteration on this patch within the
>kit.  I'm glazing over a bit tonight.
>
>jeff