This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
- From: Martin Liška <mliska at suse dot cz>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Jason Merrill <jason at redhat dot com>
- Date: Fri, 27 Oct 2017 15:48:41 +0200
- Subject: Re: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
- Authentication-results: sourceware.org; auth=none
- References: <1d468e04-9f25-65f4-04a1-51b35abb3582@suse.cz> <20171027105212.GC14653@tucnak> <63fcd2b6-44d0-cb4b-d518-99eb496c1686@suse.cz> <20171027112600.GD14653@tucnak>
On 10/27/2017 01:26 PM, Jakub Jelinek wrote:
On Fri, Oct 27, 2017 at 01:16:08PM +0200, Martin Liška wrote:
On 10/27/2017 12:52 PM, Jakub Jelinek wrote:
The decl.c change seems to be only incremental change from a not publicly
posted patch rather than the full diff against trunk.
Sorry for that. Sending full patch.
Thanks.
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15280,7 +15280,19 @@ begin_destructor_body (void)
if (flag_lifetime_dse
/* Clobbering an empty base is harmful if it overlays real data. */
&& !is_empty_class (current_class_type))
- finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ {
+ if (sanitize_flags_p (SANITIZE_VPTR)
+ && (flag_sanitize_recover & SANITIZE_VPTR) == 0)
+ {
+ tree fndecl = builtin_decl_explicit (BUILT_IN_MEMSET);
+ tree call = build_call_expr (fndecl, 3,
+ current_class_ptr, integer_zero_node,
+ TYPE_SIZE_UNIT (current_class_type));
I wonder if it wouldn't be cheaper to just use thisref = {}; rather than
memset, pretty much the same thing as build_clobber_this () emits, except
for the TREE_VOLATILE. Also, build_clobber_this has:
if (vbases)
exprstmt = build_if_in_charge (exprstmt);
so it doesn't clobber if not in charge, not sure if it applies here too.
So maybe easiest would be add a bool argument to build_clobber_this which
would say whether it is a clobber or real clearing?
Hello.
Did that in newer version of the patch, good idea!
+ finish_decl_cleanup (NULL_TREE, call);
+ }
+ else
+ finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ }
/* And insert cleanups for our bases and members so that they
will be properly destroyed if we throw. */
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
new file mode 100644
index 00000000000..96c8473d757
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
@@ -0,0 +1,26 @@
+// { dg-do run }
+// { dg-shouldfail "ubsan" }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+struct MyClass
+{
+ virtual ~MyClass () {}
+ virtual void
+ Doit ()
+ {
+ }
Why not put all the above 4 lines into a single one, the dtor already uses
that kind of formatting.
Sure.
+};
+
+int
+main ()
+{
+ MyClass *c = new MyClass;
+ c->~MyClass ();
+ c->Doit ();
+
+ return 0;
+}
+
+// { dg-output "\[^\n\r]*vptr-12.C:19:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
+// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
+
Unnecessary empty line at end.
Likewise.
Martin
Jakub
>From b1da5f4de8b630f284627f422b902d28cd1d408b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 19 Oct 2017 11:10:19 +0200
Subject: [PATCH] Zero vptr in dtor for -fsanitize=vptr.
gcc/cp/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* decl.c (build_clobber_this): Rename to ...
(build_this_constructor): ... this. Add argument clobber_p.
(start_preparsed_function): Use the argument.
(begin_destructor_body): In case of disabled recovery,
we can zero object in order to catch virtual calls after
an object lifetime.
gcc/testsuite/ChangeLog:
2017-10-27 Martin Liska <mliska@suse.cz>
* g++.dg/ubsan/vptr-12.C: New test.
---
gcc/cp/decl.c | 18 ++++++++++++++----
gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
2 files changed, 36 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 519aa06a0f9..ee48d1c157e 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -14639,8 +14639,12 @@ implicit_default_ctor_p (tree fn)
/* Clobber the contents of *this to let the back end know that the object
storage is dead when we enter the constructor or leave the destructor. */
+/* Clobber or zero (depending on CLOBBER_P argument) the contents of *this
+ to let the back end know that the object storage is dead
+ when we enter the constructor or leave the destructor. */
+
static tree
-build_clobber_this ()
+build_this_constructor (bool clobber_p)
{
/* Clobbering an empty base is pointless, and harmful if its one byte
TYPE_SIZE overlays real data. */
@@ -14657,7 +14661,9 @@ build_clobber_this ()
ctype = CLASSTYPE_AS_BASE (ctype);
tree clobber = build_constructor (ctype, NULL);
- TREE_THIS_VOLATILE (clobber) = true;
+
+ if (clobber_p)
+ TREE_THIS_VOLATILE (clobber) = true;
tree thisref = current_class_ref;
if (ctype != current_class_type)
@@ -15086,7 +15092,7 @@ start_preparsed_function (tree decl1, tree attrs, int flags)
because part of the initialization might happen before we enter the
constructor, via AGGR_INIT_ZERO_FIRST (c++/68006). */
&& !implicit_default_ctor_p (decl1))
- finish_expr_stmt (build_clobber_this ());
+ finish_expr_stmt (build_this_constructor (true));
if (!processing_template_decl
&& DECL_CONSTRUCTOR_P (decl1)
@@ -15301,7 +15307,11 @@ begin_destructor_body (void)
if (flag_lifetime_dse
/* Clobbering an empty base is harmful if it overlays real data. */
&& !is_empty_class (current_class_type))
- finish_decl_cleanup (NULL_TREE, build_clobber_this ());
+ {
+ bool s = (sanitize_flags_p (SANITIZE_VPTR)
+ && (flag_sanitize_recover & SANITIZE_VPTR) == 0);
+ finish_decl_cleanup (NULL_TREE, build_this_constructor (!s));
+ }
/* And insert cleanups for our bases and members so that they
will be properly destroyed if we throw. */
diff --git a/gcc/testsuite/g++.dg/ubsan/vptr-12.C b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
new file mode 100644
index 00000000000..51b9d36d3f2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/vptr-12.C
@@ -0,0 +1,22 @@
+// { dg-do run }
+// { dg-shouldfail "ubsan" }
+// { dg-options "-fsanitize=vptr -fno-sanitize-recover=vptr" }
+
+struct MyClass
+{
+ virtual ~MyClass () {}
+ virtual void Doit () {}
+};
+
+int
+main ()
+{
+ MyClass *c = new MyClass;
+ c->~MyClass ();
+ c->Doit ();
+
+ return 0;
+}
+
+// { dg-output "\[^\n\r]*vptr-12.C:19:\[0-9]*: runtime error: member call on address 0x\[0-9a-fA-F]* which does not point to an object of type 'MyClass'(\n|\r\n|\r)" }
+// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr(\n|\r\n|\r)" }
--
2.14.2