diff mbox

[RFC] Introduce -fsanitize=use-after-scope (v2)

Message ID e04b1ac8-e3ff-b13f-4006-bf961af6d2ea@suse.cz
State New
Headers show

Commit Message

Martin Liška Nov. 4, 2016, 9:17 a.m. UTC 
On 11/02/2016 03:35 PM, Jakub Jelinek wrote:
> On Wed, Nov 02, 2016 at 03:27:42PM +0100, Martin Liška wrote:

>>> So is there anything I should do wrt -Wswitch-unreachable?

>>>

>>> 	Marek

>>>

>>

>> Probably not. I'm having a patch puts GIMPLE_SWITCH statement to a proper place

>> in GIMPLE_BIND. Let's see whether such patch can bootstrap and survive regression

>> tests.

> 

> Please do that only for -fsanitize-use-after-scope, it will likely affect at

> least for -O0 the debugging experience.  For -O0 -fsanitize=address -fsanitize-use-after-scope

> perhaps we could arrange for some extra stmt to have the locus of the

> switch (where we still don't want the vars to appear in scope) and then

> have no locus on the ASAN_MARK and actual GIMPLE_SWITCH or something

> similar.

> 

> 	Jakub

> 


I'm sending patch where I put gimple switch statement to a place where all BIND_EXPR vars
are unpoisoned. I'm sending diff to a previous version and new version of the patch.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Apart from that,
asan bootstrap successfully finished on x86_64-linux-gnu.

Martin

Comments

Jakub Jelinek Nov. 4, 2016, 9:32 a.m. UTC | #1 
Hi!

On Fri, Nov 04, 2016 at 10:17:31AM +0100, Martin Liška wrote:
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c

> index 813777d..86ce793 100644

> --- a/gcc/gimplify.c

> +++ b/gcc/gimplify.c

> @@ -1678,7 +1678,9 @@ warn_switch_unreachable_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,

>  	 worse location info.  */

>        if (gimple_try_eval (stmt) == NULL)

>  	{

> -	  wi->info = stmt;

> +	  gimple_stmt_iterator *it = XNEW (gimple_stmt_iterator);

> +	  memcpy (it, gsi_p, sizeof (gimple_stmt_iterator));


That would be cleaner as *it = *gsi_p;
That set, I fail to see
1) the need to use a gsi pointer in wi->info compared to stmt itself,
   you can gsi_for_stmt cheaply at any time
2) why is anything done about this in warn_switch_unreachable_r
   - the problem isn't related to this warning IMHO.  Even
switch (x)
  {
  case 1:
    int x;
    x = 6;
    ptr = &x;
    break;
  case 2:
    ptr = &x;
    *ptr = 7;
    break;
  }
   has the same issue and there is no switch unreachable code there, but you
   still want for -fsanitize-use-after-scope pretend it is actually:
x_tmp = x;
{
  int x;
  switch (x_tmp)
    {
    case 1:
      x = 6;
      ptr = &x;
      break;
    case 2:
      ptr = &x;
      *ptr = 7;
      break;
    }
}
    and put ASAN_MARK unpoisoning before GIMPLE_SWITCH.

	Jakub
Martin Liška Nov. 4, 2016, 10:59 a.m. UTC | #2 
On 11/04/2016 10:32 AM, Jakub Jelinek wrote:
> Hi!

> 

> On Fri, Nov 04, 2016 at 10:17:31AM +0100, Martin Liška wrote:

>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c

>> index 813777d..86ce793 100644

>> --- a/gcc/gimplify.c

>> +++ b/gcc/gimplify.c

>> @@ -1678,7 +1678,9 @@ warn_switch_unreachable_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,

>>  	 worse location info.  */

>>        if (gimple_try_eval (stmt) == NULL)

>>  	{

>> -	  wi->info = stmt;

>> +	  gimple_stmt_iterator *it = XNEW (gimple_stmt_iterator);

>> +	  memcpy (it, gsi_p, sizeof (gimple_stmt_iterator));

> 

> That would be cleaner as *it = *gsi_p;


I know that it's kind of ugly, but as the gimple stmt is not yet added to a BB, using
gsi_for_stmt ICEs:

/tmp/use-after-scope-switch.c:12:5: internal compiler error: Segmentation fault
     switch (argc)
     ^~~~~~
0xe16a14 crash_signal
	../../gcc/toplev.c:338
0xadf890 bb_seq_addr
	../../gcc/gimple.h:1658
0xae01dd gsi_start_bb
	../../gcc/gimple-iterator.h:129
0xae111f gsi_for_stmt(gimple*)
	../../gcc/gimple-iterator.c:617

> That set, I fail to see

> 1) the need to use a gsi pointer in wi->info compared to stmt itself,

>    you can gsi_for_stmt cheaply at any time

> 2) why is anything done about this in warn_switch_unreachable_r

>    - the problem isn't related to this warning IMHO.  Even

> switch (x)

>   {

>   case 1:

>     int x;

>     x = 6;

>     ptr = &x;

>     break;

>   case 2:

>     ptr = &x;

>     *ptr = 7;

>     break;

>   }

>    has the same issue and there is no switch unreachable code there, but you

>    still want for -fsanitize-use-after-scope pretend it is actually:


You're right, that's not handled. I'm wondering whether it's always profitable
because when you do not reach the case 1, you're not doing a poisoning operation?

Martin

> x_tmp = x;

> {

>   int x;

>   switch (x_tmp)

>     {

>     case 1:

>       x = 6;

>       ptr = &x;

>       break;

>     case 2:

>       ptr = &x;

>       *ptr = 7;

>       break;

>     }

> }

>     and put ASAN_MARK unpoisoning before GIMPLE_SWITCH.

> 

> 	Jakub

>
diff mbox

Patch

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 813777d..86ce793 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1678,7 +1678,9 @@  warn_switch_unreachable_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
 	 worse location info.  */
       if (gimple_try_eval (stmt) == NULL)
 	{
-	  wi->info = stmt;
+	  gimple_stmt_iterator *it = XNEW (gimple_stmt_iterator);
+	  memcpy (it, gsi_p, sizeof (gimple_stmt_iterator));
+	  wi->info = it;
 	  return integer_zero_node;
 	}
       /* Fall through.  */
@@ -1689,9 +1691,18 @@  warn_switch_unreachable_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
       /* Walk the sub-statements.  */
       *handled_ops_p = false;
       break;
+    case GIMPLE_CALL:
+      if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+	{
+	  *handled_ops_p = false;
+	  break;
+	}
+      /* Fall through.  */
     default:
       /* Save the first "real" statement (not a decl/lexical scope/...).  */
-      wi->info = stmt;
+      gimple_stmt_iterator *it = XNEW (gimple_stmt_iterator);
+      memcpy (it, gsi_p, sizeof (gimple_stmt_iterator));
+      wi->info = it;
       return integer_zero_node;
     }
   return NULL_TREE;
@@ -1713,7 +1724,11 @@  maybe_warn_switch_unreachable (gimple_seq seq)
   struct walk_stmt_info wi;
   memset (&wi, 0, sizeof (wi));
   walk_gimple_seq (seq, warn_switch_unreachable_r, NULL, &wi);
-  gimple *stmt = (gimple *) wi.info;
+  gimple *stmt = NULL;
+  gimple_stmt_iterator *gsi = (gimple_stmt_iterator *) wi.info;
+  if (gsi)
+    stmt = gsi_stmt (*gsi);
+  free (wi.info);
 
   if (stmt && gimple_code (stmt) != GIMPLE_LABEL)
     {
@@ -1802,6 +1900,8 @@  collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
 	  if (find_label_entry (labels, label))
 	    prev = gsi_stmt (*gsi_p);
 	}
+      else if (gimple_call_internal_p (gsi_stmt (*gsi_p), IFN_ASAN_MARK))
+	;
       else
 	prev = gsi_stmt (*gsi_p);
       gsi_next (gsi_p);

@@ -2224,7 +2239,22 @@  gimplify_switch_expr (tree *expr_p, gimple_seq *pre_p)
 
       switch_stmt = gimple_build_switch (SWITCH_COND (switch_expr),
 					   default_case, labels);
-      gimplify_seq_add_stmt (pre_p, switch_stmt);
+
+      if (asan_sanitize_use_after_scope ())
+	{
+	  struct walk_stmt_info wi;
+	  memset (&wi, 0, sizeof (wi));
+	  walk_gimple_seq (switch_body_seq, warn_switch_unreachable_r, NULL, &wi);
+	  gimple_stmt_iterator *it = (gimple_stmt_iterator *)wi.info;
+	  if (gsi_stmt (*it) == switch_body_seq)
+	    gimplify_seq_add_stmt (pre_p, switch_stmt);
+	  else
+	    gsi_insert_before_without_update (it, switch_stmt, GSI_SAME_STMT);
+	  free (it);
+	}
+      else
+	gimplify_seq_add_stmt (pre_p, switch_stmt);
+
       gimplify_seq_add_seq (pre_p, switch_body_seq);
       labels.release ();
     }
diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index db72156..150b2ab 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -43,6 +43,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "langhooks.h"
 #include "cfgloop.h"
 #include "gimple-low.h"
+#include "asan.h"
 
 /* In some instances a tree and a gimple need to be stored in a same table,
    i.e. in hash tables. This is a structure to do this. */
@@ -706,6 +707,9 @@  verify_norecord_switch_expr (struct leh_state *state,
   if (!tf)
     return;
 
+  if (asan_sanitize_use_after_scope ())
+    return;
+
   n = gimple_switch_num_labels (switch_expr);
 
   for (i = 0; i < n; ++i)
-- 
2.10.1