Re: [PATCH] Invalid unpoisoning of stack redzones on ARM

[Richard Sandiford]

Jakub Jelinek <ja...@redhat.com> writes:
> 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:

Hmm, it just seems wrong to be assigning to registers returned by force_reg
and expand_binop though.  Would this variation be OK?

Thanks,
Richard


Index: gcc/asan.c
===================================================================
--- gcc/asan.c  (revision 204124)
+++ gcc/asan.c  (working copy)
@@ -876,9 +876,10 @@
 }
 
 /* Clear shadow memory at SHADOW_MEM, LEN bytes.  Can't call a library call 
here
-   though.  */
+   though.  If the address of the next byte is in a known register, return
+   that register, otherwise return null.  */
 
-static void
+static rtx
 asan_clear_shadow (rtx shadow_mem, HOST_WIDE_INT len)
 {
   rtx insn, insns, top_label, end, addr, tmp, jump;
@@ -893,12 +894,12 @@
   if (insn == NULL_RTX)
     {
       emit_insn (insns);
-      return;
+      return 0;
     }
 
   gcc_assert ((len & 3) == 0);
   top_label = gen_label_rtx ();
-  addr = force_reg (Pmode, XEXP (shadow_mem, 0));
+  addr = copy_to_mode_reg (Pmode, 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 +913,7 @@
   jump = get_last_insn ();
   gcc_assert (JUMP_P (jump));
   add_int_reg_note (jump, REG_BR_PROB, REG_BR_PROB_BASE * 80 / 100);
+  return addr;
 }
 
 /* Insert code to protect stack vars.  The prologue sequence should be emitted
@@ -1048,7 +1050,15 @@
                                       (last_offset - prev_offset)
                                       >> ASAN_SHADOW_SHIFT);
          prev_offset = last_offset;
-         asan_clear_shadow (shadow_mem, last_size >> ASAN_SHADOW_SHIFT);
+         rtx end_addr = asan_clear_shadow (shadow_mem,
+                                           last_size >> ASAN_SHADOW_SHIFT);
+         if (end_addr)
+           {
+             shadow_mem
+               = adjust_automodify_address (shadow_mem, VOIDmode, end_addr,
+                                            last_size >> ASAN_SHADOW_SHIFT);
+             prev_offset += last_size;
+           }
          last_offset = offset;
          last_size = 0;
        }