From 6b24e342cb30abb5c13e3092929837545a5bd49e Mon Sep 17 00:00:00 2001 From: Jakub Jelinek Date: Tue, 10 Dec 2019 22:04:08 +0100 Subject: [PATCH] re PR middle-end/92825 (Unnecesary stack protection in Firefox's LightPixel.) 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. From-SVN: r279193 --- gcc/ChangeLog | 13 +++++ gcc/cfgexpand.c | 76 ++++++++----------------- gcc/doc/invoke.texi | 13 +++-- gcc/testsuite/ChangeLog | 5 ++ gcc/testsuite/gcc.target/i386/pr92825.c | 15 +++++ 5 files changed, 65 insertions(+), 57 deletions(-) create mode 100644 gcc/testsuite/gcc.target/i386/pr92825.c diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 8130b6689da..4504e3c9115 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,5 +1,18 @@ 2019-12-10 Jakub Jelinek + 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. + * ipa-param-manipulation.c (ipa_param_body_adjustments::register_replacement): Fix comment typo - accross -> across. diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 23ec2424f9d..36c4566346a 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1888,17 +1888,23 @@ asan_decl_phase_3 (size_t i) } /* Ensure that variables in different stack protection phases conflict - so that they are not merged and share the same stack slot. */ + so that they are not merged and share the same stack slot. + Return true if there are any address taken variables. */ -static void +static bool add_stack_protection_conflicts (void) { size_t i, j, n = stack_vars_num; unsigned char *phase; + bool ret = false; phase = XNEWVEC (unsigned char, n); for (i = 0; i < n; ++i) - phase[i] = stack_protect_decl_phase (stack_vars[i].decl); + { + phase[i] = stack_protect_decl_phase (stack_vars[i].decl); + if (TREE_ADDRESSABLE (stack_vars[i].decl)) + ret = true; + } for (i = 0; i < n; ++i) { @@ -1909,6 +1915,7 @@ add_stack_protection_conflicts (void) } XDELETEVEC (phase); + return ret; } /* Create a decl for the guard at the top of the stack frame. */ @@ -1993,50 +2000,6 @@ estimated_stack_frame_size (struct cgraph_node *node) return estimated_poly_value (size); } -/* Helper routine to check if a record or union contains an array field. */ - -static int -record_or_union_type_has_array_p (const_tree tree_type) -{ - tree fields = TYPE_FIELDS (tree_type); - tree f; - - for (f = fields; f; f = DECL_CHAIN (f)) - if (TREE_CODE (f) == FIELD_DECL) - { - tree field_type = TREE_TYPE (f); - if (RECORD_OR_UNION_TYPE_P (field_type) - && record_or_union_type_has_array_p (field_type)) - return 1; - if (TREE_CODE (field_type) == ARRAY_TYPE) - return 1; - } - return 0; -} - -/* Check if the current function has local referenced variables that - have their addresses taken, contain an array, or are arrays. */ - -static bool -stack_protect_decl_p () -{ - unsigned i; - tree var; - - FOR_EACH_LOCAL_DECL (cfun, i, var) - if (!is_global_var (var)) - { - tree var_type = TREE_TYPE (var); - if (VAR_P (var) - && (TREE_CODE (var_type) == ARRAY_TYPE - || TREE_ADDRESSABLE (var) - || (RECORD_OR_UNION_TYPE_P (var_type) - && record_or_union_type_has_array_p (var_type)))) - return true; - } - return false; -} - /* Check if the current function has calls that use a return slot. */ static bool @@ -2103,8 +2066,7 @@ expand_used_vars (void) } if (flag_stack_protect == SPCT_FLAG_STRONG) - gen_stack_protect_signal - = stack_protect_decl_p () || stack_protect_return_slot_p (); + gen_stack_protect_signal = stack_protect_return_slot_p (); /* At this point all variables on the local_decls with TREE_USED set are not associated with any block scope. Lay them out. */ @@ -2180,6 +2142,8 @@ expand_used_vars (void) if (stack_vars_num > 0) { + bool has_addressable_vars = false; + add_scope_conflicts (); /* If stack protection is enabled, we don't share space between @@ -2189,7 +2153,10 @@ expand_used_vars (void) || (flag_stack_protect == SPCT_FLAG_EXPLICIT && lookup_attribute ("stack_protect", DECL_ATTRIBUTES (current_function_decl))))) - add_stack_protection_conflicts (); + has_addressable_vars = add_stack_protection_conflicts (); + + if (flag_stack_protect == SPCT_FLAG_STRONG && has_addressable_vars) + gen_stack_protect_signal = true; /* Now that we have collected all stack variables, and have computed a minimal interference graph, attempt to save some stack space. */ @@ -2206,14 +2173,16 @@ expand_used_vars (void) case SPCT_FLAG_STRONG: if (gen_stack_protect_signal - || cfun->calls_alloca || has_protected_decls + || cfun->calls_alloca + || has_protected_decls || lookup_attribute ("stack_protect", DECL_ATTRIBUTES (current_function_decl))) create_stack_guard (); break; case SPCT_FLAG_DEFAULT: - if (cfun->calls_alloca || has_protected_decls + if (cfun->calls_alloca + || has_protected_decls || lookup_attribute ("stack_protect", DECL_ATTRIBUTES (current_function_decl))) create_stack_guard (); @@ -2224,8 +2193,9 @@ expand_used_vars (void) DECL_ATTRIBUTES (current_function_decl))) create_stack_guard (); break; + default: - ; + break; } /* Assign rtl to each variable based on these partitions. */ diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index af3c7f2b910..236bed92724 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -13006,9 +13006,12 @@ on Intel Control-flow Enforcement Technology (CET). Emit extra code to check for buffer overflows, such as stack smashing attacks. This is done by adding a guard variable to functions with vulnerable objects. This includes functions that call @code{alloca}, and -functions with buffers larger than 8 bytes. The guards are initialized -when a function is entered and then checked when the function exits. -If a guard check fails, an error message is printed and the program exits. +functions with buffers larger than or equal to 8 bytes. The guards are +initialized when a function is entered and then checked when the function +exits. If a guard check fails, an error message is printed and the program +exits. Only variables that are actually allocated on the stack are +considered, optimized away variables or variables allocated in registers +don't count. @item -fstack-protector-all @opindex fstack-protector-all @@ -13018,7 +13021,9 @@ Like @option{-fstack-protector} except that all functions are protected. @opindex fstack-protector-strong Like @option{-fstack-protector} but includes additional functions to be protected --- those that have local array definitions, or have -references to local frame addresses. +references to local frame addresses. Only variables that are actually +allocated on the stack are considered, optimized away variables or variables +allocated in registers don't count. @item -fstack-protector-explicit @opindex fstack-protector-explicit diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 06ffc521bcf..1d520d6a3d2 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2019-12-10 Jakub Jelinek + + PR middle-end/92825 + * gcc.target/i386/pr92825.c: New test. + 2019-12-10 Martin Liska PR fortran/92874 diff --git a/gcc/testsuite/gcc.target/i386/pr92825.c b/gcc/testsuite/gcc.target/i386/pr92825.c new file mode 100644 index 00000000000..2c35eac433e --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr92825.c @@ -0,0 +1,15 @@ +/* PR middle-end/92825 */ +/* { dg-do compile { target fstack_protector } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ +/* { dg-final { scan-assembler-not "__stack_chk_fail" } } */ + +int +foo (int r, int g, int b) +{ + union U { int rgba; char p[4]; } u; + u.p[0] = r; + u.p[1] = g; + u.p[2] = b; + u.p[3] = -1; + return u.rgba; +} -- 2.30.2