[PATCH] Improve -fstack-protector-strong (PR middle-end/92825)

Jeff Law law@redhat.com
Tue Dec 10 20:52:00 GMT 2019


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



More information about the Gcc-patches mailing list