diff mbox

[C++] Fix ubsan downcast -fsanitize=vptr sanitization (PR c++/68508)

Message ID 20151124210150.GT5675@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Nov. 24, 2015, 9:01 p.m. UTC
Hi!

Since the introduction of -fsanitize=vptr we ICE on the attached testcase,
because for -std=c++14 the FE has strict assumptions on what the trees from
force_paren_expr should look like, but with -fsanitize=vptr there is extra
sanitization (COMPOUND_EXPR with UBSAN_VPTR call and the ADDR_EXPR it is
looking for), thus I wrote the second (for now untested) patch.

But then got surprised that we actually sanitize the A * to A & static
cast at all, I believe we ought to only sanitize downcasts, but:
1) in the second build_static_cast_1 caller of
cp_ubsan_maybe_instrument_downcast we call build_base_path which apparently
already converts the intype to type, so cp_ubsan_maybe_instrument_downcast
then doesn't know if it is a downcast or not
2) the downcast check is just DERIVED_FROM_P check, but my undestanding
of that is that DERIVED_FROM_P (x, x) is true too
So, this first included (non-attached) patch is my attempt to only sanitize
real downcasts.  This patch has been bootstrapped/regtested on x86_64-linux
and i686-linux on the trunk, ok for trunk?

If yes, is the second patch better for the branch?

2015-11-24  Jakub Jelinek  <jakub@redhat.com>

	PR c++/68508
	* cp-tree.h (cp_ubsan_maybe_instrument_downcast): Add INTYPE argument.
	* cp-ubsan.c (cp_ubsan_maybe_instrument_downcast): Likewise.  Use
	it instead of or in addition to TREE_TYPE (op).  Return NULL_TREE if
	TREE_TYPE (intype) and TREE_TYPE (type) are the same type.
	* typeck.c (build_static_cast_1): Adjust callers.

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


	Jakub
2015-11-24  Jakub Jelinek  <jakub@redhat.com>

	PR c++/68508
	* typeck.c (check_return_expr): Strip away also
	cp_ubsan_maybe_instrument_downcast added instrumentation.

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

--- gcc/cp/typeck.c.jj	2015-11-20 08:17:50.000000000 +0100
+++ gcc/cp/typeck.c	2015-11-24 13:46:37.618746306 +0100
@@ -8802,9 +8804,23 @@ check_return_expr (tree retval, bool *no
 	  && REF_PARENTHESIZED_P (retval))
 	{
 	  retval = TREE_OPERAND (retval, 0);
-	  while (TREE_CODE (retval) == NON_LVALUE_EXPR
-		 || TREE_CODE (retval) == NOP_EXPR)
-	    retval = TREE_OPERAND (retval, 0);
+	  do
+	    {
+	      if (TREE_CODE (retval) == NON_LVALUE_EXPR
+		  || TREE_CODE (retval) == NOP_EXPR)
+		retval = TREE_OPERAND (retval, 0);
+	      /* Also look through cp_ubsan_maybe_instrument_downcast
+		 instrumentation.  */
+	      else if (TREE_CODE (retval) == COMPOUND_EXPR
+		       && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR
+		       && CALL_EXPR_FN (TREE_OPERAND (retval, 0)) == NULL_TREE
+		       && (CALL_EXPR_IFN (TREE_OPERAND (retval, 0))
+			   == IFN_UBSAN_VPTR))
+		retval = TREE_OPERAND (retval, 1);
+	      else
+		break;
+	    }
+	  while (1);
 	  gcc_assert (TREE_CODE (retval) == ADDR_EXPR);
 	  retval = TREE_OPERAND (retval, 0);
 	}
--- gcc/testsuite/g++.dg/ubsan/pr68508.C.jj	2015-11-24 13:58:27.506683395 +0100
+++ gcc/testsuite/g++.dg/ubsan/pr68508.C	2015-11-24 13:53:41.000000000 +0100
@@ -0,0 +1,15 @@
+// PR c++/68508
+// { dg-do compile }
+// { dg-options "-std=c++14 -fsanitize=vptr" }
+
+struct A
+{
+  virtual int foo () { return 0; }
+};
+
+const A &
+bar ()
+{
+  static A x = A ();
+  return (x);
+}

Comments

Jason Merrill Nov. 25, 2015, 2:45 p.m. UTC | #1
On 11/24/2015 04:01 PM, Jakub Jelinek wrote:
> 2) the downcast check is just DERIVED_FROM_P check, but my undestanding
> of that is that DERIVED_FROM_P (x, x) is true too

Yes, but you can use is_properly_derived_from instead.

With that change the first cast is OK for trunk and branch.

Jason
diff mbox

Patch

--- gcc/cp/cp-tree.h.jj	2015-11-20 09:52:25.000000000 +0100
+++ gcc/cp/cp-tree.h	2015-11-24 13:42:30.096194615 +0100
@@ -6854,7 +6854,7 @@  extern bool cilk_valid_spawn
 /* In cp-ubsan.c */
 extern void cp_ubsan_maybe_instrument_member_call (tree);
 extern void cp_ubsan_instrument_member_accesses (tree *);
-extern tree cp_ubsan_maybe_instrument_downcast	(location_t, tree, tree);
+extern tree cp_ubsan_maybe_instrument_downcast	(location_t, tree, tree, tree);
 extern tree cp_ubsan_maybe_instrument_cast_to_vbase (location_t, tree, tree);
 
 /* -- end of C++ */
--- gcc/cp/cp-ubsan.c.jj	2015-11-14 19:35:53.000000000 +0100
+++ gcc/cp/cp-ubsan.c	2015-11-24 13:45:12.837927413 +0100
@@ -245,13 +245,18 @@  cp_ubsan_instrument_member_accesses (tre
 /* Instrument downcast.  */
 
 tree
-cp_ubsan_maybe_instrument_downcast (location_t loc, tree type, tree op)
+cp_ubsan_maybe_instrument_downcast (location_t loc, tree type,
+				    tree intype, tree op)
 {
   if (!POINTER_TYPE_P (type)
+      || !POINTER_TYPE_P (intype)
       || !POINTER_TYPE_P (TREE_TYPE (op))
       || !CLASS_TYPE_P (TREE_TYPE (type))
+      || !CLASS_TYPE_P (TREE_TYPE (intype))
       || !CLASS_TYPE_P (TREE_TYPE (TREE_TYPE (op)))
-      || !DERIVED_FROM_P (TREE_TYPE (TREE_TYPE (op)), TREE_TYPE (type)))
+      || !DERIVED_FROM_P (TREE_TYPE (intype), TREE_TYPE (type))
+      || same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (intype)),
+		      TYPE_MAIN_VARIANT (TREE_TYPE (type))))
     return NULL_TREE;
 
   return cp_ubsan_maybe_instrument_vptr (loc, op, TREE_TYPE (type), true,
--- gcc/cp/typeck.c.jj	2015-11-20 08:17:50.000000000 +0100
+++ gcc/cp/typeck.c	2015-11-24 13:46:37.618746306 +0100
@@ -6590,7 +6590,8 @@  build_static_cast_1 (tree type, tree exp
       if (flag_sanitize & SANITIZE_VPTR)
 	{
 	  tree ubsan_check
-	    = cp_ubsan_maybe_instrument_downcast (input_location, type, expr);
+	    = cp_ubsan_maybe_instrument_downcast (input_location, type,
+						  intype, expr);
 	  if (ubsan_check)
 	    expr = ubsan_check;
 	}
@@ -6737,7 +6738,8 @@  build_static_cast_1 (tree type, tree exp
       if (flag_sanitize & SANITIZE_VPTR)
 	{
 	  tree ubsan_check
-	    = cp_ubsan_maybe_instrument_downcast (input_location, type, expr);
+	    = cp_ubsan_maybe_instrument_downcast (input_location, type,
+						  intype, expr);
 	  if (ubsan_check)
 	    expr = ubsan_check;
 	}
--- gcc/testsuite/g++.dg/ubsan/pr68508.C.jj	2015-11-24 13:58:27.506683395 +0100
+++ gcc/testsuite/g++.dg/ubsan/pr68508.C	2015-11-24 13:53:41.000000000 +0100
@@ -0,0 +1,15 @@ 
+// PR c++/68508
+// { dg-do compile }
+// { dg-options "-std=c++14 -fsanitize=vptr" }
+
+struct A
+{
+  virtual int foo () { return 0; }
+};
+
+const A &
+bar ()
+{
+  static A x = A ();
+  return (x);
+}