On Tue, 2019-12-10 at 10:27 +0100, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, -fstack-protector-strong in some cases
> instruments
> even functions which except for the stack canary setup and later
> testing of
> that don't touch the stack at all.
>
> This is because for -fstack-protector-strong we setup the canary
> whenever stack_protect_decl_p returns true, and that goes through all
> local
> decls and if it is a var that is not a global var and is either
> address
> taken or is array or has arrays embedded in its type, returns true.
> Now, the problem is that by going through all non-global variables,
> it goes
> even through variables that are or have arrays, but we allocate them
> in
> registers like in the testcase below, or in theory for nested
> functions
> it could be variables from the parent function too.
> Variables which live in registers, even when they have arrays in
> them,
> shouldn't really cause stack overflows of any kind, out of bounds
> accesses
> to them never cause out of bounds stack accesses, usually they ought
> to be
> already optimized away, arrays that are accessed through non-constant
> indexes should cause variables to live in memory.
>
> Another issue is that the record_or_union_type_has_array_p function
> duplicates
> the behavior of stack_protect_classify_type when it returns the
> SPCT_HAS_ARRAY
> bit set.
>
> Initially, I thought I'd just move the stack_protect_decl_p call
> later when
> the stack_vars array is computed and instead of FOR_EACH_LOCAL_DECL
> walk the stack_vars array. But for the discovery of stack_vars that
> are or
> have arrays that is a waste of time, all such variables are already
> picked
> for phase 1 or phase 2 and thus has_protected_decls bool will be set
> if
> there are any and we already do create the stack canary if
> has_protected_decls is set. So, the only thing that remains is catch
> variables that aren't or don't contain arrays, but are address
> taken. Those
> go into the last unnumbered phase, but we still want to create stack
> canary.
> Instead of adding yet another walk I've done that in a function that
> already
> walks them.
>
> In addition to this, I've tried to clarify this in the
> documentation. For
> -fstack-protector, we've been doing it this way already before,
> optimized
> away or arrays living in registers didn't count, because we only
> walked
> stack-vars. And, the documentation also said that only > 8 byte
> arrays are
> protected with -stack-protector, but it is actually >= 8 byte (or
> whatever
> the ssp-buffer-size param is).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, in addition
> regtested
> (just check-gcc, check-c++-all and check-target-libstdc++-v3) with
> --target_board=unix/-fstack-protector-strong on both.
> Ok for trunk?
>
> 2019-12-10 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/92825
> * cfgexpand.c (add_stack_protection_conflicts): Change return
> type
> from void to bool, return true if at least one
> stack_vars[i].decl
> is addressable.
> (record_or_union_type_has_array_p, stack_protect_decl_p):
> Remove.
> (expand_used_vars): Don't call stack_protect_decl_p, instead
> for
> -fstack-protector-strong set gen_stack_protect_signal to true
> if add_stack_protection_conflicts returned true. Formatting
> fixes.
> * doc/invoke.texi (-fstack-protector-strong): Clarify that
> optimized
> out variables or variables not living on the stack don't count.
> (-fstack-protector): Likewise. Clarify it affects >= 8 byte
> arrays
> rather than > 8 byte.
>
> * gcc.target/i386/pr92825.c: New test.
OK
jeff
>
