diff mbox

Invalid unpoisoning of stack redzones on ARM

Message ID 20131029093730.GV30970@tucnak.zalov.cz
State New
Headers show

Commit Message

Jakub Jelinek Oct. 29, 2013, 9:37 a.m. UTC 
On Wed, Oct 16, 2013 at 09:35:21AM +0400, Yury Gribov wrote:
> >>> I've recently submitted a bug report regarding invalid
> unpoisoning of stack frame redzones
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58543). Could someone
> take a look at proposed patch (a simple one-liner) and check whether
> it's ok for commit?
> >>
> >> Can you please be more verbose
> >
> > Do you think the proposed patch is ok or should I provide some
> additional info?
> 
> Jakub, Dodji,
> 
> Any updates on this one? Note that this bug is a huge blocker for
> using AddressSanitizer on ARM platforms.

Sorry for the delay, I finally found time to look at it.
While your patch fixes the issue, I wonder if for the case where
shadow_mem before the asan_clear_shadow call is already of the form
(mem (reg)) it isn't better to just adjust next asan_clear_shadow
base from the end of the cleared region rather than from the start of it,
because the end will be already in the right pseudo, while with your
approach it needs to be done in a different register and potentially
increase register pressure.  So here is (completely untested) alternate fix:

2013-10-29  Jakub Jelinek  <jakub@redhat.com>
	    Yury Gribov  <y.gribov@samsung.com>

	PR sanitizer/58543
	* asan.c (asan_clear_shadow): Return bool whether the emitted loop
	(if any) updated shadow_mem's base.
	(asan_emit_stack_protection): If asan_clear_shadow returned true,
	adjust shadow_mem and prev_offset.



	Jakub

Comments

Yury Gribov Oct. 29, 2013, 11:57 a.m. UTC | #1 
> Sorry for the delay, I finally found time to look at it.

Np, thanks for helping!

 > While your patch fixes the issue, I wonder
 > ...
 > potentially increase register pressure.

Makes sense. I didn't take care of this because I believed that we can 
freely allocate vregs and rely on register allocator do it's work.

 > So here is (completely untested) alternate fix:

Nice, this seems to fix my repro cases.

I'll only be able to test my larger ARM app in 1-2 days. We could wait 
until then or commit now.

-Y
diff mbox

Patch

--- gcc/asan.c.jj	2013-10-23 14:43:15.000000000 +0200
+++ gcc/asan.c	2013-10-29 10:26:56.085406934 +0100
@@ -878,10 +878,11 @@  asan_shadow_cst (unsigned char shadow_by
 /* Clear shadow memory at SHADOW_MEM, LEN bytes.  Can't call a library call here
    though.  */
 
-static void
+static bool
 asan_clear_shadow (rtx shadow_mem, HOST_WIDE_INT len)
 {
   rtx insn, insns, top_label, end, addr, tmp, jump;
+  bool ret;
 
   start_sequence ();
   clear_storage (shadow_mem, GEN_INT (len), BLOCK_OP_NORMAL);
@@ -893,12 +894,13 @@  asan_clear_shadow (rtx shadow_mem, HOST_
   if (insn == NULL_RTX)
     {
       emit_insn (insns);
-      return;
+      return false;
     }
 
   gcc_assert ((len & 3) == 0);
   top_label = gen_label_rtx ();
   addr = force_reg (Pmode, XEXP (shadow_mem, 0));
+  ret = addr == XEXP (shadow_mem, 0);
   shadow_mem = adjust_automodify_address (shadow_mem, SImode, addr, 0);
   end = force_reg (Pmode, plus_constant (Pmode, addr, len));
   emit_label (top_label);
@@ -912,6 +914,7 @@  asan_clear_shadow (rtx shadow_mem, HOST_
   jump = get_last_insn ();
   gcc_assert (JUMP_P (jump));
   add_int_reg_note (jump, REG_BR_PROB, REG_BR_PROB_BASE * 80 / 100);
+  return ret;
 }
 
 /* Insert code to protect stack vars.  The prologue sequence should be emitted
@@ -1048,7 +1051,14 @@  asan_emit_stack_protection (rtx base, HO
 				       (last_offset - prev_offset)
 				       >> ASAN_SHADOW_SHIFT);
 	  prev_offset = last_offset;
-	  asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT);
+	  if (asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT))
+	    {
+	      shadow_mem
+		= adjust_automodify_address (shadow_mem, VOIDmode,
+					     XEXP (shadow_mem, 0),
+					     last_size >> ASAN_SHADOW_SHIFT);
+	      prev_offset += last_size;
+	    }
 	  last_offset = offset;
 	  last_size = 0;
 	}