Zero vptr in dtor for -fsanitize=vptr.

Hello.

This is small improvement that can catch a virtual call after a lifetime
scope of an object.


Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

gcc/cp/ChangeLog:

2017-10-27  Martin Liska  <mliska@suse.cz>

	* decl.c (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                        |  3 ++-
  gcc/testsuite/g++.dg/ubsan/vptr-12.C | 26 ++++++++++++++++++++++++++
  2 files changed, 28 insertions(+), 1 deletion(-)
  create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C

On Fri, Oct 27, 2017 at 12:47:12PM +0200, Martin Liška wrote:
> Hello.
> 
> This is small improvement that can catch a virtual call after a lifetime
> scope of an object.
> 
> 
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> 
> Ready to be installed?

The decl.c change seems to be only incremental change from a not publicly
posted patch rather than the full diff against trunk.

> 2017-10-27  Martin Liska  <mliska@suse.cz>
> 
> 	* decl.c (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                        |  3 ++-
>  gcc/testsuite/g++.dg/ubsan/vptr-12.C | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
> 
> 

> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 15a8d283353..69636e30008 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -15281,7 +15281,8 @@ begin_destructor_body (void)
>  	  /* Clobbering an empty base is harmful if it overlays real data.  */
>  	  && !is_empty_class (current_class_type))
>  	{
> -	  if (sanitize_flags_p (SANITIZE_VPTR))
> +	  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,

	Jakub

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.

Martin
From df0cc0c2da18b150b1f0fbef418450a223470d7f 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 (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                        | 14 +++++++++++++-
 gcc/testsuite/g++.dg/ubsan/vptr-12.C | 26 ++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 42b52748e2a..69636e30008 100644
--- 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));
+	      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 ()
+  {
+  }
+};
+
+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)" }
+

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?

> +	      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.

> +};
> +
> +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.

	Jakub

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)" }

On Fri, Oct 27, 2017 at 03:48:41PM +0200, Martin Liška wrote:
> --- 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)

I think build_clobber_this is better name, but will defer final review
to Jason or Nathan.  Also, seems there was already a function comment
and you've added yet another one, instead of ammending the first one.

Otherwise LGTM.

	Jakub

On Fri, Oct 27, 2017 at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Oct 27, 2017 at 03:48:41PM +0200, Martin Liška wrote:
>> --- 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)
>
> I think build_clobber_this is better name, but will defer final review
> to Jason or Nathan.  Also, seems there was already a function comment
> and you've added yet another one, instead of ammending the first one.

Agreed.

If the point is to clear the vptr, why are you also clearing the rest
of the object?

Jason

On Fri, Oct 27, 2017 at 02:10:10PM -0400, Jason Merrill wrote:
> On Fri, Oct 27, 2017 at 9:52 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Fri, Oct 27, 2017 at 03:48:41PM +0200, Martin Liška wrote:
> >> --- 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)
> >
> > I think build_clobber_this is better name, but will defer final review
> > to Jason or Nathan.  Also, seems there was already a function comment
> > and you've added yet another one, instead of ammending the first one.
> 
> Agreed.
> 
> If the point is to clear the vptr, why are you also clearing the rest
> of the object?

Can there be multiple vptr pointers in the object or is there just one?
Even if there can be multiple, perhaps earlier destructors would
have cleared those other vptr pointers though.

	Jakub

On 10/27/2017 02:18 PM, Jakub Jelinek wrote:
> On Fri, Oct 27, 2017 at 02:10:10PM -0400, Jason Merrill wrote:

>> If the point is to clear the vptr, why are you also clearing the rest
>> of the object?
> 
> Can there be multiple vptr pointers in the object or is there just one?
> Even if there can be multiple, perhaps earlier destructors would
> have cleared those other vptr pointers though.

There can be multiple vptrs in an object (multiple polymorphic bases). 
However, each such case will have its own base dtor invoked, as you 
postulated.  In fact, there may be a base dtor invoked that maps onto 
the single vptr, in the cases when we're singly inheriting a polymorphic 
base.

nathan

[polymorphic here includes the case of having virtual bases].

On Fri, Oct 27, 2017 at 02:30:39PM -0400, Nathan Sidwell wrote:
> On 10/27/2017 02:18 PM, Jakub Jelinek wrote:
> > On Fri, Oct 27, 2017 at 02:10:10PM -0400, Jason Merrill wrote:
> 
> > > If the point is to clear the vptr, why are you also clearing the rest
> > > of the object?
> > 
> > Can there be multiple vptr pointers in the object or is there just one?
> > Even if there can be multiple, perhaps earlier destructors would
> > have cleared those other vptr pointers though.
> 
> There can be multiple vptrs in an object (multiple polymorphic bases).
> However, each such case will have its own base dtor invoked, as you
> postulated.  In fact, there may be a base dtor invoked that maps onto the
> single vptr, in the cases when we're singly inheriting a polymorphic base.

But when singly inheriting a polymorphic base and thus mapped to the same
vptr all but the last dtor will not be in charge, right?
So, if using build_clobber_this for this, instead of clobbering what we
clobber we'd just clear the single vptr (couldn't clobber the rest, even
if before the store, because that would make the earlier other vptr stores
dead).

	Jakub

On 10/27/2017 02:34 PM, Jakub Jelinek wrote:

> But when singly inheriting a polymorphic base and thus mapped to the same
> vptr all but the last dtor will not be in charge, right?

Correct.

> So, if using build_clobber_this for this, instead of clobbering what we
> clobber we'd just clear the single vptr (couldn't clobber the rest, even
> if before the store, because that would make the earlier other vptr stores
> dead).

ok (I'd not looked at the patch to see if in chargeness was signficant)

nathan

On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
> 
>> But when singly inheriting a polymorphic base and thus mapped to the same
>> vptr all but the last dtor will not be in charge, right?
> 
> Correct.
> 
>> So, if using build_clobber_this for this, instead of clobbering what we
>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>> if before the store, because that would make the earlier other vptr stores
>> dead).
> 
> ok (I'd not looked at the patch to see if in chargeness was signficant)
> 
> nathan
> 

Hello.

I'm sending v2 which only zeros vptr of object.

Ready to be installed after finishing tests?
Martin
From 098932be5472656c834b402038accb0b861afcc1 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-11-03  Martin Liska  <mliska@suse.cz>

	* decl.c (begin_destructor_body): In case of VPTR sanitization
	(with disabled recovery), zero vptr in order to catch virtual calls
	after lifetime of an object.

gcc/testsuite/ChangeLog:

2017-10-27  Martin Liska  <mliska@suse.cz>

	* g++.dg/ubsan/vptr-12.C: New test.
---
 gcc/cp/decl.c                        | 20 +++++++++++++++++++-
 gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index d88c78f348b..d45cc29e636 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15241,7 +15241,25 @@ 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 binfo = TYPE_BINFO (current_class_type);
+	      tree ref
+		= cp_build_indirect_ref (current_class_ptr, RO_NULL,
+					 tf_warning_or_error);
+
+	      tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
+	      tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
+	      tree stmt = cp_build_modify_expr (input_location, vtbl_ptr,
+						NOP_EXPR, vtbl,
+						tf_warning_or_error);
+	      finish_decl_cleanup (NULL_TREE, stmt);
+	    }
+	  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..be5c074dfc1
--- /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:16:\[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)" }

On Fri, Nov 03, 2017 at 03:25:25PM +0100, Martin Liška wrote:
> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
> > On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
> > 
> >> But when singly inheriting a polymorphic base and thus mapped to the same
> >> vptr all but the last dtor will not be in charge, right?
> > 
> > Correct.
> > 
> >> So, if using build_clobber_this for this, instead of clobbering what we
> >> clobber we'd just clear the single vptr (couldn't clobber the rest, even
> >> if before the store, because that would make the earlier other vptr stores
> >> dead).
> > 
> > ok (I'd not looked at the patch to see if in chargeness was signficant)
> > 
> > nathan
> > 
> 
> Hello.
> 
> I'm sending v2 which only zeros vptr of object.
> 
> Ready to be installed after finishing tests?
> Martin

> From 098932be5472656c834b402038accb0b861afcc1 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-11-03  Martin Liska  <mliska@suse.cz>
> 
> 	* decl.c (begin_destructor_body): In case of VPTR sanitization
> 	(with disabled recovery), zero vptr in order to catch virtual calls
> 	after lifetime of an object.
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-10-27  Martin Liska  <mliska@suse.cz>
> 
> 	* g++.dg/ubsan/vptr-12.C: New test.
> ---
>  gcc/cp/decl.c                        | 20 +++++++++++++++++++-
>  gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C
> 
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index d88c78f348b..d45cc29e636 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -15241,7 +15241,25 @@ 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 binfo = TYPE_BINFO (current_class_type);
> +	      tree ref
> +		= cp_build_indirect_ref (current_class_ptr, RO_NULL,
> +					 tf_warning_or_error);
> +
> +	      tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
> +	      tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
> +	      tree stmt = cp_build_modify_expr (input_location, vtbl_ptr,
> +						NOP_EXPR, vtbl,
> +						tf_warning_or_error);
> +	      finish_decl_cleanup (NULL_TREE, stmt);
> +	    }
> +	  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..be5c074dfc1
> --- /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:16:\[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)" }

I think the last dg-output shouldn't have any regexps at the end, so:

// { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr" }

	Marek

On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>
>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>> vptr all but the last dtor will not be in charge, right?
>>
>> Correct.
>>
>>> So, if using build_clobber_this for this, instead of clobbering what we
>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>> if before the store, because that would make the earlier other vptr stores
>>> dead).
>>
>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>
>> nathan
>>
>
> Hello.
>
> I'm sending v2 which only zeros vptr of object.
>
> Ready to be installed after finishing tests?

Surely we also want to check TYPE_CONTAINS_VPTR_P.

Jason

On 11/03/2017 03:31 PM, Marek Polacek wrote:
> I think the last dg-output shouldn't have any regexps at the end, so:
> 
> // { dg-output "0x\[0-9a-fA-F]*: note: object has invalid vptr" }

Thanks for that.

I'll fix it.

Martin

On 11/03/2017 04:21 PM, Jason Merrill wrote:
> On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
>> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>>
>>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>>> vptr all but the last dtor will not be in charge, right?
>>>
>>> Correct.
>>>
>>>> So, if using build_clobber_this for this, instead of clobbering what we
>>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>>> if before the store, because that would make the earlier other vptr stores
>>>> dead).
>>>
>>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>>
>>> nathan
>>>
>>
>> Hello.
>>
>> I'm sending v2 which only zeros vptr of object.
>>
>> Ready to be installed after finishing tests?
> 
> Surely we also want to check TYPE_CONTAINS_VPTR_P.
> 
> Jason
> 

Done that in attached patch.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin
From 882becbc5446f304a9445281c6f778b80086ce39 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-11-03  Martin Liska  <mliska@suse.cz>

	* decl.c (begin_destructor_body): In case of VPTR sanitization
	(with disabled recovery), zero vptr in order to catch virtual calls
	after lifetime of an object.

gcc/testsuite/ChangeLog:

2017-10-27  Martin Liska  <mliska@suse.cz>

	* g++.dg/ubsan/vptr-12.C: New test.
---
 gcc/cp/decl.c                        | 21 ++++++++++++++++++++-
 gcc/testsuite/g++.dg/ubsan/vptr-12.C | 22 ++++++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/ubsan/vptr-12.C

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 0ce8f2d3435..48d2760afde 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15247,7 +15247,26 @@ 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
+	    && TYPE_CONTAINS_VPTR_P (current_class_type))
+	  {
+	    tree binfo = TYPE_BINFO (current_class_type);
+	    tree ref
+	      = cp_build_indirect_ref (current_class_ptr, RO_NULL,
+				       tf_warning_or_error);
+
+	    tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
+	    tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));
+	    tree stmt = cp_build_modify_expr (input_location, vtbl_ptr,
+					      NOP_EXPR, vtbl,
+					      tf_warning_or_error);
+	    finish_decl_cleanup (NULL_TREE, stmt);
+	  }
+	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..f23bbc3fd10
--- /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:16:\[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" }

PING^1

On 11/06/2017 09:27 AM, Martin Liška wrote:
> On 11/03/2017 04:21 PM, Jason Merrill wrote:
>> On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
>>> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>>>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>>>
>>>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>>>> vptr all but the last dtor will not be in charge, right?
>>>>
>>>> Correct.
>>>>
>>>>> So, if using build_clobber_this for this, instead of clobbering what we
>>>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>>>> if before the store, because that would make the earlier other vptr stores
>>>>> dead).
>>>>
>>>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>>>
>>>> nathan
>>>>
>>>
>>> Hello.
>>>
>>> I'm sending v2 which only zeros vptr of object.
>>>
>>> Ready to be installed after finishing tests?
>>
>> Surely we also want to check TYPE_CONTAINS_VPTR_P.
>>
>> Jason
>>
> 
> Done that in attached patch.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> 
> Ready to be installed?
> Martin
>

OK.

On Mon, Nov 6, 2017 at 3:27 AM, Martin Liška <mliska@suse.cz> wrote:
> On 11/03/2017 04:21 PM, Jason Merrill wrote:
>> On Fri, Nov 3, 2017 at 10:25 AM, Martin Liška <mliska@suse.cz> wrote:
>>> On 10/27/2017 09:44 PM, Nathan Sidwell wrote:
>>>> On 10/27/2017 02:34 PM, Jakub Jelinek wrote:
>>>>
>>>>> But when singly inheriting a polymorphic base and thus mapped to the same
>>>>> vptr all but the last dtor will not be in charge, right?
>>>>
>>>> Correct.
>>>>
>>>>> So, if using build_clobber_this for this, instead of clobbering what we
>>>>> clobber we'd just clear the single vptr (couldn't clobber the rest, even
>>>>> if before the store, because that would make the earlier other vptr stores
>>>>> dead).
>>>>
>>>> ok (I'd not looked at the patch to see if in chargeness was signficant)
>>>>
>>>> nathan
>>>>
>>>
>>> Hello.
>>>
>>> I'm sending v2 which only zeros vptr of object.
>>>
>>> Ready to be installed after finishing tests?
>>
>> Surely we also want to check TYPE_CONTAINS_VPTR_P.
>>
>> Jason
>>
>
> Done that in attached patch.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?
> Martin

Thanks for review. I actually noticed your introduction of
cp_build_fold_indirect_ref after I installed my patch.

I'm testing following fix.

Martin
From 63d9cff5c183f3614cff527ff991e1586a9efa5b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 15 Nov 2017 10:01:51 +0100
Subject: [PATCH] Fix fallout of -fsanitize=vptr.

gcc/cp/ChangeLog:

2017-11-15  Martin Liska  <mliska@suse.cz>

	* decl.c (begin_destructor_body): Use cp_build_fold_indirect_ref
	instead of cp_build_indirect_ref.
---
 gcc/cp/decl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 041893db937..7e16f7b415b 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -15253,8 +15253,7 @@ begin_destructor_body (void)
 	  {
 	    tree binfo = TYPE_BINFO (current_class_type);
 	    tree ref
-	      = cp_build_indirect_ref (current_class_ptr, RO_NULL,
-				       tf_warning_or_error);
+	      = cp_build_fold_indirect_ref (current_class_ptr);
 
 	    tree vtbl_ptr = build_vfield_ref (ref, TREE_TYPE (binfo));
 	    tree vtbl = build_zero_cst (TREE_TYPE (vtbl_ptr));