Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186).

[Martin Liška <mliska at suse dot cz>]

On 06/27/2017 05:29 PM, Michael Matz wrote:
> Hi,
> 
> On Tue, 27 Jun 2017, Martin Liška wrote:
> 
>> Following bug was for me very educative. I learned that we support 
>> non-local gotos that can be combined with nested functions. Real fun :) 
>> Well, the problem is that both cfun->nonlocal_goto_save_area and 
>> cfun->static_chain_decl (emitted in expand_function_start) are put 
>> before NOTE_INSN_FUNCTION_BEG. And so result of expand_used_vars is put 
>> after these instrumentations. That causes problems as it uses stack 
>> before we initialize it (use-after-return checking):
> 
> I don't think that's the right fix.  The purpose of 
> NOTE_INSN_FUNCTION_BEG is to mark the "real" beginning of the function, 
> i.e. without all the compiler generated stuff that's necessary to set up 
> parameters or local variables and so on.  The goto save area and the 
> static chain are also such compiler generated implementation details, and 
> hence are correctly put in front of the function begin note.

Hello.

Thanks for the review. I'm not so familiar with RTL, but hopefully new version
of the patch should do it properly. Idea is to come up with a new asan_stack_birth_insn
that points after place where stack is ready to use (when one uses use-after-return).
And then in function.c static_chain_decl and nonlocal_goto_save_area expansion is
generated to a new sequence and the sequence is put after the asan_stack_birth_insn.

> 
> Also if you put something in front of the static_chain_decl initialization 
> (as you do if you move the parm_birth_insn in front of it) you'd have to 
> make sure that the incoming hidding parameter containing the static chain 
> (r10 on x86_64) isn't clobbered from function start up to that point.  
> So that won't work either generally.
> 
> I don't know what exactly is the issue with calling 
> __asan_handle_no_return before the other instructions emitted by 
> expand_used_vars.  Either it shouldn't be called for the static chain 
> (i.e. not instrumented) or whatever setup asan needs needs to happen in 
> front of the static chain setup, but then without clobbering the incoming 
> static chain param (!).

Here I need to have FRAME variable to be sanitized as seen in pr78541.c
and pr78541-2.c test-cases.

Thoughts?
Thanks,
Martin

> 
> 
> Ciao,
> Michael.
>>
>> expanded cfun->static_chain_decl:
>>
>> (note 1 0 5 NOTE_INSN_DELETED)
>> (note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
>> (insn 2 5 3 2 (set (reg/f:DI 88 [ CHAIN.1 ])
>>         (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1
>>      (nil))
>> (insn 3 2 4 2 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
>>                 (const_int -8 [0xfffffffffffffff8])) [0  S8 A64])
>>         (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1
>>      (nil))
>> (note 4 3 7 2 NOTE_INSN_FUNCTION_BEG)
>> (insn 7 4 8 2 (set (reg/f:DI 87 [ _2 ])
>>         (reg/f:DI 88 [ CHAIN.1 ])) "pr81186.c":5 -1
>>      (nil))
>> (call_insn 8 7 9 2 (call (mem:QI (symbol_ref:DI ("__asan_handle_no_return") [flags 0x41]  <function_decl 0x2ba005ad5d00 __builtin___asan_handle_no_return>) [0 __builtin___asan_handle_no_return S1 A8])
>>         (const_int 0 [0])) "pr81186.c":5 -1
>>      (expr_list:REG_EH_REGION (const_int 0 [0])
>>         (nil))
>>     (nil))
>>
>> expanded cfun->nonlocal_goto_save_area:
>>
>> (note 1 0 34 NOTE_INSN_DELETED)
>> (note 34 1 31 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
>> (insn 31 34 32 2 (set (mem/f/c:DI (plus:DI (reg:DI 95)
>>                 (const_int -64 [0xffffffffffffffc0])) [4 FRAME.0.__nl_goto_buf+0 S8 A64])
>>         (reg/f:DI 82 virtual-stack-vars)) "pr81186.c":3 -1
>>      (nil))
>> (insn 32 31 2 2 (set (mem/f/c:DI (plus:DI (reg:DI 95)
>>                 (const_int -56 [0xffffffffffffffc8])) [4 FRAME.0.__nl_goto_buf+8 S8 A64])
>>         (reg/f:DI 7 sp)) "pr81186.c":3 -1
>>      (nil))
>> (insn 2 32 3 2 (parallel [
>>             (set (reg:DI 96)
>>                 (plus:DI (reg/f:DI 82 virtual-stack-vars)
>>                     (const_int -96 [0xffffffffffffffa0])))
>>             (clobber (reg:CC 17 flags))
>>         ]) "pr81186.c":3 -1
>>      (nil))
>> (insn 3 2 4 2 (set (reg:DI 97)
>>         (reg:DI 96)) "pr81186.c":3 -1
>>      (nil))
>> (insn 4 3 5 2 (set (reg:CCZ 17 flags)
>>         (compare:CCZ (mem/c:SI (symbol_ref:DI ("__asan_option_detect_stack_use_after_return") [flags 0x40]  <var_decl 0x2ba005b5b750 __asan_option_detect_stack_use_after_return>) [5 __asan_option_detect_stack_use_after_return+0 S4 A32])
>>             (const_int 0 [0]))) "pr81186.c":3 -1
>>      (nil))
>>
>> And thus I suggest to move both these instrumentations after NOTE_INSN_FUNCTION_BEG.
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Ready to be installed?
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2017-06-27  Martin Liska  <mliska@suse.cz>
>>
>>         PR sanitize/81186
>> 	* function.c (expand_function_start): Move static chain and non-local
>> 	goto init after NOTE_INSN_FUNCTION_BEG.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-06-27  Martin Liska  <mliska@suse.cz>
>>
>>         PR sanitize/81186
>> 	* gcc.dg/asan/pr81186.c: New test.
>> ---
>>  gcc/function.c                      | 18 +++++++++---------
>>  gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++
>>  2 files changed, 22 insertions(+), 9 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c
>>
>>

>From d52e3359b189aaa17fd8cd42a7e1ea72227a452d Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 23 Jun 2017 14:05:59 +0200
Subject: [PATCH] Emit nonlocal_goto_save_area and static_chain_decl properly
 (PR sanitize/81186).

gcc/ChangeLog:

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

        PR sanitize/81186
	* asan.c (asan_emit_stack_protection): Emit
	asan_stack_birth_insn.
	* emit-rtl.h (struct GTY): Add x_asan_stack_birth_insn.
	* function.c (expand_function_start): Move nonlocal_goto_save_area
	and static_chain_decl expansion after asan_stack_birth_insn if
	set.

gcc/testsuite/ChangeLog:

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

        PR sanitize/81186
	* gcc.dg/asan/pr81186.c: New test.
---
 gcc/asan.c                          |  1 +
 gcc/emit-rtl.h                      |  4 ++++
 gcc/function.c                      | 13 +++++++++++++
 gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++
 4 files changed, 31 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c

diff --git a/gcc/asan.c b/gcc/asan.c
index e730530930b..c7c839fb1a1 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1168,6 +1168,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
 					   gen_int_mode (base_align_bias
 							 - base_offset, Pmode),
 					   NULL_RTX, 1, OPTAB_DIRECT));
+      asan_stack_birth_insn = get_last_insn ();
     }
   mem = gen_rtx_MEM (ptr_mode, base);
   mem = adjust_address (mem, VOIDmode, base_align_bias);
diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
index b455b4c00fb..7658fa121f2 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -128,6 +128,9 @@ struct GTY(()) rtl_data {
      If stack grows up, this is the address for the next slot.  */
   HOST_WIDE_INT x_frame_offset;
 
+  /* Insn after which AddressSanitizer prepares stack for use.  */
+  rtx_insn *x_asan_stack_birth_insn;
+
   /* Insn after which register parms and SAVE_EXPRs are born, if nonopt.  */
   rtx_insn *x_parm_birth_insn;
 
@@ -298,6 +301,7 @@ struct GTY(()) rtl_data {
 #define return_label (crtl->x_return_label)
 #define naked_return_label (crtl->x_naked_return_label)
 #define stack_slot_list (crtl->x_stack_slot_list)
+#define asan_stack_birth_insn (crtl->x_asan_stack_birth_insn)
 #define parm_birth_insn (crtl->x_parm_birth_insn)
 #define frame_offset (crtl->x_frame_offset)
 #define stack_check_probe_note (crtl->x_stack_check_probe_note)
diff --git a/gcc/function.c b/gcc/function.c
index f625489205b..c0c626d32c4 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5220,6 +5220,8 @@ expand_function_start (tree subr)
      In some cases this requires emitting insns.  */
   assign_parms (subr);
 
+  start_sequence ();
+
   /* If function gets a static chain arg, store it.  */
   if (cfun->static_chain_decl)
     {
@@ -5284,6 +5286,17 @@ expand_function_start (tree subr)
       update_nonlocal_goto_save_area ();
     }
 
+  rtx_insn *seq = get_insns ();
+  end_sequence ();
+
+  if (seq)
+    {
+      if (asan_stack_birth_insn == NULL_RTX)
+	asan_stack_birth_insn = get_insns ();
+
+      emit_insn_after (seq, asan_stack_birth_insn);
+    }
+
   /* The following was moved from init_function_start.
      The move is supposed to make sdb output more accurate.  */
   /* Indicate the beginning of the function body,
diff --git a/gcc/testsuite/gcc.dg/asan/pr81186.c b/gcc/testsuite/gcc.dg/asan/pr81186.c
new file mode 100644
index 00000000000..74d3837a482
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asan/pr81186.c
@@ -0,0 +1,13 @@
+/* PR sanitizer/81186 */
+/* { dg-do run } */
+
+int
+main ()
+{
+  __label__ l;
+  void f () { goto l; }
+
+  f ();
+l:
+  return 0;
+}
-- 
2.13.1