[C++ PATCH] Fix sanitization ICE (PR c++/80973)

[C++ PATCH] Fix sanitization ICE (PR c++/80973)

[Jakub Jelinek]

Hi!

cp_genericize_r now instruments INTEGER_CSTs that have REFERENCE_TYPE,
so that we can diagnose binding references to NULL in some cases,
see PR79572.  As the following testcase shows, there is one exception
when we do not want to do that - in MEM_EXPR, the second operand
is an INTEGER_CST whose value is an offset, but type is something
unrelated - what should be used for aliasing purposes.  So, that
is something we do not want to diagnose, and it is also invalid IL,
as the second argument has to be an INTEGER_CST, not some expression
with side-effects.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
ok for trunk/7.x?

2017-06-08  Jakub Jelinek  <jakub@redhat.com>

	PR c++/80973
	* cp-gimplify.c (cp_genericize_r): Don't instrument MEM_REF second
	argument even if it has REFERENCE_TYPE.

	* g++.dg/ubsan/pr80973.C: New test.

--- gcc/cp/cp-gimplify.c.jj	2017-06-08 13:24:48.000000000 +0200
+++ gcc/cp/cp-gimplify.c	2017-06-08 17:48:13.466868875 +0200
@@ -1476,6 +1476,21 @@ cp_genericize_r (tree *stmt_p, int *walk
 		cp_ubsan_maybe_instrument_member_call (stmt);
 	    }
 	}
+      else if (TREE_CODE (stmt) == MEM_REF)
+	{
+	  /* For MEM_REF, make sure not to sanitize the second operand even
+	     if it has reference type.  It is just an offset with a type
+	     holding other information.  */
+	  if (TREE_CODE (TREE_OPERAND (stmt, 1)) == INTEGER_CST
+	      && (TREE_CODE (TREE_TYPE (TREE_OPERAND (stmt, 1)))
+		  == REFERENCE_TYPE)
+	      && (flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT)))
+	    {
+	      cp_walk_tree (&TREE_OPERAND (stmt, 0), cp_genericize_r,
+			    data, NULL);
+	      *walk_subtrees = 0;
+	    }
+	}
     }
 
   p_set->add (*stmt_p);
--- gcc/testsuite/g++.dg/ubsan/pr80973.C.jj	2017-06-08 17:49:55.491622907 +0200
+++ gcc/testsuite/g++.dg/ubsan/pr80973.C	2017-06-08 17:49:51.056677069 +0200
@@ -0,0 +1,16 @@
+// PR c++/80973
+// { dg-do compile }
+// { dg-options "-fsanitize=undefined -std=c++14" }
+
+struct A {
+  A();
+  A(const A &);
+};
+struct B {
+  B();
+  template <typename... Args> auto g(Args &&... p1) {
+    return [=] { f(p1...); };
+  }
+  void f(A, const char *);
+};
+B::B() { g(A(), ""); }

	Jakub

Re: [C++ PATCH] Fix sanitization ICE (PR c++/80973)

[Jason Merrill]

On Thu, Jun 8, 2017 at 12:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> cp_genericize_r now instruments INTEGER_CSTs that have REFERENCE_TYPE,
> so that we can diagnose binding references to NULL in some cases,
> see PR79572.  As the following testcase shows, there is one exception
> when we do not want to do that - in MEM_EXPR, the second operand
> is an INTEGER_CST whose value is an offset, but type is something
> unrelated - what should be used for aliasing purposes.  So, that
> is something we do not want to diagnose, and it is also invalid IL,
> as the second argument has to be an INTEGER_CST, not some expression
> with side-effects.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
> ok for trunk/7.x?
>
>         PR c++/80973
>         * cp-gimplify.c (cp_genericize_r): Don't instrument MEM_REF second
>         argument even if it has REFERENCE_TYPE.

I wonder if we want to handle this in walk_tree_1, so all tree walks
by default avoid the second operand.

Jason

Re: [C++ PATCH] Fix sanitization ICE (PR c++/80973)

[Jakub Jelinek]

On Fri, Jun 09, 2017 at 12:30:10PM -0700, Jason Merrill wrote:
> On Thu, Jun 8, 2017 at 12:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > cp_genericize_r now instruments INTEGER_CSTs that have REFERENCE_TYPE,
> > so that we can diagnose binding references to NULL in some cases,
> > see PR79572.  As the following testcase shows, there is one exception
> > when we do not want to do that - in MEM_EXPR, the second operand
> > is an INTEGER_CST whose value is an offset, but type is something
> > unrelated - what should be used for aliasing purposes.  So, that
> > is something we do not want to diagnose, and it is also invalid IL,
> > as the second argument has to be an INTEGER_CST, not some expression
> > with side-effects.
> >
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
> > ok for trunk/7.x?
> >
> >         PR c++/80973
> >         * cp-gimplify.c (cp_genericize_r): Don't instrument MEM_REF second
> >         argument even if it has REFERENCE_TYPE.
> 
> I wonder if we want to handle this in walk_tree_1, so all tree walks
> by default avoid the second operand.

MEM_REF has the second argument and there could be callbacks that really
want to walk even that argument for whatever reason (gather all types
mentioned in some expression, whatever).  Implying from one place where
we do not want that what other code might want to do doesn't look right.

But, if you want, the patch could be simplified, handle the MEM_REF
in there this way unconditionally, so:
  else if (TREE_CODE (stmt) == MEM_REF)
    {
      cp_walk_tree (&TREE_OPERAND (stmt, 0), cp_genericize_r, data, NULL);
      *walk_subtrees = 0;
    }
before the sanitizer block, perhaps with some comments explaining it,
because we know cp_genericize_r doesn't need to have the callback
on the second MEM_REF argument.

	Jakub

Re: [C++ PATCH] Fix sanitization ICE (PR c++/80973)

[Jason Merrill]

On Mon, Jun 12, 2017 at 4:04 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jun 09, 2017 at 12:30:10PM -0700, Jason Merrill wrote:
>> On Thu, Jun 8, 2017 at 12:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > cp_genericize_r now instruments INTEGER_CSTs that have REFERENCE_TYPE,
>> > so that we can diagnose binding references to NULL in some cases,
>> > see PR79572.  As the following testcase shows, there is one exception
>> > when we do not want to do that - in MEM_EXPR, the second operand
>> > is an INTEGER_CST whose value is an offset, but type is something
>> > unrelated - what should be used for aliasing purposes.  So, that
>> > is something we do not want to diagnose, and it is also invalid IL,
>> > as the second argument has to be an INTEGER_CST, not some expression
>> > with side-effects.
>> >
>> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
>> > ok for trunk/7.x?
>> >
>> >         PR c++/80973
>> >         * cp-gimplify.c (cp_genericize_r): Don't instrument MEM_REF second
>> >         argument even if it has REFERENCE_TYPE.
>>
>> I wonder if we want to handle this in walk_tree_1, so all tree walks
>> by default avoid the second operand.
>
> MEM_REF has the second argument and there could be callbacks that really
> want to walk even that argument for whatever reason (gather all types
> mentioned in some expression, whatever).  Implying from one place where
> we do not want that what other code might want to do doesn't look right.

Well, such callbacks could walk it specifically, but...

> But, if you want, the patch could be simplified, handle the MEM_REF
> in there this way unconditionally, so:
>   else if (TREE_CODE (stmt) == MEM_REF)
>     {
>       cp_walk_tree (&TREE_OPERAND (stmt, 0), cp_genericize_r, data, NULL);
>       *walk_subtrees = 0;
>     }
> before the sanitizer block, perhaps with some comments explaining it,
> because we know cp_genericize_r doesn't need to have the callback
> on the second MEM_REF argument.

...this approach is fine, too.

Jason