Re: [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer.

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Apr 08, 2016 at 11:52:32AM +0300, Maxim Ostapenko wrote:
> 2016-04-08  Maxim Ostapenko  <m.ostapenko@samsung.com>
> 
> 	PR sanitizer/70541
> 	* asan.c (instrument_derefs): If we get unknown location, extract it
> 	with EXPR_LOCATION.
> 	(maybe_instrument_call): Instrument gimple_call's arguments if needed.
> 	* tree-inline.c (initialize_inlined_parameters): Set location for
> 	inlined parameters initialization statements.
> 
> gcc/testsuite/ChangeLog:
> 
> 2016-04-08  Maxim Ostapenko  <m.ostapenko@samsung.com>
> 
> 	PR sanitizer/70541
> 	* c-c++-common/asan/pr70541-1.c: New test.
> 	* c-c++-common/pr70541-2.c: Likewise.

> +  /* Walk through gimple_call arguments and check them id needed.  */
> +  unsigned args_num = gimple_call_num_args (stmt);
> +  for (unsigned i = 0; i < args_num; ++i)
> +    {
> +      tree arg = gimple_call_arg (stmt, i);
> +      gcc_assert (TREE_CODE (arg) != CALL_EXPR);

Please remove this assert, of course in GIMPLE arguments can't be
CALL_EXPRs, they must satisfy is_gimple_val or is_gimple_lvalue, but
that is verified in verify_gimple_call.

> +      /* If ARG is not a non-aggregate register variable, compiler in general
> +	 creates temporary for it and pass it as argument to gimple call.
> +	 But in some cases, e.g. when we pass by value a small structure that
> +	 fits to register, compiler can avoid extra overhead by pulling out
> +	 these temporaries.  In this case, we should check the argument.  */
> +      if (!is_gimple_reg (arg))

Actually, I believe this should be
      if (!is_gimple_reg (arg) && !is_gimple_min_invariant (arg))
because constants, or addresses etc. also aren't memory references.
I know instrument_derefs exits early if the t isn't one of the selected
few trees, but still, it looks unclean to call instrument_derefs on
constants or addresses.

The asan.c changes look good to me with those changes, feel free to commit
them separately from the tree-inline.c change (perhaps with something in the
testcase XFAILed for now).

> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -3247,7 +3247,8 @@ initialize_inlined_parameters (copy_body_data *id, gimple *stmt,
>      {
>        tree val;
>        val = i < gimple_call_num_args (stmt) ? gimple_call_arg (stmt, i) : NULL;
> -      setup_one_parameter (id, p, val, fn, bb, &vars);
> +      if (gimple *new_stmt = setup_one_parameter (id, p, val, fn, bb, &vars))
> +	gimple_set_location (new_stmt, gimple_location (stmt));
>      }
>    /* After remapping parameters remap their types.  This has to be done
>       in a second loop over all parameters to appropriately remap
> @@ -3285,7 +3286,9 @@ initialize_inlined_parameters (copy_body_data *id, gimple *stmt,
>        /* No static chain?  Seems like a bug in tree-nested.c.  */
>        gcc_assert (static_chain);
>  
> -      setup_one_parameter (id, p, static_chain, fn, bb, &vars);
> +      if (gimple *new_stmt = setup_one_parameter (id, p, static_chain, fn, bb,
> +						  &vars))
> +	gimple_set_location (new_stmt, gimple_location (stmt));
>      }
>  
>    declare_inline_vars (id->block, vars);

The tree-inline.c change looks much more risky at this point, could we defer
that for GCC 7?    Unlike the asan change this is going to affect debugging, etc.
Maybe e.g. pass gimple_location (stmt) as argument to setup_one_parameter
and set it in there instead?  Sometimes there are multiple statements
created in there, not just one, etc.  In any case, I'd prefer second pair of
eyes on the tree-inline.c changes, so I'm CCing Richi and Honza.

	Jakub