Re: [PATCH v2][ASAN] Implement dynamic allocas/VLAs sanitization.​

[Jakub Jelinek <jakub@redhat.com>]

Hi!

Sorry for the review delay.

On Mon, Jun 26, 2017 at 03:49:23PM +0300, Maxim Ostapenko wrote:
>	(handle_builtin_stackrestore): Likewise.

The function is called with _ between stack and restore.

> 	* match.pd: Add new pattern.

Unless the patch relies on this, I think it should be posted separately
and reviewed by Richard.

> @@ -245,6 +246,7 @@ along with GCC; see the file COPYING3.  If not see
>  static unsigned HOST_WIDE_INT asan_shadow_offset_value;
>  static bool asan_shadow_offset_computed;
>  static vec<char *> sanitized_sections;
> +static tree last_alloca_addr = NULL_TREE;

You are shadowing this variable in multiple places.  Either rename it to
something different, or rename the results of get_last_alloca_addr.
And the " = NULL_TREE" part is not needed.

>  
>  /* Set of variable declarations that are going to be guarded by
>     use-after-scope sanitizer.  */
> @@ -529,11 +531,183 @@ get_mem_ref_of_assignment (const gassign *assignment,
>    return true;
>  }
>  
> +/* Return address of last allocated dynamic alloca.  */
> +
> +static tree
> +get_last_alloca_addr ()
> +{
> +  if (last_alloca_addr)
> +    return last_alloca_addr;
> +
> +  gimple_seq seq = NULL;
> +  gassign *g;
> +
> +  last_alloca_addr = create_tmp_reg (ptr_type_node, "last_alloca_addr");
> +  g = gimple_build_assign (last_alloca_addr, NOP_EXPR,
> +			   build_int_cst (ptr_type_node, 0));

Instead of build_int_cst (ptr_type_node, 0) you should use
null_pointer_node.  And the NOP_EXPR there is just wrong, either it
should be gimple_build_assign (last_alloca_addr, null_pointer_node);
or gimple_build_assign (last_alloca_addr, INTEGER_CST, null_pointer_node);

> +  gimple_seq_add_stmt_without_update (&seq, g);

Why the seq stuff at all?  You have a single stmt you want to insert on
edge.

> +
> +  edge e = single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun));
> +  gsi_insert_seq_on_edge_immediate (e, seq);

So just use here
  gsi_insert_on_edge_immediate (e, g);
instead.

> +  return last_alloca_addr;
> +}
> +
> +/* Insert __asan_allocas_unpoison (top, bottom) call after
> +   __builtin_stack_restore (new_sp) call.
> +   The pseudocode of this routine should look like this:
> +     __builtin_stack_restore (new_sp);
> +     top = last_alloca_addr;
> +     bot = virtual_dynamic_stack_rtx;
> +     __asan_allocas_unpoison (top, bottom);
> +     last_alloca_addr = new_sp;

The comment doesn't seem to agree with what you actually implement.
There is no virtual_dynamic_stack_rtx during the asan pass, it is there
only during expansion until the virtual regs are instantiated in the next
pass.  Furthermore, you have bot variable, but then use bottom.

> +  tree last_alloca_addr = get_last_alloca_addr ();

Here is the shadowing I talked about.

> +  tree restored_stack = gimple_call_arg (call, 0);
> +  tree fn = builtin_decl_implicit (BUILT_IN_ASAN_ALLOCAS_UNPOISON);
> +  gimple *g = gimple_build_call (fn, 2, last_alloca_addr, restored_stack);

Here you clearly use the first argument of __builtin_stack_restore, which
is that new_sp.

> +  gimple_seq_add_stmt_without_update (&seq, g);

Why the messing up with sequences?  Just insert the stmt immediately in,
and the others as well.

> +  g = gimple_build_assign (last_alloca_addr, NOP_EXPR, restored_stack);

This is again wrong, here you really don't know what restored_stack is,
it could be SSA_NAME, but also something different, so you should use
gimple_build_assign (last_alloca_addr, restored_stack);
and let it figure out the rhs code.

> +  /* Extract lower bits from old_size.  */
> +  wide_int size_nonzero_bits = get_nonzero_bits (old_size);
> +  wide_int rz_mask
> +    = wi::uhwi (redzone_mask, wi::get_precision (size_nonzero_bits));
> +  wide_int old_size_lower_bits = wi::bit_and (size_nonzero_bits, rz_mask);
> +
> +  /* If alloca size is aligned to ASAN_RED_ZONE_SIZE, we don't need partial
> +     redzone.  Otherwise, compute its size here.  */
> +  if (wi::ne_p (old_size_lower_bits, 0))
> +    {
> +      /* misalign = size & (ASAN_RED_ZONE_SIZE - 1)
> +         partial_size = ASAN_RED_ZONE_SIZE - misalign.  */
> +      g = gimple_build_assign (make_ssa_name (size_type_node, NULL),
> +			       BIT_AND_EXPR, old_size, alloca_rz_mask);
> +      gimple_seq_add_stmt_without_update (&seq, g);
> +      tree misalign = gimple_assign_lhs (g);
> +      g = gimple_build_assign (make_ssa_name (size_type_node, NULL), MINUS_EXPR,
> +			       redzone_size, misalign);
> +      gimple_seq_add_stmt_without_update (&seq, g);

Again, why add the stmts into a seq first instead of just adding it
immediately into the IL?
> @@ -4962,6 +4962,20 @@ expand_builtin_alloca (tree exp)
>    return result;
>  }
>  

Missing function comment here.

> +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 see you are here pretty much ignoring the old second argument and instead
using a different one.  But then the above mentioned comment should explain
that, how you transform it during the asan pass and how you later change it
during expansion.

> +    case BUILT_IN_ASAN_ALLOCAS_UNPOISON:
> +      target = expand_asan_emit_allocas_unpoison (exp);
> +      if (target)
> +	return target;

Do you need this test, when it always returns non-NULL?
Just return expand_asan_emit_allocas_unpoison (exp);

	Jakub