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

[Michael Matz <matz at suse dot de>]

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.

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 (!).


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
> 
> 
>