[PATCH v3][ASAN] Implement dynamic allocas/VLAs sanitization.​

[Jakub Jelinek]

On Wed, Jul 05, 2017 at 11:24:15AM +0300, Maxim Ostapenko wrote:
> +   In general, can't we use new_sp as bot parameter because on some

s/can't we/we can't/

> +  /* new_alloca = new_alloca_with_rz + align.  */
> +  g = gimple_build_assign (make_ssa_name (ptr_type), POINTER_PLUS_EXPR,
> +			   new_alloca_with_rz,
> +			   build_int_cst (size_type_node,
> +					  align / BITS_PER_UNIT));
> +  gsi_insert_before (iter, g, GSI_SAME_STMT);
> +  tree new_alloca = gimple_assign_lhs (g);
> +
> +  /* Replace old alloca ptr with NEW_ALLOCA.  */
> +  replace_call_with_value (iter, new_alloca);
> +
> +  /* Poison newly created alloca redzones:
> +      __asan_alloca_poison (new_alloca, old_size).  */
> +  fn = builtin_decl_implicit (BUILT_IN_ASAN_ALLOCA_POISON);
> +  gg = gimple_build_call (fn, 2, new_alloca, old_size);
> +  gsi_insert_before (iter, gg, GSI_SAME_STMT);
> +
> +  /* Save new_alloca_with_rz value into last_alloca to use it during
> +     allocas unpoisoning.  */
> +  g = gimple_build_assign (last_alloca, new_alloca_with_rz);
> +  gsi_insert_before (iter, g, GSI_SAME_STMT);

I think the replace_call_with_value should go only after these two,
so that it matches the order in which the stmts are emitted.
Or maybe better, keep the __builtin_alloca call stmt in the IL,
instead of add another one, just change its argument and return value,
and add some new stmts before the call and some after the call,
then you wouldn't need to export replace_call_with_value.

Also, the function comment above handle_builtin_alloca describes
only small portion of the statements you actually emit, can you please
update it so that it also lists __asan_alloca_poison etc.?

> +/* Emit a call to __asan_allocas_unpoison call in EXP.  Replace second argument
> +   of the call with virtual_stack_dynamic_rtx because in asan pass we emit a
> +   dummy value into second parameter relying on this function to perform the
> +   change.  See motivation for this in comment to handle_builtin_stack_restore
> +   function.  */
> +
> +static rtx
> +expand_asan_emit_allocas_unpoison (tree exp)
> +{
> +  tree arg0 = CALL_EXPR_ARG (exp, 0);
> +  rtx top = expand_expr (arg0, NULL_RTX, GET_MODE (virtual_stack_dynamic_rtx),
> +			 EXPAND_NORMAL);
> +  rtx ret = init_one_libfunc ("__asan_allocas_unpoison");
> +  ret = emit_library_call_value (ret, NULL_RTX, LCT_NORMAL, ptr_mode, 2, top,
> +				 TYPE_MODE (pointer_sized_int_node),
> +				 virtual_stack_dynamic_rtx,
> +				 TYPE_MODE (pointer_sized_int_node));

I think another possibility to implement this would be
  CALL_EXPR_ARG (exp, 1)
    = make_tree (pointer_sized_int_mode, virtual_stack_dynamic_rtx);
  return expand_call (exp, const0_rtx, 1);

Otherwise LGTM.

	Jakub