This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Extend -fstack-protector-strong to cover calls with return slot
- From: Florian Weimer <fweimer at redhat dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, shenhan at google dot com
- Date: Wed, 08 Jan 2014 15:57:25 +0100
- Subject: Re: Extend -fstack-protector-strong to cover calls with return slot
- Authentication-results: sourceware.org; auth=none
- References: <52C6B807 dot 1070203 at redhat dot com> <20140103185715 dot GO892 at tucnak dot redhat dot com> <52C72F05 dot 2060901 at redhat dot com> <52CBF834 dot 3040004 at redhat dot com> <20140107130748 dot GP892 at tucnak dot redhat dot com> <52CC00A8 dot 7070203 at redhat dot com> <20140107133723 dot GR892 at tucnak dot redhat dot com>
On 01/07/2014 02:37 PM, Jakub Jelinek wrote:
On Tue, Jan 07, 2014 at 02:27:04PM +0100, Florian Weimer wrote:
gimplify_modify_expr_rhs, in the CALL_EXPR case:
if (use_target)
{
CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
mark_addressable (*to_p);
}
Yeah, that sets it in some cases too, not in other testcases.
Just look at how the flag is used when actually expanding it:
if (target && MEM_P (target) && CALL_EXPR_RETURN_SLOT_OPT (exp))
structure_value_addr = XEXP (target, 0);
else
{
/* For variable-sized objects, we must be called with a target
specified. If we were to allocate space on the stack here,
we would have no way of knowing when to free it. */
rtx d = assign_temp (rettype, 1, 1);
structure_value_addr = XEXP (d, 0);
target = 0;
}
Okay, I'm beginning to understand. I tried to actually reach the second
branch, and ended up with PR59711. :)
foo12 in the new C testcase covers it in part without a variable-sized
object.
so, if it is set, the address of the var on the LHS is passed to the
function as hidden argument, if it is not set, we pass address of
a stack temporary instead. Both the automatic var and the stack temporary
can overflow, if the callee does something wrong.
What about the attached version? It still does not exactly match your
original suggestion because gimple_call_lhs (stmt) can be NULL_TREE if
the result is ignored and this case needs instrumentation, as you
explained, so I use the function return type in the aggregate_value_p check.
Testing is still under way, but looks good so far. I'm bootstrapping
with BOOT_CFLAGS="-O2 -g -fstack-protector-strong" with Ada enabled, for
additional coverage.
--
Florian Weimer / Red Hat Product Security Team
gcc/
2014-01-08 Florian Weimer <fweimer@redhat.com>
* cfgexpand.c (stack_protect_decl_p): New function, extracted from
expand_used_vars.
(stack_protect_return_slot_p): New function.
(expand_used_vars): Call stack_protect_decl_p and
stack_protect_return_slot_p for -fstack-protector-strong.
gcc/testsuite/
2014-01-08 Florian Weimer <fweimer@redhat.com>
* gcc.dg/fstack-protector-strong.c: Add coverage for return slots.
* g++.dg/fstack-protector-strong.C: Likewise.
* gcc.target/i386/ssp-strong-reg.c: New file.
Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c (revision 206311)
+++ gcc/cfgexpand.c (working copy)
@@ -1599,6 +1599,52 @@
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 (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))))
+ return true;
+ }
+ return false;
+}
+
+/* Check if the current function has calls that use a return slot. */
+
+static bool
+stack_protect_return_slot_p ()
+{
+ basic_block bb;
+
+ FOR_ALL_BB_FN (bb, cfun)
+ for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
+ !gsi_end_p (gsi); gsi_next (&gsi))
+ {
+ gimple stmt = gsi_stmt (gsi);
+ /* This assumes that calls to internal-only functions never
+ use a return slot. */
+ if (is_gimple_call (stmt)
+ && !gimple_call_internal_p (stmt)
+ && aggregate_value_p (TREE_TYPE (gimple_call_fntype (stmt)),
+ gimple_call_fndecl (stmt)))
+ return true;
+ }
+ return false;
+}
+
/* Expand all variables used in the function. */
static rtx
@@ -1669,22 +1715,8 @@
pointer_map_destroy (ssa_name_decls);
if (flag_stack_protect == SPCT_FLAG_STRONG)
- FOR_EACH_LOCAL_DECL (cfun, i, var)
- 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 = true;
- break;
- }
- }
+ gen_stack_protect_signal
+ = stack_protect_decl_p () || 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. */
Index: gcc/testsuite/g++.dg/fstack-protector-strong.C
===================================================================
--- gcc/testsuite/g++.dg/fstack-protector-strong.C (revision 206311)
+++ gcc/testsuite/g++.dg/fstack-protector-strong.C (working copy)
@@ -32,4 +32,52 @@
return global_func (a);
}
-/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */
+/* Frame addressed exposed through return slot. */
+
+struct B
+{
+ /* Discourage passing this struct in registers. */
+ int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+ int method ();
+ B return_slot();
+};
+
+B global_func ();
+void noop ();
+
+int foo3 ()
+{
+ return global_func ().a1;
+}
+
+int foo4 ()
+{
+ try {
+ noop ();
+ return 0;
+ } catch (...) {
+ return global_func ().a1;
+ }
+}
+
+int foo5 ()
+{
+ try {
+ return global_func ().a1;
+ } catch (...) {
+ return 0;
+ }
+}
+
+int foo6 ()
+{
+ B b;
+ return b.method ();
+}
+
+int foo7 (B *p)
+{
+ return p->return_slot ().a1;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 7 } } */
Index: gcc/testsuite/gcc.dg/fstack-protector-strong.c
===================================================================
--- gcc/testsuite/gcc.dg/fstack-protector-strong.c (revision 206311)
+++ gcc/testsuite/gcc.dg/fstack-protector-strong.c (working copy)
@@ -131,4 +131,22 @@
return bb.three;
}
-/* { dg-final { scan-assembler-times "stack_chk_fail" 10 } } */
+struct B
+{
+ /* Discourage passing this struct in registers. */
+ int a1, a2, a3, a4, a5, a6, a7, a8, a9, a10;
+};
+
+struct B global3 (void);
+
+int foo11 ()
+{
+ return global3 ().a1;
+}
+
+void foo12 ()
+{
+ global3 ();
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 12 } } */
Index: gcc/testsuite/gcc.target/i386/ssp-strong-reg.c
===================================================================
--- gcc/testsuite/gcc.target/i386/ssp-strong-reg.c (revision 0)
+++ gcc/testsuite/gcc.target/i386/ssp-strong-reg.c (working copy)
@@ -0,0 +1,19 @@
+/* Test that structs returned in registers do not lead to
+ instrumentation with -fstack-protector-strong. */
+
+/* { dg-do compile { target { ! { ia32 } } } } */
+/* { dg-options "-O2 -fstack-protector-strong" } */
+
+struct S {
+ int a;
+ int b;
+};
+
+struct S f (void);
+
+int g (void)
+{
+ return f ().a;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 0 } } */