[C++ PATCH] Fix -fsanitize={null,alignment} of references (PR c++/79572)

[C++ PATCH] Fix -fsanitize={null,alignment} of references (PR c++/79572)

[Jakub Jelinek]

Hi!

Since late C++ folding has been committed, we don't sanitize some reference
bindings to NULL.  Earlier we had always NOP_EXPR to REFERENCE_TYPE say from
INTEGER_CST or whatever else, but cp_fold can now turn that right into
INTEGER_CST with REFERENCE_TYPE.  The following patch sanitizes even those.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-03-23  Jakub Jelinek  <jakub@redhat.com>

	PR c++/79572
	* c-ubsan.h (ubsan_maybe_instrument_reference): Change argument to
	tree *.
	* c-ubsan.c (ubsan_maybe_instrument_reference): Likewise.  Handle
	not just NOP_EXPR to REFERENCE_TYPE, but also INTEGER_CST with
	REFERENCE_TYPE.

	* cp-gimplify.c (cp_genericize_r): Sanitize INTEGER_CSTs with
	REFERENCE_TYPE.  Adjust ubsan_maybe_instrument_reference caller
	for NOP_EXPR to REFERENCE_TYPE.

	* g++.dg/ubsan/null-8.C: New test.

--- gcc/c-family/c-ubsan.h.jj	2017-01-01 12:45:46.000000000 +0100
+++ gcc/c-family/c-ubsan.h	2017-03-23 09:13:16.287888726 +0100
@@ -28,7 +28,7 @@ extern tree ubsan_instrument_return (loc
 extern tree ubsan_instrument_bounds (location_t, tree, tree *, bool);
 extern bool ubsan_array_ref_instrumented_p (const_tree);
 extern void ubsan_maybe_instrument_array_ref (tree *, bool);
-extern void ubsan_maybe_instrument_reference (tree);
+extern void ubsan_maybe_instrument_reference (tree *);
 extern void ubsan_maybe_instrument_member_call (tree, bool);
 
 /* Declare this here as well as in ubsan.h. */
--- gcc/c-family/c-ubsan.c.jj	2017-01-01 12:45:46.000000000 +0100
+++ gcc/c-family/c-ubsan.c	2017-03-23 09:18:51.775486699 +0100
@@ -458,17 +458,26 @@ ubsan_maybe_instrument_reference_or_call
   return fold_build2 (COMPOUND_EXPR, TREE_TYPE (op), call, op);
 }
 
-/* Instrument a NOP_EXPR to REFERENCE_TYPE if needed.  */
+/* Instrument a NOP_EXPR to REFERENCE_TYPE or INTEGER_CST with REFERENCE_TYPE
+   type if needed.  */
 
 void
-ubsan_maybe_instrument_reference (tree stmt)
+ubsan_maybe_instrument_reference (tree *stmt_p)
 {
-  tree op = TREE_OPERAND (stmt, 0);
+  tree stmt = *stmt_p;
+  tree op = stmt;
+  if (TREE_CODE (stmt) == NOP_EXPR)
+    op = TREE_OPERAND (stmt, 0);
   op = ubsan_maybe_instrument_reference_or_call (EXPR_LOCATION (stmt), op,
 						 TREE_TYPE (stmt),
 						 UBSAN_REF_BINDING);
   if (op)
-    TREE_OPERAND (stmt, 0) = op;
+    {
+      if (TREE_CODE (stmt) == NOP_EXPR) 
+	TREE_OPERAND (stmt, 0) = op;
+      else
+	*stmt_p = op;
+    }
 }
 
 /* Instrument a CALL_EXPR to a method if needed.  */
--- gcc/cp/cp-gimplify.c.jj	2017-03-03 13:23:58.000000000 +0100
+++ gcc/cp/cp-gimplify.c	2017-03-23 09:21:26.693460888 +0100
@@ -1130,6 +1130,19 @@ cp_genericize_r (tree *stmt_p, int *walk
 	}
     }
 
+  if (TREE_CODE (stmt) == INTEGER_CST
+      && TREE_CODE (TREE_TYPE (stmt)) == REFERENCE_TYPE
+      && (flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT))
+      && !wtd->no_sanitize_p)
+    {
+      ubsan_maybe_instrument_reference (stmt_p);
+      if (*stmt_p != stmt)
+	{
+	  *walk_subtrees = 0;
+	  return NULL_TREE;
+	}
+    }
+
   /* Other than invisiref parms, don't walk the same tree twice.  */
   if (p_set->contains (stmt))
     {
@@ -1477,7 +1490,7 @@ cp_genericize_r (tree *stmt_p, int *walk
       if ((flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT))
 	  && TREE_CODE (stmt) == NOP_EXPR
 	  && TREE_CODE (TREE_TYPE (stmt)) == REFERENCE_TYPE)
-	ubsan_maybe_instrument_reference (stmt);
+	ubsan_maybe_instrument_reference (stmt_p);
       else if (TREE_CODE (stmt) == CALL_EXPR)
 	{
 	  tree fn = CALL_EXPR_FN (stmt);
--- gcc/testsuite/g++.dg/ubsan/null-8.C.jj	2017-03-23 09:42:31.664696676 +0100
+++ gcc/testsuite/g++.dg/ubsan/null-8.C	2017-03-23 09:43:31.501908802 +0100
@@ -0,0 +1,19 @@
+// PR c++/79572
+// { dg-do run }
+// { dg-options "-fsanitize=null -std=c++14" }
+// { dg-output "reference binding to null pointer of type 'const int'" }
+
+void
+foo (const int &iref)
+{
+  if (&iref)
+    __builtin_printf ("iref %d\n", iref);
+  else
+    __builtin_printf ("iref is NULL\n");
+}
+
+int
+main ()
+{
+  foo (*((int*) __null));
+}

	Jakub

Re: [C++ PATCH] Fix -fsanitize={null,alignment} of references (PR c++/79572)

[Maxim Kuvyrkov]

On Thu, Mar 23, 2017 at 11:37 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Since late C++ folding has been committed, we don't sanitize some reference
> bindings to NULL.  Earlier we had always NOP_EXPR to REFERENCE_TYPE say from
> INTEGER_CST or whatever else, but cp_fold can now turn that right into
> INTEGER_CST with REFERENCE_TYPE.  The following patch sanitizes even those.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2017-03-23  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/79572
>         * c-ubsan.h (ubsan_maybe_instrument_reference): Change argument to
>         tree *.
>         * c-ubsan.c (ubsan_maybe_instrument_reference): Likewise.  Handle
>         not just NOP_EXPR to REFERENCE_TYPE, but also INTEGER_CST with
>         REFERENCE_TYPE.
>
>         * cp-gimplify.c (cp_genericize_r): Sanitize INTEGER_CSTs with
>         REFERENCE_TYPE.  Adjust ubsan_maybe_instrument_reference caller
>         for NOP_EXPR to REFERENCE_TYPE.
>
>         * g++.dg/ubsan/null-8.C: New test.
>
...
> --- gcc/testsuite/g++.dg/ubsan/null-8.C.jj      2017-03-23 09:42:31.664696676 +0100
> +++ gcc/testsuite/g++.dg/ubsan/null-8.C 2017-03-23 09:43:31.501908802 +0100
> @@ -0,0 +1,19 @@
> +// PR c++/79572
> +// { dg-do run }
> +// { dg-options "-fsanitize=null -std=c++14" }
> +// { dg-output "reference binding to null pointer of type 'const int'" }
> +
> +void
> +foo (const int &iref)
> +{
> +  if (&iref)
> +    __builtin_printf ("iref %d\n", iref);
> +  else
> +    __builtin_printf ("iref is NULL\n");

Hi Jakub,

Using __builtin_printf causes this test to fail sporadically when
cross-testing.  Stdout and stderr output can get mixed in
cross-testing, so dejagnu might see
==
g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null
pointer of type iref is NULL
'const int'
==
instead of
==
g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null
pointer of type 'const int'
iref is NULL
==

Is it essential for this testcase to use __builtin_printf or simple
"fprintf (stderr, ...)" would do just fine?

> +}
> +
> +int
> +main ()
> +{
> +  foo (*((int*) __null));
> +}

Regards,

-- 
Maxim Kuvyrkov

Re: [C++ PATCH] Fix -fsanitize={null,alignment} of references (PR c++/79572)

[Jakub Jelinek]

On Fri, Nov 24, 2017 at 05:16:27PM +0300, Maxim Kuvyrkov wrote:
> Using __builtin_printf causes this test to fail sporadically when
> cross-testing.  Stdout and stderr output can get mixed in
> cross-testing, so dejagnu might see
> ==
> g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null
> pointer of type iref is NULL
> 'const int'
> ==
> instead of
> ==
> g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null
> pointer of type 'const int'
> iref is NULL
> ==
> 
> Is it essential for this testcase to use __builtin_printf or simple
> "fprintf (stderr, ...)" would do just fine?

That would mean bringing in stdio.h, which is very much undesirable.

If you want, just revert the patch, verify the testcase FAILs,
and then tweak it to say:
__attribute__((noinline, noclone))
void
bar (const *x, int y)
{
  asm volatile ("" : : "g" (x), "g" (y) : "memory");
}

and change __builtin_printf ("iref %d\n", iref);
to bar ("iref %d\n", iref);
and __builtin_printf ("iref is NULL\n");
to bar ("iref is NULL\n", 0);
If the test still FAILs and is fixed after you reapply the patch,
the change is preapproved.

	Jakub

Re: [C++ PATCH] Fix -fsanitize={null,alignment} of references (PR c++/79572)

[Jakub Jelinek]

On Fri, Nov 24, 2017 at 03:26:11PM +0100, Jakub Jelinek wrote:
> On Fri, Nov 24, 2017 at 05:16:27PM +0300, Maxim Kuvyrkov wrote:
> > Using __builtin_printf causes this test to fail sporadically when
> > cross-testing.  Stdout and stderr output can get mixed in
> > cross-testing, so dejagnu might see
> > ==
> > g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null
> > pointer of type iref is NULL
> > 'const int'
> > ==
> > instead of
> > ==
> > g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null
> > pointer of type 'const int'
> > iref is NULL
> > ==
> > 
> > Is it essential for this testcase to use __builtin_printf or simple
> > "fprintf (stderr, ...)" would do just fine?
> 
> That would mean bringing in stdio.h, which is very much undesirable.
> 
> If you want, just revert the patch, verify the testcase FAILs,
> and then tweak it to say:
> __attribute__((noinline, noclone))
> void
> bar (const *x, int y)
> {
>   asm volatile ("" : : "g" (x), "g" (y) : "memory");
> }
> 
> and change __builtin_printf ("iref %d\n", iref);
> to bar ("iref %d\n", iref);
> and __builtin_printf ("iref is NULL\n");
> to bar ("iref is NULL\n", 0);
> If the test still FAILs and is fixed after you reapply the patch,
> the change is preapproved.

Verified myself:
./cc1plus.246620 -O0 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8
./cc1plus.246621 -O0 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8
null-8.C:25:7: runtime error: reference binding to null pointer of type 'const int'
./cc1plus.246620 -O2 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8
./cc1plus.246621 -O2 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8
null-8.C:25:7: runtime error: reference binding to null pointer of type 'const int'

Committed to trunk:

2017-11-27  Jakub Jelinek  <jakub@redhat.com>

	* g++.dg/ubsan/null-8.C (bar): New function.
	(foo): Use bar instead of __builtin_printf.

--- gcc/testsuite/g++.dg/ubsan/null-8.C.jj	2017-03-31 20:38:44.000000000 +0200
+++ gcc/testsuite/g++.dg/ubsan/null-8.C	2017-11-27 11:27:17.311529667 +0100
@@ -3,13 +3,20 @@
 // { dg-options "-fsanitize=null -std=c++14" }
 // { dg-output "reference binding to null pointer of type 'const int'" }
 
+__attribute__((noinline, noclone))
+void
+bar (int x)
+{
+  asm volatile ("" : : "r" (x) : "memory");
+}
+
 void
 foo (const int &iref)
 {
   if (&iref)
-    __builtin_printf ("iref %d\n", iref);
+    bar (iref);
   else
-    __builtin_printf ("iref is NULL\n");
+    bar (1);
 }
 
 int


	Jakub