Re: [PATCH] detect nonstring arguments to string functions (PR 82945)

[Jeff Law <law at redhat dot com>]

On 11/13/2017 06:21 PM, Martin Sebor wrote:
> Attached is an improved version that rather than hardcoding
> the built-in functions to check uses the constness of each
> built-in's char* argument as a trigger to do the checking.
> 
> This avoids the problem of the list being incomplete either
> by omission or due to getting out of sync, and also makes it
> trivial to extend the same approach to user-defined functions
> in the future.
> 
> I've excluded strndup and strnlen (but no other "bounded"
> string functions like strncmp) from the checking.
> 
> Martin
> 
> On 11/12/2017 05:52 PM, Martin Sebor wrote:
>> The recently introduced -Wstringop-truncation warning relies
>> on the new nonstring attribute to allow the historical use case
>> of calling strncpy to completely fill the destination with a copy
>> of a string without adding a terminating nul.  Glibc is currently
>> considering making use of the attribute to decorate some of its
>> data members.  To help find misuses of such data members in
>> arguments to string functions like strlen or strdup, the attached
>> patch adds checking for this new attribute in these contexts.
>> The checking is intentionally done late so  that uses such arrays
>> that can be proven to be safe (and thus folded) are not diagnosed.
>>
>> While testing this simple enhancement I noticed that the handling
>> I added for the nul termination in cases like
>>
>>   strncpy (array, s, sizeof array);
>>   array[sizeof array - 1] = 0;
>>
>> to avoid the new warning wasn't quite as robust as it could and
>> arguably should be so I improved it a bit to silently accept
>> more forms of the idiom.  For instance, this is now correctly
>> handled (and not diagnosed):
>>
>>   *stpcpy (array, s, sizeof array - 1) = 0;
>>
>> Martin
>>
>> PS A useful future enhancement would be to detect the one byte
>> overflow in:
>>
>>   *stpcpy (array, s, sizeof array) = 0;
> 
> 
> gcc-82945.diff
> 
> 
> PR tree-optimization/82945 - add warning for passing non-strings to functions that expect string arguments
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/82945
> 	* calls.c (get_attr_nonstring_decl, maybe_warn_nonstring_arg): New
> 	functions.
> 	(initialize_argument_information): Call maybe_warn_nonstring_arg.
> 	* calls.h (get_attr_nonstring_decl): Declare new function.
> 	* doc/extend.texi (attribute nonstring): Update.
> 	* gimple-fold.c (gimple_fold_builtin_strncpy): Call
> 	get_attr_nonstring_decl and handle it.
> 	* tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Same.  Improve
> 	detection of nul-termination.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/82945
> 	* c-c++-common/Wstringop-truncation-2.c: New test.
> 	* c-c++-common/Wstringop-truncation.c: Adjust.
> 	* c-c++-common/attr-nonstring-2.c: Adjust.
> 	* c-c++-common/attr-nonstring-3.c: New test.

> 
> diff --git a/gcc/calls.c b/gcc/calls.c
> index 3730f43..197d907 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -1494,6 +1494,108 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp, tree args[2], int idx[2])
>      }
>  }
>  
> +/* If EXPR refers to a character array or pointer declared attribute
> +   nonstring return a decl for that array or pointer and set *REF to
> +   the referenced enclosing object or pointer.  Otherwise returns
> +   null.  */
Generally we'd write NULL rather than null for a generic pointer and in
this specific case NULL_TREE is even better.

> +
> +tree
> +get_attr_nonstring_decl (tree expr, tree *ref)
> +{
> +  tree dcl = expr;
I think Jakub may have pointed this out, but we generally prefer decl.
Truly a nit though.


> +  if (TREE_CODE (dcl) == SSA_NAME)
> +    {
> +      if (SSA_NAME_IS_DEFAULT_DEF (dcl))
> +	dcl = SSA_NAME_VAR (dcl);
> +      else
> +	{
> +	  gimple *def = SSA_NAME_DEF_STMT (dcl);
> +
> +	  if (is_gimple_assign (def))
> +	    {
> +	      tree_code code = gimple_assign_rhs_code (def);
> +	      if (code == ADDR_EXPR
> +		  || code == COMPONENT_REF
> +		  || code == VAR_DECL)
> +		dcl = gimple_assign_rhs1 (def);
Note that COMPONENT_REFs can have arguments that are themselves
COMPONENT_REFS.  So you typically want to use a loop to strip all away.




> +  /* It's safe to call strndup and strnlen with a non-string argument
> +     since the functions provide an explicit bound for this purpose.  */
> +  if (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_STRNDUP)
> +    return;
So I see you handle BUILT_IN_STRNDUP here, but not BUILT_IN_STRNLEN.  Is
there a reason for the omission?


Jeff