PING^1
Jakub can you please take a look? I would like to have it in 7.2 if possible.
Thanks,
Martin
On 07/18/2017 10:38 AM, Martin Liška wrote:
> On 07/17/2017 03:15 PM, Michael Matz wrote:
>> Hello,
>>
>> On Mon, 17 Jul 2017, Martin Liška wrote:
>>
>>> which does all the stack preparation (including the problematic call to
>>> __asan_stack_malloc_N).
>>>
>>> Note that this code still should be placed before parm_birth_note as we
>>> cant's say that params are ready before a fake stack is prepared.
>>
>> Yes, understood.
>>
>>> Then we generate code that loads the implicit chain argument:
>>>
>>> (gdb) p debug_rtx_list(get_insns(), 100)
>>> (note 1 0 37 NOTE_INSN_DELETED)
>>>
>>> (note 37 1 38 NOTE_INSN_FUNCTION_BEG)
>>>
>>> (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ])
>>> (reg:DI 39 r10 [ CHAIN.1 ]))
>>> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>>> (nil))
>>>
>>> (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars)
>>> (const_int -584 [0xfffffffffffffdb8])) [0 S8 A64])
>>> (reg:DI 39 r10 [ CHAIN.1 ]))
>>> "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1
>>> (nil))
>>>
>>> Which is problematic as using virtual-stack-vars which should point to
>>> fake stack done by AddressSanitizer in __asan_stack_malloc_N.
>>
>> If anything, then only the stack access is problematic, i.e. the last
>> instruction. I don't understand why that should be problematic, though.
>
> Hi.
>
> Thanks one more time, it's really educative this PR and whole problematic of
> function prologue.
> So short answer for your email: marking parm_birth_insn after static chain
> init solves the problem :)
> It's because:
>
> (insn 2 1 3 (set (reg/f:DI 100 [ CHAIN.2 ])
> (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1
> (nil))
>
> (insn 3 2 4 (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.2 ])) "/tmp/nested.c":6 -1
> (nil))
>
> is just storage of &FRAME.0 from caller where content of the FRAME struct
> lives on stack (and thus on
> shadow stack). That said it's perfectly fine to store &CHAIN to real stack of
> callee.
>
> Thus I'm going to test attached patch.
>
> P.S. One interesting side effect of how static chain is implemented:
>
> Consider:
>
> int
> main ()
> {
> __label__ l;
> int buffer[100];
> void f ()
> {
> int a[123];
> *(&buffer[0] - 4) = 123;
>
> goto l;
> }
>
> f ();
> l:
> return 0;
> }
>
> It's funny that *(&buffer[0] - 4) actually corrupts __nl_goto_buf and we end
> up with a
> dead signal:
>
> ASAN:DEADLYSIGNAL
> =================================================================
> ==30888==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc
> 0x000000000000 bp 0x000000000000 sp 0x7ffe0000049b T0)
>
> Thanks,
> Martin
>
>> Probably because I don't know much about the ASAN implementation. But why
>> should there be something magic about using the non-asan stack? Most
>> local variable accesses are rewritten to be in terms of the fake stack,
>> but those that aren't could use the normal stack just fine, can't they?
>>
>> If that really is a problem then that could also be rectified by splitting
>> the static_chain_decl in expand_function_start a bit, ala this:
>>
>> if (cfun->static_chain_decl) {
>> all code except the last "if (!optimize) store-into-stack"
>> }
>> emit_note; parm_birth_insn = ...
>> if (cfun->static_chain_decl && !optimize) {
>> store into assign_stack_local
>> }
>>
>> (requires moving some local variable to an outer scope, but hey).
>>
>> But what you say above mystifies me. You claim that access via
>> virtual-stack-vars is problematic before the shadow stack is created by
>> ASAN. But the whole parameter setup always uses such local stack storage
>> whenever it needs. And those definitely happen before the ASAN setup.
>> See the subroutines of assign_parms, (e.g. assign_parm_setup_block and
>> assign_parm_setup_stack). You might need to use special function argument
>> types or special ABIs to trigger this, though you should be able to find
>> some cases to trigger also on i386 or x86_64.
>>
>> So, if the stack access for the static chain is problematic I don't see
>> why the stack accesses for the parameters are not. And if they indeed are
>> problematic, then something is confused within ASAN, and the fix for that
>> confusion is not to move parm_birth_insn, but something else (I can't say
>> what, as I don't know much about how ASAN is supposed to work in such
>> situations).
>>
>>
>> Ciao,
>> Michael.
>>
>
