Hi, David and Rong, thanks a lot! Modified code uploaded as patch 8 and is also included at the end of email body.
Ref - http://codereview.appspot.com/5461043 Regards, -Han ====== Patch start diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 6d31e90..131c1b9 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1524,15 +1524,39 @@ estimated_stack_frame_size (struct cgraph_node *node) return 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)) + return record_or_union_type_has_array_p (field_type); + if (TREE_CODE (field_type) == ARRAY_TYPE) + return 1; + } + } + return 0; +} + /* Expand all variables used in the function. */ static void expand_used_vars (void) { tree var, outer_block = DECL_INITIAL (current_function_decl); + referenced_var_iterator rvi; VEC(tree,heap) *maybe_local_decls = NULL; unsigned i; unsigned len; + int gen_stack_protect_signal = 0; /* Compute the phase of the stack frame for this function. */ { @@ -1565,6 +1589,23 @@ expand_used_vars (void) } } + FOR_EACH_REFERENCED_VAR (cfun, var, rvi) + if (!is_global_var (var)) + { + tree var_type = TREE_TYPE (var); + /* Examine local referenced variables that have their addresses taken, + contain an array, or are arrays. */ + if (TREE_CODE (var) == VAR_DECL + && (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)))) + { + ++gen_stack_protect_signal; + break; + } + } + /* At this point all variables on the local_decls with TREE_USED set are not associated with any block scope. Lay them out. */ @@ -1655,11 +1696,18 @@ expand_used_vars (void) dump_stack_var_partition (); } - /* There are several conditions under which we should create a - stack guard: protect-all, alloca used, protected decls present. */ - if (flag_stack_protect == 2 - || (flag_stack_protect - && (cfun->calls_alloca || has_protected_decls))) + /* Create stack guard, if + a) "-fstack-protector-all" - always; + b) "-fstack-protector-strong" - if there are arrays, memory + references to local variables, alloca used, or protected decls present; + c) "-fstack-protector" - if alloca used, or protected decls present */ + if (flag_stack_protect == 3 /* -fstack-protector-all */ + || (flag_stack_protect == 2 /* -fstack-protector-strong */ + && (gen_stack_protect_signal || cfun->calls_alloca + || has_protected_decls)) + || (flag_stack_protector == 1 /* -fstack-protector */ + && (cfun->calls_alloca + || has_protected_decls))) create_stack_guard (); /* Assign rtl to each variable based on these partitions. */ diff --git a/gcc/common.opt b/gcc/common.opt index ec1dbd1..b79b8cc 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1835,8 +1835,12 @@ fstack-protector Common Report Var(flag_stack_protect, 1) Use propolice as a stack protection method -fstack-protector-all +fstack-protector-strong Common Report RejectNegative Var(flag_stack_protect, 2) +Use a smart stack protection method for certain functions + +fstack-protector-all +Common Report RejectNegative Var(flag_stack_protect, 3) Use a stack protection method for every function fstack-usage diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index e3d3789..607a7a5 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -400,8 +400,8 @@ Objective-C and Objective-C++ Dialects}. -fsel-sched-pipelining -fsel-sched-pipelining-outer-loops @gol -fshrink-wrap -fsignaling-nans -fsingle-precision-constant @gol -fsplit-ivs-in-unroller -fsplit-wide-types -fstack-protector @gol --fstack-protector-all -fstrict-aliasing -fstrict-overflow @gol --fthread-jumps -ftracer -ftree-bit-ccp @gol +-fstack-protector-strong -fstack-protector-all -fstrict-aliasing @gol +-fstrict-overflow -fthread-jumps -ftracer -ftree-bit-ccp @gol -ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-copy-prop @gol -ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol -ftree-forwprop -ftree-fre -ftree-loop-if-convert @gol @@ -8443,6 +8443,12 @@ 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. +@item -fstack-protector-strong +@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. + @item -fstack-protector-all @opindex fstack-protector-all Like @option{-fstack-protector} except that all functions are protected. diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C b/gcc/testsuite/g++.dg/fstack-protector-strong.C new file mode 100644 index 0000000..a4f0f81 --- /dev/null +++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C @@ -0,0 +1,35 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +class A +{ +public: + A() {} + ~A() {} + void method(); + int state; +}; + +/* Frame address exposed to A::method via "this". */ +int +foo1 () +{ + A a; + a.method (); + return a.state; +} + +/* Possible destroying foo2's stack via &a. */ +int +global_func (A& a); + +/* Frame address exposed to global_func. */ +int foo2 () +{ + A a; + return global_func (a); +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */ diff --git a/gcc/testsuite/gcc.dg/fstack-protector-strong.c b/gcc/testsuite/gcc.dg/fstack-protector-strong.c new file mode 100644 index 0000000..5a5cf98 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c @@ -0,0 +1,135 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +#include<string.h> +#include<stdlib.h> + +extern int g0; +extern int* pg0; +int +goo (int *); +int +hoo (int); + +/* Function frame address escaped function call. */ +int +foo1 () +{ + int i; + return goo (&i); +} + +struct ArrayStruct +{ + int a; + int array[10]; +}; + +struct AA +{ + int b; + struct ArrayStruct as; +}; + +/* Function frame contains array. */ +int +foo2 () +{ + struct AA aa; + int i; + for (i = 0; i < 10; ++i) + { + aa.as.array[i] = i * (i-1) + i / 2; + } + return aa.as.array[5]; +} + +/* Address computation based on a function frame address. */ +int +foo3 () +{ + int a; + int *p; + p = &a + 5; + return goo (p); +} + +/* Address cast based on a function frame address. */ +int +foo4 () +{ + int a; + return goo (g0 << 2 ? (int *)(3 * (long)(void *)(&a)) : 0); +} + +/* Address cast based on a local array. */ +int +foo5 () +{ + short array[10]; + return goo ((int *)(array + 5)); +} + +struct BB +{ + int one; + int two; + int three; +}; + +/* Address computaton based on a function frame address.*/ +int +foo6 () +{ + struct BB bb; + return goo (&bb.one + sizeof(int)); +} + +/* Function frame address escaped via global variable. */ +int +foo7 () +{ + int a; + pg0 = &a; + goo (pg0); + return *pg0; +} + +/* Check that this covers -fstack-protector. */ +int +foo8 () +{ + char base[100]; + memcpy ((void *)base, (const void *)pg0, 105); + return (int)(base[32]); +} + +/* Check that this covers -fstack-protector. */ +int +foo9 () +{ + char* p = alloca (100); + return goo ((int *)(p + 50)); +} + +int +global2 (struct BB* pbb); + +/* Address taken on struct. */ +int +foo10 () +{ + struct BB bb; + int i; + bb.one = global2 (&bb); + for (i = 0; i < 10; ++i) + { + bb.two = bb.one + bb.two; + bb.three = bb.one + bb.two + bb.three; + } + return bb.three; +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */ ===== patch end On Tue, Jan 24, 2012 at 2:16 PM, <davi...@google.com> wrote: > > Also need to update doc/invoke.texi file for the new option. > > > > > > http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c > File gcc/cfgexpand.c (right): > > http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1531 > gcc/cfgexpand.c:1531: record_or_union_type_has_array (const_tree > tree_type) > Better add '_p' suffix to the predicate function name. > > http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1535 > gcc/cfgexpand.c:1535: for (f = fields; f; f = DECL_CHAIN (f)) > Add an empty line after declarations. > > http://codereview.appspot.com/5461043/diff/16001/gcc/cfgexpand.c#newcode1702diff > --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 6d31e90..131c1b9 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1524,15 +1524,39 @@ estimated_stack_frame_size (struct cgraph_node *node) return 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)) + return record_or_union_type_has_array_p (field_type); + if (TREE_CODE (field_type) == ARRAY_TYPE) + return 1; + } + } + return 0; +} + /* Expand all variables used in the function. */ static void expand_used_vars (void) { tree var, outer_block = DECL_INITIAL (current_function_decl); + referenced_var_iterator rvi; VEC(tree,heap) *maybe_local_decls = NULL; unsigned i; unsigned len; + int gen_stack_protect_signal = 0; /* Compute the phase of the stack frame for this function. */ { @@ -1565,6 +1589,23 @@ expand_used_vars (void) } } + FOR_EACH_REFERENCED_VAR (cfun, var, rvi) + if (!is_global_var (var)) + { + tree var_type = TREE_TYPE (var); + /* Examine local referenced variables that have their addresses taken, + contain an array, or are arrays. */ + if (TREE_CODE (var) == VAR_DECL + && (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)))) + { + ++gen_stack_protect_signal; + break; + } + } + /* At this point all variables on the local_decls with TREE_USED set are not associated with any block scope. Lay them out. */ @@ -1655,11 +1696,18 @@ expand_used_vars (void) dump_stack_var_partition (); } - /* There are several conditions under which we should create a - stack guard: protect-all, alloca used, protected decls present. */ - if (flag_stack_protect == 2 - || (flag_stack_protect - && (cfun->calls_alloca || has_protected_decls))) + /* Create stack guard, if + a) "-fstack-protector-all" - always; + b) "-fstack-protector-strong" - if there are arrays, memory + references to local variables, alloca used, or protected decls present; + c) "-fstack-protector" - if alloca used, or protected decls present */ + if (flag_stack_protect == 3 /* -fstack-protector-all */ + || (flag_stack_protect == 2 /* -fstack-protector-strong */ + && (gen_stack_protect_signal || cfun->calls_alloca + || has_protected_decls)) + || (flag_stack_protector == 1 /* -fstack-protector */ + && (cfun->calls_alloca + || has_protected_decls))) create_stack_guard (); /* Assign rtl to each variable based on these partitions. */ diff --git a/gcc/common.opt b/gcc/common.opt index ec1dbd1..b79b8cc 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1835,8 +1835,12 @@ fstack-protector Common Report Var(flag_stack_protect, 1) Use propolice as a stack protection method -fstack-protector-all +fstack-protector-strong Common Report RejectNegative Var(flag_stack_protect, 2) +Use a smart stack protection method for certain functions + +fstack-protector-all +Common Report RejectNegative Var(flag_stack_protect, 3) Use a stack protection method for every function fstack-usage diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index e3d3789..607a7a5 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -400,8 +400,8 @@ Objective-C and Objective-C++ Dialects}. -fsel-sched-pipelining -fsel-sched-pipelining-outer-loops @gol -fshrink-wrap -fsignaling-nans -fsingle-precision-constant @gol -fsplit-ivs-in-unroller -fsplit-wide-types -fstack-protector @gol --fstack-protector-all -fstrict-aliasing -fstrict-overflow @gol --fthread-jumps -ftracer -ftree-bit-ccp @gol +-fstack-protector-strong -fstack-protector-all -fstrict-aliasing @gol +-fstrict-overflow -fthread-jumps -ftracer -ftree-bit-ccp @gol -ftree-builtin-call-dce -ftree-ccp -ftree-ch -ftree-copy-prop @gol -ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol -ftree-forwprop -ftree-fre -ftree-loop-if-convert @gol @@ -8443,6 +8443,12 @@ 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. +@item -fstack-protector-strong +@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. + @item -fstack-protector-all @opindex fstack-protector-all Like @option{-fstack-protector} except that all functions are protected. diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C b/gcc/testsuite/g++.dg/fstack-protector-strong.C new file mode 100644 index 0000000..a4f0f81 --- /dev/null +++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C @@ -0,0 +1,35 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +class A +{ +public: + A() {} + ~A() {} + void method(); + int state; +}; + +/* Frame address exposed to A::method via "this". */ +int +foo1 () +{ + A a; + a.method (); + return a.state; +} + +/* Possible destroying foo2's stack via &a. */ +int +global_func (A& a); + +/* Frame address exposed to global_func. */ +int foo2 () +{ + A a; + return global_func (a); +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */ diff --git a/gcc/testsuite/gcc.dg/fstack-protector-strong.c b/gcc/testsuite/gcc.dg/fstack-protector-strong.c new file mode 100644 index 0000000..5a5cf98 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c @@ -0,0 +1,135 @@ +/* Test that stack protection is done on chosen functions. */ + +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-O2 -fstack-protector-strong" } */ + +#include<string.h> +#include<stdlib.h> + +extern int g0; +extern int* pg0; +int +goo (int *); +int +hoo (int); + +/* Function frame address escaped function call. */ +int +foo1 () +{ + int i; + return goo (&i); +} + +struct ArrayStruct +{ + int a; + int array[10]; +}; + +struct AA +{ + int b; + struct ArrayStruct as; +}; + +/* Function frame contains array. */ +int +foo2 () +{ + struct AA aa; + int i; + for (i = 0; i < 10; ++i) + { + aa.as.array[i] = i * (i-1) + i / 2; + } + return aa.as.array[5]; +} + +/* Address computation based on a function frame address. */ +int +foo3 () +{ + int a; + int *p; + p = &a + 5; + return goo (p); +} + +/* Address cast based on a function frame address. */ +int +foo4 () +{ + int a; + return goo (g0 << 2 ? (int *)(3 * (long)(void *)(&a)) : 0); +} + +/* Address cast based on a local array. */ +int +foo5 () +{ + short array[10]; + return goo ((int *)(array + 5)); +} + +struct BB +{ + int one; + int two; + int three; +}; + +/* Address computaton based on a function frame address.*/ +int +foo6 () +{ + struct BB bb; + return goo (&bb.one + sizeof(int)); +} + +/* Function frame address escaped via global variable. */ +int +foo7 () +{ + int a; + pg0 = &a; + goo (pg0); + return *pg0; +} + +/* Check that this covers -fstack-protector. */ +int +foo8 () +{ + char base[100]; + memcpy ((void *)base, (const void *)pg0, 105); + return (int)(base[32]); +} + +/* Check that this covers -fstack-protector. */ +int +foo9 () +{ + char* p = alloca (100); + return goo ((int *)(p + 50)); +} + +int +global2 (struct BB* pbb); + +/* Address taken on struct. */ +int +foo10 () +{ + struct BB bb; + int i; + bb.one = global2 (&bb); + for (i = 0; i < 10; ++i) + { + bb.two = bb.one + bb.two; + bb.three = bb.one + bb.two + bb.three; + } + return bb.three; +} + +/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */ > gcc/cfgexpand.c:1702: if (flag_stack_protect == 2 > Add more descriptions. Better yet, fix the flag value mapping -- > protect_all-> 3, protect --> 2, and protect_strong-->1 > > http://codereview.appspot.com/5461043/