Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable

[Jason Merrill <jason@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

On 6/20/22 16:16, Jason Merrill wrote:
> On 6/20/22 07:05, Jakub Jelinek wrote:
>> On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote:
>>> Related to PR104642, the current situation where we get less return 
>>> checking
>>> with just -fsanitize=unreachable than no sanitize flags seems 
>>> undesirable; I
>>> propose that we do return checking when -fsanitize=unreachable.
>>
>> __builtin_unreachable itself (unless turned into trap or
>> __ubsan_handle_builtin_unreachable) is not any kind of return 
>> checking, it
>> is just an optimization.
> 
> Yes, but I'm talking about "when -fsanitize=unreachable".
> 
>>> Looks like clang just traps on missing return if not 
>>> -fsanitize=return, but
>>> the approach in this patch seems more helpful to me if we're already
>>> sanitizing other should-be-unreachable code.
>>>
>>> I'm assuming that the difference in treatment of SANITIZE_UNREACHABLE 
>>> and
>>> SANITIZE_RETURN with regard to loop optimization is deliberate.
>>
>> return and unreachable are separate sanitizers and such silent one way
>> implication can have quite unexpected consequences, especially with
>> -fsanitize-trap=.
>> Say with -fsanitize=unreachable -fsanitize-trap=unreachable, both current
>> trunk and clang will link without -lubsan, because the only enabled UBSan
>> sanitizers use __builtin_trap () which doesn't need library.
>> With -fsanitize=unreachable silently meaning 
>> -fsanitize=unreachable,return
>> the above would link in -lubsan, because while SANITIZE_UNREACHABLE uses
>> __builtin_trap, SANITIZE_RETURN doesn't.
>> Similarly, one has no_sanitize attribute, one could in certain function
>> __attribute__((no_sanitize ("unreachable"))) and because on the command
>> line using -fsanitize=unreachable assume other sanitizers aren't enabled,
>> but the silent addition of return sanitizer would break that.
> 
> Ah, true.  How about this approach instead?

Or, this approach relies on the PR104642 patch, and just fixes the line 
number issue.  This is less clear about the problem than using the 
return ubsan library function, but avoids using one entry point to 
implement the other sanitizer, if that's important.

[-- Attachment #2: 0001-ubsan-do-return-check-with-fsanitize-unreachable.patch --]
[-- Type: text/x-patch, Size: 4045 bytes --]

From da268c4c1f9ac0a7eaa4d428791c3ed51cf0994d Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Wed, 15 Jun 2022 15:45:48 -0400
Subject: [PATCH] ubsan: do return check with -fsanitize=unreachable
To: gcc-patches@gcc.gnu.org

The current situation where we get less return checking with just
-fsanitize=unreachable than no sanitize flags seems undesirable; we would
get checking there except that we explicitly avoid emitting a
__builtin_unreachable.  The documented reason is that the use of
BUILTIN_LOCATION makes the message confusing, so let's fix that.

The !optimize check seems unneded since the PR104642 patch.

gcc/cp/ChangeLog:

	* cp-gimplify.cc (cp_maybe_instrument_return): Pass the real
	location to the ubsan unreachable function.

gcc/ChangeLog:

	* tree-cfg.cc (pass_warn_function_return::execute): Check
	for ubsan unreachable.

gcc/testsuite/ChangeLog:

	* g++.dg/ubsan/return-8c.C: New test.
---
 gcc/cp/cp-gimplify.cc                  | 19 ++++++-------------
 gcc/testsuite/g++.dg/ubsan/return-8c.C | 16 ++++++++++++++++
 gcc/tree-cfg.cc                        |  3 +++
 3 files changed, 25 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ubsan/return-8c.C

diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
index c05be833357..fcea9f8d0e0 100644
--- a/gcc/cp/cp-gimplify.cc
+++ b/gcc/cp/cp-gimplify.cc
@@ -1806,18 +1806,6 @@ cp_maybe_instrument_return (tree fndecl)
       || !targetm.warn_func_return (fndecl))
     return;
 
-  if (!sanitize_flags_p (SANITIZE_RETURN, fndecl)
-      /* Don't add __builtin_unreachable () if not optimizing, it will not
-	 improve any optimizations in that case, just break UB code.
-	 Don't add it if -fsanitize=unreachable -fno-sanitize=return either,
-	 UBSan covers this with ubsan_instrument_return above where sufficient
-	 information is provided, while the __builtin_unreachable () below
-	 if return sanitization is disabled will just result in hard to
-	 understand runtime error without location.  */
-      && ((!optimize && !flag_unreachable_traps)
-	  || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl)))
-    return;
-
   tree t = DECL_SAVED_TREE (fndecl);
   while (t)
     {
@@ -1864,7 +1852,12 @@ cp_maybe_instrument_return (tree fndecl)
   if (sanitize_flags_p (SANITIZE_RETURN, fndecl))
     t = ubsan_instrument_return (loc);
   else
-    t = build_builtin_unreachable (BUILTINS_LOCATION);
+    {
+      /* Pass the real location to the ubsan function.  */
+      t = build_builtin_unreachable (loc);
+      /* But set BUILTINS_LOCATION for pass_warn_function_return.  */
+      SET_EXPR_LOCATION (t, BUILTINS_LOCATION);
+    }
 
   append_to_statement_list (t, p);
 }
diff --git a/gcc/testsuite/g++.dg/ubsan/return-8c.C b/gcc/testsuite/g++.dg/ubsan/return-8c.C
new file mode 100644
index 00000000000..828b24efa31
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ubsan/return-8c.C
@@ -0,0 +1,16 @@
+// PR c++/104642
+
+// -fsanitize=unreachable should catch missing return even without
+// -fsanitize=return.
+
+// { dg-do run }
+// { dg-shouldfail { *-*-* } }
+// { dg-additional-options "-O -fsanitize=unreachable" }
+
+bool b;
+
+int f() {
+  if (b) return 42;
+} // { dg-warning "-Wreturn-type" }
+
+int main() { f(); }
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index 734fdddbf97..647d98f0079 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -9551,10 +9551,13 @@ pass_warn_function_return::execute (function *fun)
 	      gimple *last = last_stmt (bb);
 	      const enum built_in_function ubsan_missing_ret
 		= BUILT_IN_UBSAN_HANDLE_MISSING_RETURN;
+	      const enum built_in_function ubsan_unreachable
+		= BUILT_IN_UBSAN_HANDLE_BUILTIN_UNREACHABLE;
 	      if (last
 		  && ((LOCATION_LOCUS (gimple_location (last))
 		       == BUILTINS_LOCATION
 		       && (gimple_call_builtin_p (last, BUILT_IN_UNREACHABLE)
+			   || gimple_call_builtin_p (last, ubsan_unreachable)
 			   || gimple_call_builtin_p (last, BUILT_IN_TRAP)))
 		      || gimple_call_builtin_p (last, ubsan_missing_ret)))
 		{
-- 
2.27.0