Re: [PATCH] Add a new option "-fstack-protector-strong"

[Han Shen(ææ) <shenhan at google dot com>]

Thanks.

On Wed, Apr 17, 2013 at 2:26 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 04/17/2013 02:49 AM, Han Shen wrote:
>>> Indentation is off (unless both mail clients I tried are clobbering your
>>> patch).  I think the GNU coding style prohibits the braces around the
>>> single-statement body of the outer 'for".
>>>
>> Done with indentation properly on and removed the braces. (GMail
>> composing window drops all the tabs when pasting... I have to use
>> Thunderbird to paste the patch. Hope it is right this time)
>
>
> Thunderbird mangles patches as well, but I was able to repair the damage.
> When using Thunderbird, please send the patch as a text file attachment.
> You can put the changelog snippets at the beginning of the file as well.
> This way, everything is sent out unchanged.
>
Patch sent as attachment.
>
>
>>> Can you make the conditional more similar to the comment, perhaps using a
>>> switch statement on the value of the flag_stack_protect variable? That's
>>> going to be much easier to read.
>>>
>> Re-coded. Now using 'switch-case'.
>
>
> Thanks.  I think the comment is now redundant because it matches the code
> almost word-for-word. 8-)
>
Comment deleted.
>
>> No for 'struct-returning' functions. But I regard this not an issue ---
>> at the programming level, there is no way to get one's hand on the
>> address of a returned structure ---
>>     struct Node foo();
>>     struct Node *p = &foo();  // compiler error - lvalue required as
>> unary '&' operand.
>
>
> C++ const references can bind to rvalues.
> 2013-04-11  Jakub Jelinek  <jakub@redhat.com>
> But I'm more worried about the interaction with the return value
> optimization.  Consider this C++ code:
>
> struct S {
>   S();
>   int a;
>   int b;
>   int c;
>   int d;
>   int e;
> };
>
> void f1(int *);
>
> S f2()
> {
>   S s;
>   f1(&s.a);
>   return s;
> }
>
> S g2();
>
> void g3()
> {
>   S s = g2();
> }
>
> void g3b(const S&);
>
> void g3b()
> {
>   g3b(g2());
> }
>
> With your patch and -O2 -fstack-protector-strong, this generates the
> following assembly:
>
>         .globl  _Z2f2v
>         .type   _Z2f2v, @function
> _Z2f2v:
> .LFB0:
>         .cfi_startproc
>         pushq   %rbx
>         .cfi_def_cfa_offset 16
>         .cfi_offset 3, -16
>         movq    %rdi, %rbx
>         call    _ZN1SC1Ev
>         movq    %rbx, %rdi
>         call    _Z2f1Pi
>         movq    %rbx, %rax
>         popq    %rbx
>         .cfi_def_cfa_offset 8
>         ret
>         .cfi_endproc
> .LFE0:
>         .size   _Z2f2v, .-_Z2f2v
>         .p2align 4,,15
>         .globl  _Z2g3v
>         .type   _Z2g3v, @function
> _Z2g3v:
> .LFB1:
>         .cfi_startproc
>         subq    $40, %rsp
>         .cfi_def_cfa_offset 48
>         movq    %rsp, %rdi
>         call    _Z2g2v
>         addq    $40, %rsp
>         .cfi_def_cfa_offset 8
>         ret
>         .cfi_endproc
> .LFE1:
>         .size   _Z2g3v, .-_Z2g3v
>         .p2align 4,,15
>         .globl  _Z3g3bv
>         .type   _Z3g3bv, @function
> _Z3g3bv:
> .LFB2:
>         .cfi_startproc
>         subq    $40, %rsp
>         .cfi_def_cfa_offset 48
>         movq    %rsp, %rdi
>         movq    %fs:40, %rax
>         movq    %rax, 24(%rsp)
>         xorl    %eax, %eax
>         call    _Z2g2v
>         movq    %rsp, %rdi
>         call    _Z3g3bRK1S
>         movq    24(%rsp), %rax
>         xorq    %fs:40, %rax
>         jne     .L9
>         addq    $40, %rsp
>         .cfi_remember_state
>         .cfi_def_cfa_offset 8
>         ret
> .L9:
>         .cfi_restore_state
>         .p2align 4,,6
>         call    __stack_chk_fail
>         .cfi_endproc
> .LFE2:
>         .size   _Z3g3bv, .-_Z3g3bv
>
> Here, g3b() is correctly instrumented, and f2() does not need
> instrumentation (because space for the returned object is not part of the
> local frame).  But an address on the stack escapes in g3() and is used for
> the return value of the call to g2().  This requires instrumentation, which
> is missing in this example.
>
> I suppose this can be handled in a follow-up patch if necessary.
>
Thanks for the case. Yeah, I see the problem here - in cfgexpand phase
- where functions being scanned for stack protection - the tree
representation has no indication that a local address is computed,
just as listed below -
  void g3() ()
  {
    struct S s;
    <bb 2>:
    s = g2 (); [return slot optimization]
    s ={v} {CLOBBER};
    return;
  }
One solution might be to scan for the "[return slot optimization]" tag
in the tree. I'll post later another patch to address this.
>
>> ChangeLog and patch below --
>>
>> gcc/ChangeLog
>> 2013-04-16  Han Shen  <shenhan@google.com>
>>      * cfgexpand.c (record_or_union_type_has_array_p): Helper function
>>      to check if a record or union contains an array field.
>
>
> I think the GNU convention is to write only this:
>
>         * cfgexpand.c (record_or_union_type_has_array_p): New function.
Done.
>
>
>>      (expand_used_vars): Add logic handling '-fstack-protector-strong'.
>>      * common.opt (fstack-protector-all): New option.
>
>
> Should be "fstack-protector-strong".
Done.
>
>
> --
> Florian Weimer / Red Hat Product Security Team

Patch attached as plain txt.

Thanks,
H.

Attachment: patch
Description: Binary data