diff mbox

[RFC] Sanopt for use-after-scope ASAN_MARK internal functions

Message ID 5ea79c11-5164-e496-5d1b-801510c9c543@suse.cz
State Superseded
Headers show

Commit Message

Martin Liška Dec. 13, 2016, 12:12 p.m. UTC
On 12/13/2016 10:17 AM, Jakub Jelinek wrote:
> On Tue, Dec 13, 2016 at 09:56:00AM +0100, Martin Liška wrote:

>> @@ -671,18 +678,203 @@ public:

>>  

>>  }; // class pass_sanopt

>>  

> 

> Please add a short function comment here...

> 

>> +static void

>> +sanitize_asan_mark_unpoison (void)

>> +{

>> +  /* 1) Find all BBs that contain an ASAN_MARK poison call.  */

>> +  auto_sbitmap with_poison (last_basic_block_for_fn (cfun) + 1);

>> +  bitmap_clear (with_poison);

>> +  basic_block bb;

> 

> ... and here.


Done.

> 

>> +static void

>> +sanitize_asan_mark_poison (void)

>> +{

>> +  /* 1) Find all BBs that possibly contain an ASAN_CHECK.  */

>> +  auto_sbitmap with_check (last_basic_block_for_fn (cfun) + 1);

>> +  bitmap_clear (with_check);

>> +  basic_block bb;

> 

>> +  FOR_EACH_BB_FN (bb, cfun)

>> +    {

>> +      if (bitmap_bit_p (with_check, bb->index))

>> +	continue;

> 

> When would this happen?  It seems you only set it for the current bb,

> it doesn't seem that you'd try to process the same bb multiple times.


That's not needed, probably an artifact from very first version.

> 

>> +      gimple_stmt_iterator gsi;

>> +      bool finish = false;

>> +      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))

>> +	{

>> +	  gimple *stmt = gsi_stmt (gsi);

>> +	  if (maybe_contains_asan_check (stmt))

>> +	    {

>> +	      bitmap_set_bit (with_check, bb->index);

>> +	      finish = true;

> 

> Why the finish var and separate if (finish) break; ?  Perhaps from the first

> iteration when you used a switch?  Just doing break; should be enough.


Well, I verified that even the first iteration (of patch) needed if(finish) break;
as I need to jump to next iteration of FOR_EACH_BB_FN (bb, cfun).

> 

>> +	  else if (asan_mark_p (stmt, ASAN_MARK_POISON))

>> +	    {

>> +	      if (dump_file)

>> +		fprintf (dump_file, "Removing ASAN_MARK poison\n");

>> +	      unlink_stmt_vdef (stmt);

>> +	      gsi_remove (&gsi, true);

> 

> Don't you need also release_defs (stmt); here (and in the other gsi_remove

> spot)?


As I remove only internal function calls that do not define a SSA name then not:
  ASAN_MARK (UNPOISON, &my_char, 1);
  ASAN_MARK (POISON, &my_char, 1);

Ready to install?
M.

> 

> 	Jakub

>

Comments

Jakub Jelinek Dec. 13, 2016, 12:29 p.m. UTC | #1
On Tue, Dec 13, 2016 at 01:12:34PM +0100, Martin Liška wrote:
> >> +      gimple_stmt_iterator gsi;

> >> +      bool finish = false;

> >> +      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))

> >> +	{

> >> +	  gimple *stmt = gsi_stmt (gsi);

> >> +	  if (maybe_contains_asan_check (stmt))

> >> +	    {

> >> +	      bitmap_set_bit (with_check, bb->index);

> >> +	      finish = true;

> > 

> > Why the finish var and separate if (finish) break; ?  Perhaps from the first

> > iteration when you used a switch?  Just doing break; should be enough.

> 

> Well, I verified that even the first iteration (of patch) needed if(finish) break;

> as I need to jump to next iteration of FOR_EACH_BB_FN (bb, cfun).


I fail to see functional difference between
  for (...)
    {
      ...
      bool finish = false;
      for (...)
        {
          ...
          if (...)
	    {
	      ...
	      finish = true;
	    }
	  if (finish)
	    break;
        }
    }
and
  for (...)
    {
      ...
      for (...)
        {
          ...
          if (...)
	    {
	      ...
	      break;
	    }
        }
    }
just the latter is not obfuscated.  The break is in both cases in the same
for loop.

> > Don't you need also release_defs (stmt); here (and in the other gsi_remove

> > spot)?

> 

> As I remove only internal function calls that do not define a SSA name then not:

>   ASAN_MARK (UNPOISON, &my_char, 1);

>   ASAN_MARK (POISON, &my_char, 1);


If they have a vdef, then I believe they do define a SSA name (the vdef).
I think unlink_stmt_vdef does not release the vdef SSA_NAME if any.

	Jakub
diff mbox

Patch

From 9335008e783155e131ca1dec1ef468f371cfcd1e Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 12 Dec 2016 15:22:05 +0100
Subject: [PATCH] Add sanopt for ASAN_MARK poison and unpoison.

gcc/ChangeLog:

2016-12-12  Martin Liska  <mliska@suse.cz>

	* sanopt.c (sanopt_optimize_walker): Set contains_asan_mark.
	(sanopt_optimize): Add new argument.
	(sanitize_asan_mark_unpoison): New function.
	(maybe_contains_asan_check): Likewise.
	(sanitize_asan_mark_poison): Likewise.
	(pass_sanopt::execute): Call the new functions.
---
 gcc/sanopt.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 207 insertions(+), 3 deletions(-)

diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 320e14e9421..3aebb1621c9 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -160,8 +160,10 @@  struct sanopt_ctx
 
   /* Number of IFN_ASAN_CHECK statements.  */
   int asan_num_accesses;
-};
 
+  /* True when the current functions constains an ASAN_MARK.  */
+  bool contains_asan_mark;
+};
 
 /* Return true if there might be any call to free/munmap operation
    on any path in between DOM (which should be imm(BB)) and BB.  */
@@ -582,6 +584,9 @@  sanopt_optimize_walker (basic_block bb, struct sanopt_ctx *ctx)
 	    if (!remove)
 	      ctx->asan_num_accesses++;
 	    break;
+	  case IFN_ASAN_MARK:
+	    ctx->contains_asan_mark = true;
+	    break;
 	  default:
 	    break;
 	  }
@@ -620,10 +625,11 @@  sanopt_optimize_walker (basic_block bb, struct sanopt_ctx *ctx)
 /* Try to remove redundant sanitizer checks in function FUN.  */
 
 static int
-sanopt_optimize (function *fun)
+sanopt_optimize (function *fun, bool *contains_asan_mark)
 {
   struct sanopt_ctx ctx;
   ctx.asan_num_accesses = 0;
+  ctx.contains_asan_mark = false;
 
   /* Set up block info for each basic block.  */
   alloc_aux_for_blocks (sizeof (sanopt_info));
@@ -638,6 +644,7 @@  sanopt_optimize (function *fun)
 
   free_aux_for_blocks ();
 
+  *contains_asan_mark = ctx.contains_asan_mark;
   return ctx.asan_num_accesses;
 }
 
@@ -671,18 +678,207 @@  public:
 
 }; // class pass_sanopt
 
+/* Sanitize all ASAN_MARK unpoison calls that are not reachable by a BB
+   that contains an ASAN_MARK poison.  All these ASAN_MARK unpoison call
+   can be removed as all variables are unpoisoned in a function prologue.  */
+
+static void
+sanitize_asan_mark_unpoison (void)
+{
+  /* 1) Find all BBs that contain an ASAN_MARK poison call.  */
+  auto_sbitmap with_poison (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (with_poison);
+  basic_block bb;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      if (bitmap_bit_p (with_poison, bb->index))
+	continue;
+
+      gimple_stmt_iterator gsi;
+      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (asan_mark_p (stmt, ASAN_MARK_POISON))
+	    {
+	      bitmap_set_bit (with_poison, bb->index);
+	      break;
+	    }
+	}
+    }
+
+  auto_sbitmap poisoned (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (poisoned);
+  auto_sbitmap worklist (last_basic_block_for_fn (cfun) + 1);
+  bitmap_copy (worklist, with_poison);
+
+  /* 2) Propagate the information to all reachable blocks.  */
+  while (!bitmap_empty_p (worklist))
+    {
+      unsigned i = bitmap_first_set_bit (worklist);
+      bitmap_clear_bit (worklist, i);
+      basic_block bb = BASIC_BLOCK_FOR_FN (cfun, i);
+      gcc_assert (bb);
+
+      edge e;
+      edge_iterator ei;
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	if (!bitmap_bit_p (poisoned, e->dest->index))
+	  {
+	    bitmap_set_bit (poisoned, e->dest->index);
+	    bitmap_set_bit (worklist, e->dest->index);
+	  }
+    }
+
+  /* 3) Iterate all BBs not included in POISONED BBs and remove unpoison
+	ASAN_MARK preceding an ASAN_MARK poison (which can still happen).  */
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      if (bitmap_bit_p (poisoned, bb->index))
+	continue;
+
+      gimple_stmt_iterator gsi;
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi);)
+	{
+	  bool next = true;
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+	    {
+	      if (asan_mark_p (stmt, ASAN_MARK_POISON))
+		break;
+	      else
+		{
+		  if (dump_file)
+		    fprintf (dump_file, "Removing ASAN_MARK unpoison\n");
+		  unlink_stmt_vdef (stmt);
+		  gsi_remove (&gsi, true);
+		  next = false;
+		}
+	    }
+
+	  if (next)
+	    gsi_next (&gsi);
+	}
+    }
+}
+
+/* Return true when STMT is either ASAN_CHECK call or a call of a function
+   that can contain an ASAN_CHECK.  */
+
+static bool
+maybe_contains_asan_check (gimple *stmt)
+{
+  if (is_gimple_call (stmt))
+    {
+      if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+	return false;
+      else
+	return !(gimple_call_flags (stmt) & ECF_CONST);
+    }
+  else if (is_a<gasm *> (stmt))
+    return true;
+
+  return false;
+}
+
+/* Sanitize all ASAN_MARK poison calls that are not followed by an ASAN_CHECK
+   call.  These calls can be removed.  */
+
+static void
+sanitize_asan_mark_poison (void)
+{
+  /* 1) Find all BBs that possibly contain an ASAN_CHECK.  */
+  auto_sbitmap with_check (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (with_check);
+  basic_block bb;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      gimple_stmt_iterator gsi;
+      bool finish = false;
+      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (maybe_contains_asan_check (stmt))
+	    {
+	      bitmap_set_bit (with_check, bb->index);
+	      finish = true;
+	    }
+
+	  if (finish)
+	    break;
+	}
+    }
+
+  auto_sbitmap can_reach_check (last_basic_block_for_fn (cfun) + 1);
+  bitmap_clear (can_reach_check);
+  auto_sbitmap worklist (last_basic_block_for_fn (cfun) + 1);
+  bitmap_copy (worklist, with_check);
+
+  /* 2) Propagate the information to all definitions blocks.  */
+  while (!bitmap_empty_p (worklist))
+    {
+      unsigned i = bitmap_first_set_bit (worklist);
+      bitmap_clear_bit (worklist, i);
+      basic_block bb = BASIC_BLOCK_FOR_FN (cfun, i);
+      gcc_assert (bb);
+
+      edge e;
+      edge_iterator ei;
+      FOR_EACH_EDGE (e, ei, bb->preds)
+	if (!bitmap_bit_p (can_reach_check, e->src->index))
+	  {
+	    bitmap_set_bit (can_reach_check, e->src->index);
+	    bitmap_set_bit (worklist, e->src->index);
+	  }
+    }
+
+  /* 3) Iterate all BBs not included in CAN_REACH_CHECK BBs and remove poison
+	ASAN_MARK not followed by a call to function having an ASAN_CHECK.  */
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      if (bitmap_bit_p (can_reach_check, bb->index))
+	continue;
+
+      gimple_stmt_iterator gsi;
+      bool finish = false;
+      for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi);)
+	{
+	  bool prev = true;
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (maybe_contains_asan_check (stmt))
+	    finish = true;
+	  else if (asan_mark_p (stmt, ASAN_MARK_POISON))
+	    {
+	      if (dump_file)
+		fprintf (dump_file, "Removing ASAN_MARK poison\n");
+	      unlink_stmt_vdef (stmt);
+	      gsi_remove (&gsi, true);
+	      prev = false;
+	    }
+
+	  if (finish)
+	    break;
+
+	  if (prev)
+	    gsi_prev (&gsi);
+	}
+    }
+}
+
 unsigned int
 pass_sanopt::execute (function *fun)
 {
   basic_block bb;
   int asan_num_accesses = 0;
+  bool contains_asan_mark = false;
 
   /* Try to remove redundant checks.  */
   if (optimize
       && (flag_sanitize
 	  & (SANITIZE_NULL | SANITIZE_ALIGNMENT
 	     | SANITIZE_ADDRESS | SANITIZE_VPTR)))
-    asan_num_accesses = sanopt_optimize (fun);
+    asan_num_accesses = sanopt_optimize (fun, &contains_asan_mark);
   else if (flag_sanitize & SANITIZE_ADDRESS)
     {
       gimple_stmt_iterator gsi;
@@ -692,9 +888,17 @@  pass_sanopt::execute (function *fun)
 	    gimple *stmt = gsi_stmt (gsi);
 	    if (gimple_call_internal_p (stmt, IFN_ASAN_CHECK))
 	      ++asan_num_accesses;
+	    else if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+	      contains_asan_mark = true;
 	  }
     }
 
+  if (contains_asan_mark)
+    {
+      sanitize_asan_mark_unpoison ();
+      sanitize_asan_mark_poison ();
+    }
+
   bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX
     && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
 
-- 
2.11.0