Re: [PATCH] warn on returning alloca and VLA (PR 71924, 90549)

[Martin Sebor <msebor@gmail.com>]

On 5/29/19 11:45 AM, Jeff Law wrote:
> On 5/22/19 3:34 PM, Martin Sebor wrote:
>> -Wreturn-local-addr detects a subset of instances of returning
>> the address of a local object from a function but the warning
>> doesn't try to handle alloca or VLAs, or some non-trivial cases
>> of ordinary automatic variables[1].
>>
>> The attached patch extends the implementation of the warning to
>> detect those.  It still doesn't detect instances where the address
>> is the result of a built-in such strcpy[2].
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> [1] For example, this is only diagnosed with the patch:
>>
>>    void* f (int i)
>>    {
>>      struct S { int a[2]; } s[2];
>>      return &s->a[i];
>>    }
>>
>> [2] The following is not diagnosed even with the patch:
>>
>>    void sink (void*);
>>
>>    void* f (int i)
>>    {
>>      char a[6];
>>      char *p = __builtin_strcpy (a, "123");
>>      sink (p);
>>      return p;
>>    }
>>
>> I would expect detecting to be possible and useful.  Maybe as
>> a follow-up.
>>
>> gcc-71924.diff
>>
>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result
>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset
>>
>> gcc/ChangeLog:
>>
>> 	PR c/71924
>> 	* gimple-ssa-isolate-paths.c (is_addr_local): New function.
>> 	(warn_return_addr_local_phi_arg, warn_return_addr_local): Same.
>> 	(find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg.
>> 	(find_explicit_erroneous_behavior): Call warn_return_addr_local.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c/71924
>> 	* gcc.dg/Wreturn-local-addr-2.c: New test.
>> 	* gcc.dg/Walloca-4.c: Prune expected warnings.
>> 	* gcc.dg/pr41551.c: Same.
>> 	* gcc.dg/pr59523.c: Same.
>> 	* gcc.dg/tree-ssa/pr88775-2.c: Same.
>> 	* gcc.dg/winline-7.c: Same.
>>
>> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
>> index 33fe352bb23..2933ecf502e 100644
>> --- a/gcc/gimple-ssa-isolate-paths.c
>> +++ b/gcc/gimple-ssa-isolate-paths.c
>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt)
>>     return false;
>>   }
>>   
>> +/* Return true if EXPR is a expression of pointer type that refers
>> +   to the address of a variable with automatic storage duration.
>> +   If so, set *PLOC to the location of the object or the call that
>> +   allocated it (for alloca and VLAs).  When PMAYBE is non-null,
>> +   also consider PHI statements and set *PMAYBE when some but not
>> +   all arguments of such statements refer to local variables, and
>> +   to clear it otherwise.  */
>> +
>> +static bool
>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL,
>> +	       hash_set<gphi *> *visited = NULL)
>> +{
>> +  if (TREE_CODE (exp) == ADDR_EXPR)
>> +    {
>> +      tree baseaddr = get_base_address (TREE_OPERAND (exp, 0));
>> +      if (TREE_CODE (baseaddr) == MEM_REF)
>> +	return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, visited);
>> +
>> +      if ((!VAR_P (baseaddr)
>> +	   || is_global_var (baseaddr))
>> +	  && TREE_CODE (baseaddr) != PARM_DECL)
>> +	return false;
>> +
>> +      *ploc = DECL_SOURCE_LOCATION (baseaddr);
>> +      return true;
>> +    }
>> +
>> +  if (TREE_CODE (exp) == SSA_NAME)
>> +    {
>> +      gimple *def_stmt = SSA_NAME_DEF_STMT (exp);
>> +      enum gimple_code code = gimple_code (def_stmt);
>> +
>> +      if (is_gimple_assign (def_stmt))
>> +	{
>> +	  tree type = TREE_TYPE (gimple_assign_lhs (def_stmt));
>> +	  if (POINTER_TYPE_P (type))
>> +	    {
>> +	      tree ptr = gimple_assign_rhs1 (def_stmt);
>> +	      return is_addr_local (ptr, ploc, pmaybe, visited);
>> +	    }
>> +	  return false;
>> +	}
>> +
>> +      if (code == GIMPLE_CALL
>> +	  && gimple_call_builtin_p (def_stmt))
>> +	{
>> +	  tree fn = gimple_call_fndecl (def_stmt);
>> +	  int code = DECL_FUNCTION_CODE (fn);
>> +	  if (code != BUILT_IN_ALLOCA
>> +	      && code != BUILT_IN_ALLOCA_WITH_ALIGN)
>> +	    return false;
>> +
>> +	  *ploc = gimple_location (def_stmt);
>> +	  return true;
>> +	}
>> +
>> +      if (code == GIMPLE_PHI && pmaybe)
>> +	{
>> +	  unsigned count = 0;
>> +	  gphi *phi_stmt = as_a <gphi *> (def_stmt);
>> +
>> +	  unsigned nargs = gimple_phi_num_args (phi_stmt);
>> +	  for (unsigned i = 0; i < nargs; ++i)
>> +	    {
>> +	      if (!visited->add (phi_stmt))
>> +		{
>> +		  tree arg = gimple_phi_arg_def (phi_stmt, i);
>> +		  if (is_addr_local (arg, ploc, pmaybe, visited))
>> +		    ++count;
>> +		}
>> +	    }
>> +
>> +	  *pmaybe = count && count < nargs;
>> +	  return count != 0;
>> +	}
>> +    }
>> +
>> +  return false;
>> +}
> Is there some reason you didn't query the alias oracle here?  It would
> seem a fairly natural fit.  Ultimately given a pointer (which will be an
> SSA_NAME) you want to ask whether or not it conclusively points into the
> stack.
> 
> That would seem to dramatically simplify is_addr_local.

I did think about it but decided against changing the existing
design (iterating over PHI arguments), for a couple of reasons:

1) It feels like a bigger change than my simple "bug fix" calls
    for.
2) I'm not familiar enough with the alias oracle to say for sure
    if it can be used to give the same results as the existing
    implementation.  I.e., make it possible to identify and
    isolate each path that returns a local address (rather than
    just answering: yes, this pointer may point to some local
    in this function).

If the alias oracle can be used to give the same results without
excessive false positives then I think it would be fine to make
use of it.  Is that something you consider a prerequisite for
this change or should I look into it as a followup?

FWIW, I'm working on enhancing this to detect returning freed
pointers (under a different option).  That seems like a good
opportunity to also look into making use of the alias oracle.

Besides these enhancements, there's also a request to diagnose
dereferencing pointers to compound literals whose lifetime has
ended (PR 89990), or more generally, those to any such local
object.  It's about preventing essentially the same problem
(accessing bad data or corrupting others) but it seems that
both the analysis and the handling will be sufficiently
different to consider implementing it somewhere else.  What
are your thoughts on that?

Martin

> 
> The rest looks pretty reasonable.
> 
> Jeff
>