public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] ASAN: handle addressable params (PR sanitize/81040).
@ 2017-06-19 13:50 Martin Liška
  2017-06-19 14:13 ` Jakub Jelinek
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Liška @ 2017-06-19 13:50 UTC (permalink / raw)
  To: GCC Patches; +Cc: Jakub Jelinek

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

Hi.

Following patch addresses issue where we have a function argument which address
is taken and -fsanitize=address does not wrap up the argument with red zone.
It's done in sanopt pass, where I create a new automatic variable which is used
in the function instead of the original argument.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
And I can bootstrap-asan on the same machine.

Ready to be installed?
Martin

[-- Attachment #2: 0001-ASAN-handle-addressable-params-PR-sanitize-81040.patch --]
[-- Type: text/x-patch, Size: 7939 bytes --]

From f8a48a3f361d9914dd45c1896e8c5ba607a62b06 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 14 Jun 2017 11:40:01 +0200
Subject: [PATCH] ASAN: handle addressable params (PR sanitize/81040).

gcc/testsuite/ChangeLog:

2017-06-19  Martin Liska  <mliska@suse.cz>

	PR sanitize/81040
	* g++.dg/asan/function-argument-1.C: New test.
	* g++.dg/asan/function-argument-2.C: New test.
	* g++.dg/asan/function-argument-3.C: New test.

gcc/ChangeLog:

2017-06-19  Martin Liska  <mliska@suse.cz>

	PR sanitize/81040
	* sanopt.c (rewrite_usage_of_param): New function.
	(sanitize_rewrite_addressable_params): Likewise.
	(pass_sanopt::execute): Call rewrite_usage_of_param.
---
 gcc/sanopt.c                                    | 118 ++++++++++++++++++++++++
 gcc/testsuite/g++.dg/asan/function-argument-1.C |  30 ++++++
 gcc/testsuite/g++.dg/asan/function-argument-2.C |  24 +++++
 gcc/testsuite/g++.dg/asan/function-argument-3.C |  27 ++++++
 4 files changed, 199 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-1.C
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-2.C
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-3.C

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 16bdba76042..10464841972 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -37,6 +37,10 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-ssa.h"
 #include "tree-phinodes.h"
 #include "ssa-iterators.h"
+#include "gimplify.h"
+#include "gimple-iterator.h"
+#include "gimple-walk.h"
+#include "cfghooks.h"
 
 /* This is used to carry information about basic blocks.  It is
    attached to the AUX field of the standard CFG block.  */
@@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void)
     }
 }
 
+static tree
+rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data)
+{
+  struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
+  std::pair<tree, tree> *replacement = (std::pair<tree, tree> *)wi->info;
+
+  if (*op == replacement->first)
+    {
+      *op = replacement->second;
+      *walk_subtrees = 0;
+    }
+
+  return NULL;
+}
+
+/* For a given function FUN, rewrite all addressable parameters so that
+   a new automatic variable is introduced.  Right after function entry
+   a parameter is assigned to the variable.  */
+
+static void
+sanitize_rewrite_addressable_params (function *fun)
+{
+  basic_block entry_bb = NULL;
+
+  for (tree arg = DECL_ARGUMENTS (current_function_decl);
+       arg; arg = DECL_CHAIN (arg))
+    {
+      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
+	{
+	  /* The parameter is no longer addressable.  */
+	  tree type = TREE_TYPE (arg);
+	  TREE_ADDRESSABLE (arg) = 0;
+
+	  /* Create a new automatic variable.  */
+	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
+				 VAR_DECL, DECL_NAME (arg), type);
+	  TREE_ADDRESSABLE (var) = 1;
+	  DECL_ARTIFICIAL (var) = 1;
+	  DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
+
+	  gimple_add_tmp_var (var);
+
+	  if (dump_file)
+	    fprintf (dump_file,
+		     "Rewritting parameter whos address is taken: %s\n",
+		     IDENTIFIER_POINTER (DECL_NAME (arg)));
+
+	  gimple_seq stmts = NULL;
+
+	  /* Assign value of parameter to newly created variable.  */
+	  if ((TREE_CODE (type) == COMPLEX_TYPE
+	       || TREE_CODE (type) == VECTOR_TYPE))
+	    {
+	      /* We need to create a SSA name that will be used for the
+		 assignment.  */
+	      tree tmp = make_ssa_name (type);
+	      gimple *g = gimple_build_assign (tmp, arg);
+	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+	      gimple_seq_add_stmt (&stmts, g);
+	      g = gimple_build_assign (var, tmp);
+	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+	      gimple_seq_add_stmt (&stmts, g);
+	    }
+	  else
+	    {
+	      gimple *g = gimple_build_assign (var, arg);
+	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+	      gimple_seq_add_stmt (&stmts, g);
+	    }
+
+	  /* Replace all usages of PARM_DECL with the newly
+	     created variable VAR.  */
+	  basic_block bb;
+	  gimple_stmt_iterator gsi;
+	  FOR_EACH_BB_FN (bb, fun)
+	    {
+	      std::pair<tree, tree> replacement (arg, var);
+	      struct walk_stmt_info wi;
+	      memset (&wi, 0, sizeof (wi));
+	      wi.info = (void *)&replacement;
+
+	      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+		{
+		  gimple *stmt = gsi_stmt (gsi);
+		  gimple_stmt_iterator it = gsi_for_stmt (stmt);
+		  walk_gimple_stmt (&it, NULL, rewrite_usage_of_param, &wi);
+		}
+	      for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+		{
+		  gphi *phi = dyn_cast<gphi *> (gsi_stmt (gsi));
+		  for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
+		    {
+		      hash_set<tree> visited_nodes;
+		      walk_tree (gimple_phi_arg_def_ptr (phi, i),
+				 rewrite_usage_of_param, &wi, &visited_nodes);
+		    }
+		}
+	    }
+
+	  if (entry_bb == NULL)
+	    {
+	      entry_bb = ENTRY_BLOCK_PTR_FOR_FN (fun);
+	      entry_bb = split_edge (single_succ_edge (entry_bb));
+	    }
+
+	  gimple_stmt_iterator gsi2 = gsi_start_bb (entry_bb);
+	  gsi_insert_seq_before (&gsi2, stmts, GSI_NEW_STMT);
+	}
+    }
+}
+
 unsigned int
 pass_sanopt::execute (function *fun)
 {
@@ -891,6 +1006,9 @@ pass_sanopt::execute (function *fun)
       sanitize_asan_mark_poison ();
     }
 
+  if (asan_sanitize_stack_p ())
+    sanitize_rewrite_addressable_params (fun);
+
   bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX
     && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
 
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-1.C b/gcc/testsuite/g++.dg/asan/function-argument-1.C
new file mode 100644
index 00000000000..148c4628316
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-1.C
@@ -0,0 +1,30 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+struct A
+{
+  int a[5];
+};
+
+static __attribute__ ((noinline)) int
+goo (A *a)
+{
+  int *ptr = &a->a[0];
+  return *(volatile int *) (ptr - 1);
+}
+
+__attribute__ ((noinline)) int
+foo (A arg)
+{
+  return goo (&arg);
+}
+
+int
+main ()
+{
+  return foo (A ());
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-underflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* underflows this variable.*" }
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-2.C b/gcc/testsuite/g++.dg/asan/function-argument-2.C
new file mode 100644
index 00000000000..3a7c33bdaaa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-2.C
@@ -0,0 +1,24 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+static __attribute__ ((noinline)) int
+goo (int *a)
+{
+  return *(volatile int *)a;
+}
+
+__attribute__ ((noinline)) int
+foo (char arg)
+{
+  return goo ((int *)&arg);
+}
+
+int
+main ()
+{
+  return foo (12);
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* partially overflows this variable.*" }
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-3.C b/gcc/testsuite/g++.dg/asan/function-argument-3.C
new file mode 100644
index 00000000000..14617ba8425
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-3.C
@@ -0,0 +1,27 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+typedef int v4si __attribute__ ((vector_size (16)));
+
+static __attribute__ ((noinline)) int
+goo (v4si *a)
+{
+  return (*(volatile v4si *) (a + 1))[2];
+}
+
+__attribute__ ((noinline)) int
+foo (v4si arg)
+{
+  return goo (&arg);
+}
+
+int
+main ()
+{
+  v4si v = {1,2,3,4};
+  return foo (v);
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* overflows this variable.*" }
-- 
2.13.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-19 13:50 [PATCH] ASAN: handle addressable params (PR sanitize/81040) Martin Liška
@ 2017-06-19 14:13 ` Jakub Jelinek
  2017-06-20  9:23   ` Martin Liška
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2017-06-19 14:13 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Mon, Jun 19, 2017 at 03:50:42PM +0200, Martin Liška wrote:
> @@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void)
>      }
>  }
>  

Missing function comment.

> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data)
> +{
> +  struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
> +  std::pair<tree, tree> *replacement = (std::pair<tree, tree> *)wi->info;

Missing space after )

> +
> +  if (*op == replacement->first)
> +    {
> +      *op = replacement->second;
> +      *walk_subtrees = 0;
> +    }
> +
> +  return NULL;
> +}

> +static void
> +sanitize_rewrite_addressable_params (function *fun)
> +{
> +  basic_block entry_bb = NULL;
> +
> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
> +       arg; arg = DECL_CHAIN (arg))
> +    {
> +      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> +	{
> +	  /* The parameter is no longer addressable.  */
> +	  tree type = TREE_TYPE (arg);
> +	  TREE_ADDRESSABLE (arg) = 0;
> +
> +	  /* Create a new automatic variable.  */
> +	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
> +				 VAR_DECL, DECL_NAME (arg), type);
> +	  TREE_ADDRESSABLE (var) = 1;
> +	  DECL_ARTIFICIAL (var) = 1;
> +	  DECL_SEEN_IN_BIND_EXPR_P (var) = 0;

I think it is highly inefficient to walk the whole IL for every addressable
argument.  Can't you first find out what PARM_DECLs you need to change,
stick the corresponding VAR_DECL somewhere (dunno, e.g. a vector with pairs
perhaps sorted by DECL_UID, or stick it into DECL_VALUE_EXPR or whatever),
then if you create at least one, walk whole IL and replace all the
PARM_DECLs you want to replace, then finally clear the TREE_ADDRESSABLE
flag for all of them and emit the initialization sequence?
Then something needs to be done for debugging too.  If it is without VTA,
then probably just having DECL_VALUE_EXPR is good enough, otherwise
(VTA) you probably don't want that (or can reset it at that point), but
instead emit after the initialization stmt a debug stmt that the variable
value now lives in a different var.  Though ideally we want the debugger
to be able to also change the value of the var, that might be harder.
With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
the prologue until it is assigned to the slot.

> +
> +	  gimple_add_tmp_var (var);
> +
> +	  if (dump_file)
> +	    fprintf (dump_file,
> +		     "Rewritting parameter whos address is taken: %s\n",
> +		     IDENTIFIER_POINTER (DECL_NAME (arg)));

s/tting/ting/, s/whos/whose/ 
> +
> +	  gimple_seq stmts = NULL;
> +
> +	  /* Assign value of parameter to newly created variable.  */
> +	  if ((TREE_CODE (type) == COMPLEX_TYPE
> +	       || TREE_CODE (type) == VECTOR_TYPE))
> +	    {
> +	      /* We need to create a SSA name that will be used for the
> +		 assignment.  */
> +	      tree tmp = make_ssa_name (type);
> +	      gimple *g = gimple_build_assign (tmp, arg);
> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
> +	      gimple_seq_add_stmt (&stmts, g);
> +	      g = gimple_build_assign (var, tmp);
> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
> +	      gimple_seq_add_stmt (&stmts, g);
> +	    }
> +	  else
> +	    {
> +	      gimple *g = gimple_build_assign (var, arg);
> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
> +	      gimple_seq_add_stmt (&stmts, g);
> +	    }

I don't understand the distinction.  If you turn the original parm
for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
(but I think it would be better to use the default SSA_NAME of the PARM_DECL
if it is a gimple reg type, rather than use the PARM_DECL itself
and wait for update_ssa).

	Jakub

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-19 14:13 ` Jakub Jelinek
@ 2017-06-20  9:23   ` Martin Liška
  2017-06-20  9:32     ` Jakub Jelinek
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Liška @ 2017-06-20  9:23 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC Patches

On 06/19/2017 04:13 PM, Jakub Jelinek wrote:
> On Mon, Jun 19, 2017 at 03:50:42PM +0200, Martin Liška wrote:
>> @@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void)
>>      }
>>  }
>>  
> 
> Missing function comment.
> 
>> +static tree
>> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data)
>> +{
>> +  struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
>> +  std::pair<tree, tree> *replacement = (std::pair<tree, tree> *)wi->info;
> 
> Missing space after )
> 
>> +
>> +  if (*op == replacement->first)
>> +    {
>> +      *op = replacement->second;
>> +      *walk_subtrees = 0;
>> +    }
>> +
>> +  return NULL;
>> +}
> 
>> +static void
>> +sanitize_rewrite_addressable_params (function *fun)
>> +{
>> +  basic_block entry_bb = NULL;
>> +
>> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
>> +       arg; arg = DECL_CHAIN (arg))
>> +    {
>> +      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
>> +	{
>> +	  /* The parameter is no longer addressable.  */
>> +	  tree type = TREE_TYPE (arg);
>> +	  TREE_ADDRESSABLE (arg) = 0;
>> +
>> +	  /* Create a new automatic variable.  */
>> +	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
>> +				 VAR_DECL, DECL_NAME (arg), type);
>> +	  TREE_ADDRESSABLE (var) = 1;
>> +	  DECL_ARTIFICIAL (var) = 1;
>> +	  DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
> 
> I think it is highly inefficient to walk the whole IL for every addressable
> argument.  Can't you first find out what PARM_DECLs you need to change,
> stick the corresponding VAR_DECL somewhere (dunno, e.g. a vector with pairs
> perhaps sorted by DECL_UID, or stick it into DECL_VALUE_EXPR or whatever),
> then if you create at least one, walk whole IL and replace all the
> PARM_DECLs you want to replace, then finally clear the TREE_ADDRESSABLE
> flag for all of them and emit the initialization sequence?

Yes, this is doable. I've done that.

> Then something needs to be done for debugging too.  If it is without VTA,
> then probably just having DECL_VALUE_EXPR is good enough, otherwise
> (VTA) you probably don't want that (or can reset it at that point), but
> instead emit after the initialization stmt a debug stmt that the variable
> value now lives in a different var.  Though ideally we want the debugger
> to be able to also change the value of the var, that might be harder.
> With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
> the prologue until it is assigned to the slot.

Here I'm not sure about how to distinguish whether to build or not to build
the debug statement. According to flag_var_tracking?

You mean something like:
g = gimple_build_debug_bind (arg, var, g);
?

> 
>> +
>> +	  gimple_add_tmp_var (var);
>> +
>> +	  if (dump_file)
>> +	    fprintf (dump_file,
>> +		     "Rewritting parameter whos address is taken: %s\n",
>> +		     IDENTIFIER_POINTER (DECL_NAME (arg)));
> 
> s/tting/ting/, s/whos/whose/ 
>> +
>> +	  gimple_seq stmts = NULL;
>> +
>> +	  /* Assign value of parameter to newly created variable.  */
>> +	  if ((TREE_CODE (type) == COMPLEX_TYPE
>> +	       || TREE_CODE (type) == VECTOR_TYPE))
>> +	    {
>> +	      /* We need to create a SSA name that will be used for the
>> +		 assignment.  */
>> +	      tree tmp = make_ssa_name (type);
>> +	      gimple *g = gimple_build_assign (tmp, arg);
>> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> +	      gimple_seq_add_stmt (&stmts, g);
>> +	      g = gimple_build_assign (var, tmp);
>> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> +	      gimple_seq_add_stmt (&stmts, g);
>> +	    }
>> +	  else
>> +	    {
>> +	      gimple *g = gimple_build_assign (var, arg);
>> +	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> +	      gimple_seq_add_stmt (&stmts, g);
>> +	    }
> 
> I don't understand the distinction.  If you turn the original parm
> for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
> (but I think it would be better to use the default SSA_NAME of the PARM_DECL
> if it is a gimple reg type, rather than use the PARM_DECL itself
> and wait for update_ssa).

Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me
as one needs to have a temporary SSA name, otherwise:

/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: error: invalid rhs for gimple memory store
 foo (v4si arg)
 ^~~
arg

arg

# .MEM_4 = VDEF <.MEM_1(D)>
arg = arg;
during GIMPLE pass: sanopt

If I see correctly the function in my test-case does not have default def SSA name for the parameter.
Thus I guess I need to create a SSA name?

Thanks,
Martin



> 
> 	Jakub
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-20  9:23   ` Martin Liška
@ 2017-06-20  9:32     ` Jakub Jelinek
  2017-06-20 13:07       ` Martin Liška
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2017-06-20  9:32 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote:
> > Then something needs to be done for debugging too.  If it is without VTA,
> > then probably just having DECL_VALUE_EXPR is good enough, otherwise
> > (VTA) you probably don't want that (or can reset it at that point), but
> > instead emit after the initialization stmt a debug stmt that the variable
> > value now lives in a different var.  Though ideally we want the debugger
> > to be able to also change the value of the var, that might be harder.
> > With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
> > the prologue until it is assigned to the slot.
> 
> Here I'm not sure about how to distinguish whether to build or not to build
> the debug statement. According to flag_var_tracking?

More like if (target_for_debug_bind (arg))
And if that is false, just make sure DECL_VALUE_EXPR is set to var.

> You mean something like:
> g = gimple_build_debug_bind (arg, var, g);
> ?

Well, there is no stmt, so the last argument would be just NULL.

> > I don't understand the distinction.  If you turn the original parm
> > for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
> > (but I think it would be better to use the default SSA_NAME of the PARM_DECL
> > if it is a gimple reg type, rather than use the PARM_DECL itself
> > and wait for update_ssa).
> 
> Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me
> as one needs to have a temporary SSA name, otherwise:
> 
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: error: invalid rhs for gimple memory store
>  foo (v4si arg)
>  ^~~
> arg
> 
> arg
> 
> # .MEM_4 = VDEF <.MEM_1(D)>
> arg = arg;
> during GIMPLE pass: sanopt
> 
> If I see correctly the function in my test-case does not have default def SSA name for the parameter.
> Thus I guess I need to create a SSA name?

I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and
use the default def, you shouldn't run into this.

	Jakub

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-20  9:32     ` Jakub Jelinek
@ 2017-06-20 13:07       ` Martin Liška
  2017-06-28 13:16         ` Martin Liška
  2017-06-29 11:17         ` Jakub Jelinek
  0 siblings, 2 replies; 13+ messages in thread
From: Martin Liška @ 2017-06-20 13:07 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

On 06/20/2017 11:32 AM, Jakub Jelinek wrote:
> On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote:
>>> Then something needs to be done for debugging too.  If it is without VTA,
>>> then probably just having DECL_VALUE_EXPR is good enough, otherwise
>>> (VTA) you probably don't want that (or can reset it at that point), but
>>> instead emit after the initialization stmt a debug stmt that the variable
>>> value now lives in a different var.  Though ideally we want the debugger
>>> to be able to also change the value of the var, that might be harder.
>>> With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
>>> the prologue until it is assigned to the slot.
>>
>> Here I'm not sure about how to distinguish whether to build or not to build
>> the debug statement. According to flag_var_tracking?
> 
> More like if (target_for_debug_bind (arg))
> And if that is false, just make sure DECL_VALUE_EXPR is set to var.
> 
>> You mean something like:
>> g = gimple_build_debug_bind (arg, var, g);
>> ?
> 
> Well, there is no stmt, so the last argument would be just NULL.
> 
>>> I don't understand the distinction.  If you turn the original parm
>>> for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
>>> (but I think it would be better to use the default SSA_NAME of the PARM_DECL
>>> if it is a gimple reg type, rather than use the PARM_DECL itself
>>> and wait for update_ssa).
>>
>> Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me
>> as one needs to have a temporary SSA name, otherwise:
>>
>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: error: invalid rhs for gimple memory store
>>  foo (v4si arg)
>>  ^~~
>> arg
>>
>> arg
>>
>> # .MEM_4 = VDEF <.MEM_1(D)>
>> arg = arg;
>> during GIMPLE pass: sanopt
>>
>> If I see correctly the function in my test-case does not have default def SSA name for the parameter.
>> Thus I guess I need to create a SSA name?
> 
> I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and
> use the default def, you shouldn't run into this.
> 
> 	Jakub
> 

Good I fixed that in v2, that passes regression tests.
Ale objections should be resolved in the version.

Ready for trunk?
Martin

[-- Attachment #2: 0001-ASAN-handle-addressable-params-PR-sanitize-81040-v2.patch --]
[-- Type: text/x-patch, Size: 8256 bytes --]

From ed5da705250c3015e964de8d23d1aa3d0056012a Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 14 Jun 2017 11:40:01 +0200
Subject: [PATCH] ASAN: handle addressable params (PR sanitize/81040).

gcc/testsuite/ChangeLog:

2017-06-19  Martin Liska  <mliska@suse.cz>

	PR sanitize/81040
	* g++.dg/asan/function-argument-1.C: New test.
	* g++.dg/asan/function-argument-2.C: New test.
	* g++.dg/asan/function-argument-3.C: New test.

gcc/ChangeLog:

2017-06-19  Martin Liska  <mliska@suse.cz>

	PR sanitize/81040
	* sanopt.c (rewrite_usage_of_param): New function.
	(sanitize_rewrite_addressable_params): Likewise.
	(pass_sanopt::execute): Call rewrite_usage_of_param.
---
 gcc/sanopt.c                                    | 132 ++++++++++++++++++++++++
 gcc/testsuite/g++.dg/asan/function-argument-1.C |  30 ++++++
 gcc/testsuite/g++.dg/asan/function-argument-2.C |  24 +++++
 gcc/testsuite/g++.dg/asan/function-argument-3.C |  27 +++++
 4 files changed, 213 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-1.C
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-2.C
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-3.C

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 16bdba76042..077811b5b93 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -37,6 +37,12 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-ssa.h"
 #include "tree-phinodes.h"
 #include "ssa-iterators.h"
+#include "gimplify.h"
+#include "gimple-iterator.h"
+#include "gimple-walk.h"
+#include "cfghooks.h"
+#include "tree-dfa.h"
+#include "tree-ssa.h"
 
 /* This is used to carry information about basic blocks.  It is
    attached to the AUX field of the standard CFG block.  */
@@ -858,6 +864,129 @@ sanitize_asan_mark_poison (void)
     }
 }
 
+/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
+   that is it's DECL_VALUE_EXPR.  */
+
+static tree
+rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
+{
+  if (TREE_CODE (*op) == PARM_DECL && DECL_VALUE_EXPR (*op) != NULL_TREE)
+    {
+      *op = DECL_VALUE_EXPR (*op);
+      *walk_subtrees = 0;
+    }
+
+  return NULL;
+}
+
+/* For a given function FUN, rewrite all addressable parameters so that
+   a new automatic variable is introduced.  Right after function entry
+   a parameter is assigned to the variable.  */
+
+static void
+sanitize_rewrite_addressable_params (function *fun)
+{
+  gimple *g;
+  gimple_seq stmts = NULL;
+  auto_vec<tree> addressable_params;
+
+  for (tree arg = DECL_ARGUMENTS (current_function_decl);
+       arg; arg = DECL_CHAIN (arg))
+    {
+      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
+	{
+	  TREE_ADDRESSABLE (arg) = 0;
+	  /* The parameter is no longer addressable.  */
+	  tree type = TREE_TYPE (arg);
+	  addressable_params.safe_push (arg);
+
+	  /* Create a new automatic variable.  */
+	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
+				 VAR_DECL, DECL_NAME (arg), type);
+	  TREE_ADDRESSABLE (var) = 1;
+	  DECL_ARTIFICIAL (var) = 1;
+	  DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
+
+	  gimple_add_tmp_var (var);
+
+	  if (dump_file)
+	    fprintf (dump_file,
+		     "Rewriting parameter whose address is taken: %s\n",
+		     IDENTIFIER_POINTER (DECL_NAME (arg)));
+
+	  SET_DECL_VALUE_EXPR (arg, var);
+
+	  /* Assign value of parameter to newly created variable.  */
+	  if ((TREE_CODE (type) == COMPLEX_TYPE
+	       || TREE_CODE (type) == VECTOR_TYPE))
+	    {
+	      /* We need to create a SSA name that will be used for the
+		 assignment.  */
+	      tree tmp;
+	      if (DECL_GIMPLE_REG_P (arg))
+		tmp = ssa_default_def (cfun, arg);
+	      else
+		{
+		  tmp = make_ssa_name (type);
+		  g = gimple_build_assign (tmp, arg);
+		  gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+		  gimple_seq_add_stmt (&stmts, g);
+		}
+	      g = gimple_build_assign (var, tmp);
+	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+	      gimple_seq_add_stmt (&stmts, g);
+	    }
+	  else
+	    {
+	      dump_function (0, cfun->decl);
+	      g = gimple_build_assign (var, arg);
+	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+	      gimple_seq_add_stmt (&stmts, g);
+	    }
+
+	  if (target_for_debug_bind (arg))
+	    {
+	      g = gimple_build_debug_bind (arg, var, NULL);
+	      gimple_seq_add_stmt (&stmts, g);
+	    }
+	}
+    }
+
+  if (addressable_params.is_empty ())
+    return;
+
+  /* Replace all usages of PARM_DECLs with the newly
+     created variable VAR.  */
+  basic_block bb;
+  FOR_EACH_BB_FN (bb, fun)
+    {
+      gimple_stmt_iterator gsi;
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  gimple_stmt_iterator it = gsi_for_stmt (stmt);
+	  walk_gimple_stmt (&it, NULL, rewrite_usage_of_param, NULL);
+	}
+      for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	{
+	  gphi *phi = dyn_cast<gphi *> (gsi_stmt (gsi));
+	  for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
+	    {
+	      hash_set<tree> visited_nodes;
+	      walk_tree (gimple_phi_arg_def_ptr (phi, i),
+			 rewrite_usage_of_param, NULL, &visited_nodes);
+	    }
+	}
+    }
+
+  /* Insert default assignments at the beginning of a function.  */
+  basic_block entry_bb = ENTRY_BLOCK_PTR_FOR_FN (fun);
+  entry_bb = split_edge (single_succ_edge (entry_bb));
+
+  gimple_stmt_iterator gsi = gsi_start_bb (entry_bb);
+  gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
+}
+
 unsigned int
 pass_sanopt::execute (function *fun)
 {
@@ -891,6 +1020,9 @@ pass_sanopt::execute (function *fun)
       sanitize_asan_mark_poison ();
     }
 
+  if (asan_sanitize_stack_p ())
+    sanitize_rewrite_addressable_params (fun);
+
   bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX
     && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
 
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-1.C b/gcc/testsuite/g++.dg/asan/function-argument-1.C
new file mode 100644
index 00000000000..148c4628316
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-1.C
@@ -0,0 +1,30 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+struct A
+{
+  int a[5];
+};
+
+static __attribute__ ((noinline)) int
+goo (A *a)
+{
+  int *ptr = &a->a[0];
+  return *(volatile int *) (ptr - 1);
+}
+
+__attribute__ ((noinline)) int
+foo (A arg)
+{
+  return goo (&arg);
+}
+
+int
+main ()
+{
+  return foo (A ());
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-underflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* underflows this variable.*" }
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-2.C b/gcc/testsuite/g++.dg/asan/function-argument-2.C
new file mode 100644
index 00000000000..3a7c33bdaaa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-2.C
@@ -0,0 +1,24 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+static __attribute__ ((noinline)) int
+goo (int *a)
+{
+  return *(volatile int *)a;
+}
+
+__attribute__ ((noinline)) int
+foo (char arg)
+{
+  return goo ((int *)&arg);
+}
+
+int
+main ()
+{
+  return foo (12);
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* partially overflows this variable.*" }
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-3.C b/gcc/testsuite/g++.dg/asan/function-argument-3.C
new file mode 100644
index 00000000000..14617ba8425
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-3.C
@@ -0,0 +1,27 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+typedef int v4si __attribute__ ((vector_size (16)));
+
+static __attribute__ ((noinline)) int
+goo (v4si *a)
+{
+  return (*(volatile v4si *) (a + 1))[2];
+}
+
+__attribute__ ((noinline)) int
+foo (v4si arg)
+{
+  return goo (&arg);
+}
+
+int
+main ()
+{
+  v4si v = {1,2,3,4};
+  return foo (v);
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* overflows this variable.*" }
-- 
2.13.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-20 13:07       ` Martin Liška
@ 2017-06-28 13:16         ` Martin Liška
  2017-06-29 11:17         ` Jakub Jelinek
  1 sibling, 0 replies; 13+ messages in thread
From: Martin Liška @ 2017-06-28 13:16 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC Patches

PING^1

On 06/20/2017 03:06 PM, Martin Liška wrote:
> On 06/20/2017 11:32 AM, Jakub Jelinek wrote:
>> On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote:
>>>> Then something needs to be done for debugging too.  If it is without VTA,
>>>> then probably just having DECL_VALUE_EXPR is good enough, otherwise
>>>> (VTA) you probably don't want that (or can reset it at that point), but
>>>> instead emit after the initialization stmt a debug stmt that the variable
>>>> value now lives in a different var.  Though ideally we want the debugger
>>>> to be able to also change the value of the var, that might be harder.
>>>> With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
>>>> the prologue until it is assigned to the slot.
>>>
>>> Here I'm not sure about how to distinguish whether to build or not to build
>>> the debug statement. According to flag_var_tracking?
>>
>> More like if (target_for_debug_bind (arg))
>> And if that is false, just make sure DECL_VALUE_EXPR is set to var.
>>
>>> You mean something like:
>>> g = gimple_build_debug_bind (arg, var, g);
>>> ?
>>
>> Well, there is no stmt, so the last argument would be just NULL.
>>
>>>> I don't understand the distinction.  If you turn the original parm
>>>> for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
>>>> (but I think it would be better to use the default SSA_NAME of the PARM_DECL
>>>> if it is a gimple reg type, rather than use the PARM_DECL itself
>>>> and wait for update_ssa).
>>>
>>> Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me
>>> as one needs to have a temporary SSA name, otherwise:
>>>
>>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: error: invalid rhs for gimple memory store
>>>  foo (v4si arg)
>>>  ^~~
>>> arg
>>>
>>> arg
>>>
>>> # .MEM_4 = VDEF <.MEM_1(D)>
>>> arg = arg;
>>> during GIMPLE pass: sanopt
>>>
>>> If I see correctly the function in my test-case does not have default def SSA name for the parameter.
>>> Thus I guess I need to create a SSA name?
>>
>> I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and
>> use the default def, you shouldn't run into this.
>>
>> 	Jakub
>>
> 
> Good I fixed that in v2, that passes regression tests.
> Ale objections should be resolved in the version.
> 
> Ready for trunk?
> Martin
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-20 13:07       ` Martin Liška
  2017-06-28 13:16         ` Martin Liška
@ 2017-06-29 11:17         ` Jakub Jelinek
  2017-06-30  9:21           ` Martin Liška
  1 sibling, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2017-06-29 11:17 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Tue, Jun 20, 2017 at 03:06:56PM +0200, Martin Liška wrote:
> +/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
> +   that is it's DECL_VALUE_EXPR.  */
> +
> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
> +{
> +  if (TREE_CODE (*op) == PARM_DECL && DECL_VALUE_EXPR (*op) != NULL_TREE)

DECL_VALUE_EXPR testing is costly (it is a hash table lookup).
Therefore you should test DECL_HAS_VALUE_EXPR_P (*op) after checking
== PARM_DECL.  And DECL_HAS_VALUE_EXPR_P should apply non-NULL
DECL_VALUE_EXPR.
That said, I wonder if we don't create DECL_VALUE_EXPR for PARM_DECLs in
other parts of the compiler, whether it wouldn't be safer to also test here
after == PARM_DECL and DECL_HAS_VALUE_EXPR_P check whether *op is in
addressable_params hash table.

> +    {
> +      *op = DECL_VALUE_EXPR (*op);
> +      *walk_subtrees = 0;
> +    }
> +
> +  return NULL;
> +}
> +
> +/* For a given function FUN, rewrite all addressable parameters so that
> +   a new automatic variable is introduced.  Right after function entry
> +   a parameter is assigned to the variable.  */
> +
> +static void
> +sanitize_rewrite_addressable_params (function *fun)
> +{
> +  gimple *g;
> +  gimple_seq stmts = NULL;
> +  auto_vec<tree> addressable_params;

You don't really use the addressable_params vector anywhere, right?
Except for:

> +
> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
> +       arg; arg = DECL_CHAIN (arg))
> +    {
> +      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> +	{
> +	  TREE_ADDRESSABLE (arg) = 0;
> +	  /* The parameter is no longer addressable.  */
> +	  tree type = TREE_TYPE (arg);
> +	  addressable_params.safe_push (arg);

pushing stuff into it and later

> +  if (addressable_params.is_empty ())
> +    return;

If you only need that, a bool flag if any params have been changed is
enough.  But see above whether it wouldn't be safer to use a hash table
to verify it.  Plus, I think it would be desirable to clear
DECL_HAS_VALUE_EXPR_P and SET_DECL_VALUE_EXPR to NULL afterwards
if (target_for_debug_bind (arg)) - whch can be done either the with vec
or with a hash table traversal, for that we don't care about the ordering.

> +
> +	  /* Create a new automatic variable.  */
> +	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
> +				 VAR_DECL, DECL_NAME (arg), type);
> +	  TREE_ADDRESSABLE (var) = 1;
> +	  DECL_ARTIFICIAL (var) = 1;
> +	  DECL_SEEN_IN_BIND_EXPR_P (var) = 0;

This is 0 already from build_decl, IMHO no need to set it.

> +	  gimple_add_tmp_var (var);
> +
> +	  if (dump_file)
> +	    fprintf (dump_file,
> +		     "Rewriting parameter whose address is taken: %s\n",
> +		     IDENTIFIER_POINTER (DECL_NAME (arg)));
> +
> +	  SET_DECL_VALUE_EXPR (arg, var);

But obviously you miss setting DECL_HAS_VALUE_EXPR_P here.

> +	  /* Assign value of parameter to newly created variable.  */
> +	  if ((TREE_CODE (type) == COMPLEX_TYPE
> +	       || TREE_CODE (type) == VECTOR_TYPE))
> +	    {
> +	      /* We need to create a SSA name that will be used for the
> +		 assignment.  */

Why don't you just set DECL_GIMPLE_REG_P (arg) = 1; for
COMPLEX_TYPE/VECTOR_TYPE?  The arg is going to be only used to copy it into
the new var.  And then just use get_or_create_ssa_default_def,
regardless of whether if is complex/vector or other.

> +  /* Replace all usages of PARM_DECLs with the newly
> +     created variable VAR.  */
> +  basic_block bb;
> +  FOR_EACH_BB_FN (bb, fun)
> +    {
> +      gimple_stmt_iterator gsi;
> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +	{
> +	  gimple *stmt = gsi_stmt (gsi);
> +	  gimple_stmt_iterator it = gsi_for_stmt (stmt);
> +	  walk_gimple_stmt (&it, NULL, rewrite_usage_of_param, NULL);
> +	}
> +      for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +	{
> +	  gphi *phi = dyn_cast<gphi *> (gsi_stmt (gsi));
> +	  for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
> +	    {
> +	      hash_set<tree> visited_nodes;
> +	      walk_tree (gimple_phi_arg_def_ptr (phi, i),
> +			 rewrite_usage_of_param, NULL, &visited_nodes);
> +	    }

Doesn't walk_gimple_stmt on the PHI handle this?

	Jakub

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-29 11:17         ` Jakub Jelinek
@ 2017-06-30  9:21           ` Martin Liška
  2017-06-30  9:31             ` Jakub Jelinek
  2017-07-04  8:49             ` [PATCH] ASAN: handle addressable params (PR sanitize/81040) Jakub Jelinek
  0 siblings, 2 replies; 13+ messages in thread
From: Martin Liška @ 2017-06-30  9:21 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 6088 bytes --]

On 06/29/2017 01:17 PM, Jakub Jelinek wrote:
> On Tue, Jun 20, 2017 at 03:06:56PM +0200, Martin Liška wrote:
>> +/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
>> +   that is it's DECL_VALUE_EXPR.  */
>> +
>> +static tree
>> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
>> +{
>> +  if (TREE_CODE (*op) == PARM_DECL && DECL_VALUE_EXPR (*op) != NULL_TREE)
> 
> DECL_VALUE_EXPR testing is costly (it is a hash table lookup).
> Therefore you should test DECL_HAS_VALUE_EXPR_P (*op) after checking
> == PARM_DECL.  And DECL_HAS_VALUE_EXPR_P should apply non-NULL
> DECL_VALUE_EXPR.
> That said, I wonder if we don't create DECL_VALUE_EXPR for PARM_DECLs in
> other parts of the compiler, whether it wouldn't be safer to also test here
> after == PARM_DECL and DECL_HAS_VALUE_EXPR_P check whether *op is in
> addressable_params hash table.

Thanks for explanation, DECL_HAS_VALUE_EXPR_P is a flag, while DECL_VALUE_EXPR
is a hash table lookup.

> 
>> +    {
>> +      *op = DECL_VALUE_EXPR (*op);
>> +      *walk_subtrees = 0;
>> +    }
>> +
>> +  return NULL;
>> +}
>> +
>> +/* For a given function FUN, rewrite all addressable parameters so that
>> +   a new automatic variable is introduced.  Right after function entry
>> +   a parameter is assigned to the variable.  */
>> +
>> +static void
>> +sanitize_rewrite_addressable_params (function *fun)
>> +{
>> +  gimple *g;
>> +  gimple_seq stmts = NULL;
>> +  auto_vec<tree> addressable_params;
> 
> You don't really use the addressable_params vector anywhere, right?
> Except for:
> 
>> +
>> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
>> +       arg; arg = DECL_CHAIN (arg))
>> +    {
>> +      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
>> +	{
>> +	  TREE_ADDRESSABLE (arg) = 0;
>> +	  /* The parameter is no longer addressable.  */
>> +	  tree type = TREE_TYPE (arg);
>> +	  addressable_params.safe_push (arg);
> 
> pushing stuff into it and later
> 
>> +  if (addressable_params.is_empty ())
>> +    return;
> 
> If you only need that, a bool flag if any params have been changed is
> enough.  But see above whether it wouldn't be safer to use a hash table
> to verify it.  Plus, I think it would be desirable to clear
> DECL_HAS_VALUE_EXPR_P and SET_DECL_VALUE_EXPR to NULL afterwards
> if (target_for_debug_bind (arg)) - whch can be done either the with vec
> or with a hash table traversal, for that we don't care about the ordering.

Good point, I decided to come up with a flag + vector of arguments where
VALUE_EXPR should be set to NULL.

> 
>> +
>> +	  /* Create a new automatic variable.  */
>> +	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
>> +				 VAR_DECL, DECL_NAME (arg), type);
>> +	  TREE_ADDRESSABLE (var) = 1;
>> +	  DECL_ARTIFICIAL (var) = 1;
>> +	  DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
> 
> This is 0 already from build_decl, IMHO no need to set it.

Done.

> 
>> +	  gimple_add_tmp_var (var);
>> +
>> +	  if (dump_file)
>> +	    fprintf (dump_file,
>> +		     "Rewriting parameter whose address is taken: %s\n",
>> +		     IDENTIFIER_POINTER (DECL_NAME (arg)));
>> +
>> +	  SET_DECL_VALUE_EXPR (arg, var);
> 
> But obviously you miss setting DECL_HAS_VALUE_EXPR_P here.

Likewise.

> 
>> +	  /* Assign value of parameter to newly created variable.  */
>> +	  if ((TREE_CODE (type) == COMPLEX_TYPE
>> +	       || TREE_CODE (type) == VECTOR_TYPE))
>> +	    {
>> +	      /* We need to create a SSA name that will be used for the
>> +		 assignment.  */
> 
> Why don't you just set DECL_GIMPLE_REG_P (arg) = 1; for
> COMPLEX_TYPE/VECTOR_TYPE?  The arg is going to be only used to copy it into
> the new var.  And then just use get_or_create_ssa_default_def,
> regardless of whether if is complex/vector or other.

Doing so fails here:

$ ./xgcc -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-1.C -fsanitize=address 
during RTL pass: expand
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-1.C: In function ‘int foo(A)’:
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-1.C:17:1: internal compiler error: in set_parm_rtl, at cfgexpand.c:1271
 foo (A arg)
 ^~~
0x938536 set_parm_rtl(tree_node*, rtx_def*)
	../../gcc/cfgexpand.c:1271
0xab9813 assign_parms
	../../gcc/function.c:3782
0xabc605 expand_function_start(tree_node*)
	../../gcc/function.c:5221
0x943e01 execute
	../../gcc/cfgexpand.c:6248

> 
>> +  /* Replace all usages of PARM_DECLs with the newly
>> +     created variable VAR.  */
>> +  basic_block bb;
>> +  FOR_EACH_BB_FN (bb, fun)
>> +    {
>> +      gimple_stmt_iterator gsi;
>> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> +	{
>> +	  gimple *stmt = gsi_stmt (gsi);
>> +	  gimple_stmt_iterator it = gsi_for_stmt (stmt);
>> +	  walk_gimple_stmt (&it, NULL, rewrite_usage_of_param, NULL);
>> +	}
>> +      for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> +	{
>> +	  gphi *phi = dyn_cast<gphi *> (gsi_stmt (gsi));
>> +	  for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
>> +	    {
>> +	      hash_set<tree> visited_nodes;
>> +	      walk_tree (gimple_phi_arg_def_ptr (phi, i),
>> +			 rewrite_usage_of_param, NULL, &visited_nodes);
>> +	    }
> 
> Doesn't walk_gimple_stmt on the PHI handle this?

No, I see following in asan bootstrap:

^[2^[1^[2^[2../../gcc/c/c-decl.c: In function ‘tree_node* grokfield(location_t, c_declarator*, c_declspecs*, tree, tree_node**)’:
../../gcc/c/c-decl.c:7525:1: error: address taken, but ADDRESSABLE bit not set
 grokfield (location_t loc,
 ^~~~~~~~~
PHI argument
&width;
for PHI node
iftmp.2774_27 = PHI <&width(16), 0B(61), &width(54)>
during GIMPLE pass: sanopt
../../gcc/c/c-decl.c:7525:1: internal compiler error: verify_ssa failed
0x115356db verify_ssa(bool, bool)
	../../gcc/tree-ssa.c:1186
0x10f850ff execute_function_todo
	../../gcc/passes.c:1996
0x10f8376b do_per_function
	../../gcc/passes.c:1655
0x10f853a7 execute_todo
	../../gcc/passes.c:2043

Sending new version that I'm going to test.

Martin

> 
> 	Jakub
> 


[-- Attachment #2: 0001-ASAN-handle-addressable-params-PR-sanitize-81040.patch --]
[-- Type: text/x-patch, Size: 8388 bytes --]

From bd7580c259c0552490d63527782d00926c9c5473 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 14 Jun 2017 11:40:01 +0200
Subject: [PATCH] ASAN: handle addressable params (PR sanitize/81040).

gcc/testsuite/ChangeLog:

2017-06-19  Martin Liska  <mliska@suse.cz>

	PR sanitize/81040
	* g++.dg/asan/function-argument-1.C: New test.
	* g++.dg/asan/function-argument-2.C: New test.
	* g++.dg/asan/function-argument-3.C: New test.

gcc/ChangeLog:

2017-06-19  Martin Liska  <mliska@suse.cz>

	PR sanitize/81040
	* sanopt.c (rewrite_usage_of_param): New function.
	(sanitize_rewrite_addressable_params): Likewise.
	(pass_sanopt::execute): Call rewrite_usage_of_param.
---
 gcc/sanopt.c                                    | 135 ++++++++++++++++++++++++
 gcc/testsuite/g++.dg/asan/function-argument-1.C |  30 ++++++
 gcc/testsuite/g++.dg/asan/function-argument-2.C |  24 +++++
 gcc/testsuite/g++.dg/asan/function-argument-3.C |  27 +++++
 4 files changed, 216 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-1.C
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-2.C
 create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-3.C

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 16bdba76042..acb09dddd5b 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -37,6 +37,12 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-ssa.h"
 #include "tree-phinodes.h"
 #include "ssa-iterators.h"
+#include "gimplify.h"
+#include "gimple-iterator.h"
+#include "gimple-walk.h"
+#include "cfghooks.h"
+#include "tree-dfa.h"
+#include "tree-ssa.h"
 
 /* This is used to carry information about basic blocks.  It is
    attached to the AUX field of the standard CFG block.  */
@@ -858,6 +864,132 @@ sanitize_asan_mark_poison (void)
     }
 }
 
+/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
+   that is it's DECL_VALUE_EXPR.  */
+
+static tree
+rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
+{
+  if (TREE_CODE (*op) == PARM_DECL && DECL_HAS_VALUE_EXPR_P (*op))
+    {
+      *op = DECL_VALUE_EXPR (*op);
+      *walk_subtrees = 0;
+    }
+
+  return NULL;
+}
+
+/* For a given function FUN, rewrite all addressable parameters so that
+   a new automatic variable is introduced.  Right after function entry
+   a parameter is assigned to the variable.  */
+
+static void
+sanitize_rewrite_addressable_params (function *fun)
+{
+  gimple *g;
+  gimple_seq stmts = NULL;
+  bool has_any_addressable_param = false;
+  auto_vec<tree> clear_value_expr_list;
+
+  for (tree arg = DECL_ARGUMENTS (current_function_decl);
+       arg; arg = DECL_CHAIN (arg))
+    {
+      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
+	{
+	  TREE_ADDRESSABLE (arg) = 0;
+	  /* The parameter is no longer addressable.  */
+	  tree type = TREE_TYPE (arg);
+	  has_any_addressable_param = true;
+
+	  /* Create a new automatic variable.  */
+	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
+				 VAR_DECL, DECL_NAME (arg), type);
+	  TREE_ADDRESSABLE (var) = 1;
+	  DECL_ARTIFICIAL (var) = 1;
+
+	  gimple_add_tmp_var (var);
+
+	  if (dump_file)
+	    fprintf (dump_file,
+		     "Rewriting parameter whose address is taken: %s\n",
+		     IDENTIFIER_POINTER (DECL_NAME (arg)));
+
+	  DECL_HAS_VALUE_EXPR_P (arg) = 1;
+	  SET_DECL_VALUE_EXPR (arg, var);
+
+	  /* Assign value of parameter to newly created variable.  */
+	  if ((TREE_CODE (type) == COMPLEX_TYPE
+	       || TREE_CODE (type) == VECTOR_TYPE))
+	    {
+	      /* We need to create a SSA name that will be used for the
+		 assignment.  */
+	      DECL_GIMPLE_REG_P (arg) = 1;
+	      tree tmp = get_or_create_ssa_default_def (cfun, arg);
+	      g = gimple_build_assign (var, tmp);
+	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+	      gimple_seq_add_stmt (&stmts, g);
+	    }
+	  else
+	    {
+	      g = gimple_build_assign (var, arg);
+	      gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+	      gimple_seq_add_stmt (&stmts, g);
+	    }
+
+	  if (target_for_debug_bind (arg))
+	    {
+	      g = gimple_build_debug_bind (arg, var, NULL);
+	      gimple_seq_add_stmt (&stmts, g);
+	      clear_value_expr_list.safe_push (arg);
+	    }
+	}
+    }
+
+  if (!has_any_addressable_param)
+    return;
+
+  /* Replace all usages of PARM_DECLs with the newly
+     created variable VAR.  */
+  basic_block bb;
+  FOR_EACH_BB_FN (bb, fun)
+    {
+      gimple_stmt_iterator gsi;
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  gimple_stmt_iterator it = gsi_for_stmt (stmt);
+	  walk_gimple_stmt (&it, NULL, rewrite_usage_of_param, NULL);
+	}
+      for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	{
+	  gphi *phi = dyn_cast<gphi *> (gsi_stmt (gsi));
+          for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
+	    {
+	      hash_set<tree> visited_nodes;
+	      walk_tree (gimple_phi_arg_def_ptr (phi, i),
+			 rewrite_usage_of_param, NULL, &visited_nodes);
+	    }
+	}
+    }
+
+  /* Unset value expr for parameters for which we created debug bind
+     expressions.  */
+  unsigned i;
+  tree arg;
+  FOR_EACH_VEC_ELT (clear_value_expr_list, i, arg)
+    {
+      DECL_HAS_VALUE_EXPR_P (arg) = 0;
+      SET_DECL_VALUE_EXPR (arg, NULL_TREE);
+    }
+
+  /* Insert default assignments at the beginning of a function.  */
+  basic_block entry_bb = ENTRY_BLOCK_PTR_FOR_FN (fun);
+  entry_bb = split_edge (single_succ_edge (entry_bb));
+
+  gimple_stmt_iterator gsi = gsi_start_bb (entry_bb);
+  gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
+}
+
 unsigned int
 pass_sanopt::execute (function *fun)
 {
@@ -891,6 +1023,9 @@ pass_sanopt::execute (function *fun)
       sanitize_asan_mark_poison ();
     }
 
+  if (asan_sanitize_stack_p ())
+    sanitize_rewrite_addressable_params (fun);
+
   bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX
     && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
 
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-1.C b/gcc/testsuite/g++.dg/asan/function-argument-1.C
new file mode 100644
index 00000000000..148c4628316
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-1.C
@@ -0,0 +1,30 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+struct A
+{
+  int a[5];
+};
+
+static __attribute__ ((noinline)) int
+goo (A *a)
+{
+  int *ptr = &a->a[0];
+  return *(volatile int *) (ptr - 1);
+}
+
+__attribute__ ((noinline)) int
+foo (A arg)
+{
+  return goo (&arg);
+}
+
+int
+main ()
+{
+  return foo (A ());
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-underflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* underflows this variable.*" }
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-2.C b/gcc/testsuite/g++.dg/asan/function-argument-2.C
new file mode 100644
index 00000000000..3a7c33bdaaa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-2.C
@@ -0,0 +1,24 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+static __attribute__ ((noinline)) int
+goo (int *a)
+{
+  return *(volatile int *)a;
+}
+
+__attribute__ ((noinline)) int
+foo (char arg)
+{
+  return goo ((int *)&arg);
+}
+
+int
+main ()
+{
+  return foo (12);
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* partially overflows this variable.*" }
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-3.C b/gcc/testsuite/g++.dg/asan/function-argument-3.C
new file mode 100644
index 00000000000..14617ba8425
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/function-argument-3.C
@@ -0,0 +1,27 @@
+// { dg-do run }
+// { dg-shouldfail "asan" }
+
+typedef int v4si __attribute__ ((vector_size (16)));
+
+static __attribute__ ((noinline)) int
+goo (v4si *a)
+{
+  return (*(volatile v4si *) (a + 1))[2];
+}
+
+__attribute__ ((noinline)) int
+foo (v4si arg)
+{
+  return goo (&arg);
+}
+
+int
+main ()
+{
+  v4si v = {1,2,3,4};
+  return foo (v);
+}
+
+// { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on address.*(\n|\r\n|\r)" }
+// { dg-output "READ of size . at.*" }
+// { dg-output ".*'arg' <== Memory access at offset \[0-9\]* overflows this variable.*" }
-- 
2.13.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-30  9:21           ` Martin Liška
@ 2017-06-30  9:31             ` Jakub Jelinek
  2017-07-04  7:47               ` [PATCH] Enable addressable params sanitization with --param asan-stack=1 Martin Liška
  2017-07-04  8:49             ` [PATCH] ASAN: handle addressable params (PR sanitize/81040) Jakub Jelinek
  1 sibling, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2017-06-30  9:31 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Fri, Jun 30, 2017 at 11:21:48AM +0200, Martin Liška wrote:
> @@ -858,6 +864,132 @@ sanitize_asan_mark_poison (void)
>      }
>  }
>  
> +/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
> +   that is it's DECL_VALUE_EXPR.  */
> +
> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
> +{
> +  if (TREE_CODE (*op) == PARM_DECL && DECL_HAS_VALUE_EXPR_P (*op))
> +    {
> +      *op = DECL_VALUE_EXPR (*op);
> +      *walk_subtrees = 0;
> +    }

If you are going to rely just on DECL_HAS_VALUE_EXPR_P here

> +  for (tree arg = DECL_ARGUMENTS (current_function_decl);
> +       arg; arg = DECL_CHAIN (arg))
> +    {

I think you should gcc_assert here that !DECL_HAS_VALUE_EXPR_P (arg) here.

If that ever fails, another possibility would be to temporarily clear those
flags and remember it and then set it again after the walk_*.  The question
would be what to do with arguments that already have DECL_VALUE_EXPR, are
addressable and you want to rewrite them.

> +      if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> +	{
> +	  TREE_ADDRESSABLE (arg) = 0;
> +	  /* The parameter is no longer addressable.  */
> +	  tree type = TREE_TYPE (arg);
> +	  has_any_addressable_param = true;

Otherwise LGTM.

	Jakub

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] Enable addressable params sanitization with --param asan-stack=1.
  2017-06-30  9:31             ` Jakub Jelinek
@ 2017-07-04  7:47               ` Martin Liška
  2017-07-04  8:00                 ` Jakub Jelinek
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Liška @ 2017-07-04  7:47 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

Hello.

As mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81040#c15, the sanitization is
done only when one uses use-after-scope. That's caused by fact that I decorated the newly
created auto variables with DECL_ARTIFICIAL = 1. Because of that

static inline bool
asan_protect_stack_decl (tree decl)
{
  return DECL_P (decl)
    && (!DECL_ARTIFICIAL (decl)
	|| (asan_sanitize_use_after_scope () && TREE_ADDRESSABLE (decl)));
}

returns false. I hope not marking the variable as DECL_ARTIFICIAL will work fine?
Or am I missing something?

Thanks,
Martin


[-- Attachment #2: 0001-Enable-addressable-params-sanitization-with-param-as.patch --]
[-- Type: text/x-patch, Size: 1575 bytes --]

From b79133e3c9ad41b44f0a12c574fc1d0b8348ad89 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 4 Jul 2017 09:22:23 +0200
Subject: [PATCH] Enable addressable params sanitization with --param
 asan-stack=1.

gcc/ChangeLog:

2017-07-04  Martin Liska  <mliska@suse.cz>

	PR sanitizer/81040
	* sanopt.c (sanitize_rewrite_addressable_params): Do not
	decorate variable as DECL_ARTIFICIAL in order to sanitize it.

gcc/testsuite/ChangeLog:

2017-07-04  Martin Liska  <mliska@suse.cz>

	PR sanitizer/81040
	* g++.dg/asan/function-argument-1.C: Run the test-case w/o
	use-after-scope sanitization.
---
 gcc/sanopt.c                                    | 1 -
 gcc/testsuite/g++.dg/asan/function-argument-1.C | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 7692f6a9db7..8c80ff37d4d 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -905,7 +905,6 @@ sanitize_rewrite_addressable_params (function *fun)
 	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
 				 VAR_DECL, DECL_NAME (arg), type);
 	  TREE_ADDRESSABLE (var) = 1;
-	  DECL_ARTIFICIAL (var) = 1;
 
 	  gimple_add_tmp_var (var);
 
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-1.C b/gcc/testsuite/g++.dg/asan/function-argument-1.C
index 148c4628316..bdbb37a44a4 100644
--- a/gcc/testsuite/g++.dg/asan/function-argument-1.C
+++ b/gcc/testsuite/g++.dg/asan/function-argument-1.C
@@ -1,5 +1,6 @@
 // { dg-do run }
 // { dg-shouldfail "asan" }
+// { dg-options "-fsanitize=address -fno-sanitize-address-use-after-scope" }
 
 struct A
 {
-- 
2.13.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Enable addressable params sanitization with --param asan-stack=1.
  2017-07-04  7:47               ` [PATCH] Enable addressable params sanitization with --param asan-stack=1 Martin Liška
@ 2017-07-04  8:00                 ` Jakub Jelinek
  2017-07-04 10:52                   ` Martin Liška
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2017-07-04  8:00 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Tue, Jul 04, 2017 at 09:47:29AM +0200, Martin Liška wrote:
> As mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81040#c15, the sanitization is
> done only when one uses use-after-scope. That's caused by fact that I decorated the newly
> created auto variables with DECL_ARTIFICIAL = 1. Because of that
> 
> static inline bool
> asan_protect_stack_decl (tree decl)
> {
>   return DECL_P (decl)
>     && (!DECL_ARTIFICIAL (decl)
> 	|| (asan_sanitize_use_after_scope () && TREE_ADDRESSABLE (decl)));
> }
> 
> returns false. I hope not marking the variable as DECL_ARTIFICIAL will work fine?
> Or am I missing something?

Well, you should make sure the debug info is correct.
Which means ideally that there is just one DW_TAG_formal_parameter and no
DW_TAG_variable for the parameter.
For the addressable parameters I hope the corresponding artificial
vars just live in memory for the whole rest of the scope, at least for the
case where you emit a debug bind (hope it is after the assignment to the
artificial var) I think it should be fine to set DECL_IGNORED_P on the
artificial var instead of DECL_ARTIFICIAL.
For the other case where there is DECL_VALUE_EXPR, perhaps try it too and
see what you get.

	Jakub

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
  2017-06-30  9:21           ` Martin Liška
  2017-06-30  9:31             ` Jakub Jelinek
@ 2017-07-04  8:49             ` Jakub Jelinek
  1 sibling, 0 replies; 13+ messages in thread
From: Jakub Jelinek @ 2017-07-04  8:49 UTC (permalink / raw)
  To: Martin Liška; +Cc: GCC Patches

On Fri, Jun 30, 2017 at 11:21:48AM +0200, Martin Liška wrote:
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/asan/function-argument-3.C
> @@ -0,0 +1,27 @@
> +// { dg-do run }
> +// { dg-shouldfail "asan" }
> +
> +typedef int v4si __attribute__ ((vector_size (16)));
> +
> +static __attribute__ ((noinline)) int
> +goo (v4si *a)
> +{
> +  return (*(volatile v4si *) (a + 1))[2];
> +}
> +
> +__attribute__ ((noinline)) int
> +foo (v4si arg)
> +{
> +  return goo (&arg);
> +}

This test fails on i686-linux when -msse2 is not on by default,
fixed thusly, committed as obvious:

2017-07-04  Jakub Jelinek  <jakub@redhat.com>

	* g++.dg/asan/function-argument-3.C: Add -Wno-psabi to additional
	options.

--- gcc/testsuite/g++.dg/asan/function-argument-3.C.jj	2017-07-03 19:03:25.000000000 +0200
+++ gcc/testsuite/g++.dg/asan/function-argument-3.C	2017-07-04 10:42:18.558716171 +0200
@@ -1,5 +1,6 @@
 // { dg-do run }
 // { dg-shouldfail "asan" }
+// { dg-additional-options "-Wno-psabi" }
 
 typedef int v4si __attribute__ ((vector_size (16)));
 


	Jakub

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Enable addressable params sanitization with --param asan-stack=1.
  2017-07-04  8:00                 ` Jakub Jelinek
@ 2017-07-04 10:52                   ` Martin Liška
  0 siblings, 0 replies; 13+ messages in thread
From: Martin Liška @ 2017-07-04 10:52 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: GCC Patches

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

On 07/04/2017 09:59 AM, Jakub Jelinek wrote:
> On Tue, Jul 04, 2017 at 09:47:29AM +0200, Martin Liška wrote:
>> As mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81040#c15, the sanitization is
>> done only when one uses use-after-scope. That's caused by fact that I decorated the newly
>> created auto variables with DECL_ARTIFICIAL = 1. Because of that
>>
>> static inline bool
>> asan_protect_stack_decl (tree decl)
>> {
>>   return DECL_P (decl)
>>     && (!DECL_ARTIFICIAL (decl)
>> 	|| (asan_sanitize_use_after_scope () && TREE_ADDRESSABLE (decl)));
>> }
>>
>> returns false. I hope not marking the variable as DECL_ARTIFICIAL will work fine?
>> Or am I missing something?
> 
> Well, you should make sure the debug info is correct.
> Which means ideally that there is just one DW_TAG_formal_parameter and no
> DW_TAG_variable for the parameter.
> For the addressable parameters I hope the corresponding artificial
> vars just live in memory for the whole rest of the scope, at least for the
> case where you emit a debug bind (hope it is after the assignment to the
> artificial var) I think it should be fine to set DECL_IGNORED_P on the
> artificial var instead of DECL_ARTIFICIAL.
> For the other case where there is DECL_VALUE_EXPR, perhaps try it too and
> see what you get.
> 
> 	Jakub
> 

Using DECL_IGNORED_P works for me.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

I'm going to install the patch.
Martin

[-- Attachment #2: 0001-Enable-addressable-params-sanitization-with-param-as.patch --]
[-- Type: text/x-patch, Size: 1589 bytes --]

From 20d69fbf4076add09df363ffb9d03cd243f8190d Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 4 Jul 2017 09:22:23 +0200
Subject: [PATCH] Enable addressable params sanitization with --param
 asan-stack=1.

gcc/ChangeLog:

2017-07-04  Martin Liska  <mliska@suse.cz>

	PR sanitizer/81040
	* sanopt.c (sanitize_rewrite_addressable_params): Mark the
	newly created variable as DECL_IGNORED_P.

gcc/testsuite/ChangeLog:

2017-07-04  Martin Liska  <mliska@suse.cz>

	PR sanitizer/81040
	* g++.dg/asan/function-argument-1.C: Run the test-case w/o
	use-after-scope sanitization.
---
 gcc/sanopt.c                                    | 2 +-
 gcc/testsuite/g++.dg/asan/function-argument-1.C | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 7692f6a9db7..b7740741d43 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -905,7 +905,7 @@ sanitize_rewrite_addressable_params (function *fun)
 	  tree var = build_decl (DECL_SOURCE_LOCATION (arg),
 				 VAR_DECL, DECL_NAME (arg), type);
 	  TREE_ADDRESSABLE (var) = 1;
-	  DECL_ARTIFICIAL (var) = 1;
+	  DECL_IGNORED_P (var) = 1;
 
 	  gimple_add_tmp_var (var);
 
diff --git a/gcc/testsuite/g++.dg/asan/function-argument-1.C b/gcc/testsuite/g++.dg/asan/function-argument-1.C
index 148c4628316..bdbb37a44a4 100644
--- a/gcc/testsuite/g++.dg/asan/function-argument-1.C
+++ b/gcc/testsuite/g++.dg/asan/function-argument-1.C
@@ -1,5 +1,6 @@
 // { dg-do run }
 // { dg-shouldfail "asan" }
+// { dg-options "-fsanitize=address -fno-sanitize-address-use-after-scope" }
 
 struct A
 {
-- 
2.13.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-07-04 10:52 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-19 13:50 [PATCH] ASAN: handle addressable params (PR sanitize/81040) Martin Liška
2017-06-19 14:13 ` Jakub Jelinek
2017-06-20  9:23   ` Martin Liška
2017-06-20  9:32     ` Jakub Jelinek
2017-06-20 13:07       ` Martin Liška
2017-06-28 13:16         ` Martin Liška
2017-06-29 11:17         ` Jakub Jelinek
2017-06-30  9:21           ` Martin Liška
2017-06-30  9:31             ` Jakub Jelinek
2017-07-04  7:47               ` [PATCH] Enable addressable params sanitization with --param asan-stack=1 Martin Liška
2017-07-04  8:00                 ` Jakub Jelinek
2017-07-04 10:52                   ` Martin Liška
2017-07-04  8:49             ` [PATCH] ASAN: handle addressable params (PR sanitize/81040) Jakub Jelinek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).