On 04/17/2013 02:49 AM, Han Shen wrote:
+ if (flag_stack_protect == 3)
+ cpp_define (pfile, "__SSP_STRONG__=3");
if (flag_stack_protect == 2)
cpp_define (pfile, "__SSP_ALL__=2");
3 and 2 should be replaced by SPCT_FLAG_STRONG and SPCT_FLAG_ALL.
I define these SPCT_FLAG_XXX in cfgexpand.c locally, so they are not
visible to c-cppbuiltin.c, do you suggest define these inside
c-cppbuiltin.c also?
I see. Let's use the constants for now.
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.
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-)
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.
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.
ChangeLog and patch below --
gcc/ChangeLog
2013-04-16 Han Shen <shen...@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.
(expand_used_vars): Add logic handling '-fstack-protector-strong'.
* common.opt (fstack-protector-all): New option.
Should be "fstack-protector-strong".
--
Florian Weimer / Red Hat Product Security Team
