Re: [PATCH] tree-optimization/109170 - bogus use-after-free with __builtin_expect

[Richard Biener <rguenther@suse.de>]

On Fri, 17 Mar 2023, Jakub Jelinek wrote:

> On Fri, Mar 17, 2023 at 12:53:48PM +0000, Richard Biener wrote:
> > On Fri, 17 Mar 2023, Jakub Jelinek wrote:
> > 
> > > On Fri, Mar 17, 2023 at 01:18:32PM +0100, Richard Biener wrote:
> > > > The following adds a missing range-op for __builtin_expect which
> > > > helps -Wuse-after-free to detect the case a realloc original
> > > > pointer is used when the result was NULL.
> > > > 
> > > > Bootstrap and regtest running on x86_64-unknown-linux-gnu, OK?
> > > > 
> > > > 	PR tree-optimization/109170
> > > > 	* gimple-range-op.cc (cfn_expect): New.
> > > > 	(gimple_range_op_handler::maybe_builtin_call): Handle
> > > > 	__builtin_expect.
> > > > 
> > > > 	* gcc.dg/Wuse-after-free-pr109170.c: New testcase.
> > > 
> > > Shouldn't that be something we handle generically for all
> > > ERF_RETURNS_ARG calls (and not just for irange, but for any
> > > supported ranges)?
> > > 
> > > Though, admittedly __builtin_expect probably doesn't set that
> > > and all the other current builtins with ERF_RETURNS_ARG return
> > > pointers I think.
> > 
> > Looking at builtin_fnspec we're indeed missing BUILT_IN_EXPECT,
> > but we could indeed use gimple_call_fnspec and look for a
> > returned argument.  If it's not the first handling this
> > generically is going to be interesting wrt op?_range though,
> > so we'd need a range operator for each case (returns arg 1,
> > returns arg 2, more args are not supported?).  Currently
> 
> I think fnspec supports 1-4, but nothing actually uses anything but 1
> or none; I could be wrong.
> 
> Anyway, I think it is fine to implement __builtin_expect this way
> for now, ERF_RETURNS_ARG will be more important for pointers, especially if
> we propagate something more than just maybe be/can't be/must be null.
> Don't you need to handle BUILT_IN_EXPECT_WITH_PROBABILITY the same though?

Yes, BUILT_IN_ASSUME_ALIGNED would be another candidate.

One issue revealed by testing is that EVRP now propagates

  b.0_1 = b;
  _2 = b.0_1 < 0;
  _3 = (long int) _2;
  _4 = __builtin_expect (_3, 0);
  if (_4 != 0)
...

  b.2_8 = b;
  _9 = b.2_8 < 0;
  d_13 = (int) _9;
  _10 = (long int) _9;
  _11 = __builtin_expect (_10, 0);
  if (_11 != 0)

and thus gcc.dg/predict-20.c FAILs and the change is that we propagate
known true/false into the last compare as

  <bb 5> [local count: 977105059]:
  # _9 = PHI <1(3), 0(4)>
  if (_9 != 0)

and lose the connection to __builtin_expect.

We also FAIL gcc.dg/tree-ssa/ssa-lim-21.c, but that's because

  for (int j = 0; j < m; j++)
    if (__builtin_expect (m, 0))

is now optimized (m is [1, +INF] when we enter the loop).  I
have difficulties in restoring the testcase by massaging it,
will try a bit more.

I've implemented the fnspec variant as well now and then we also
CSE the __builtin_expct call, see below for this patch variant.

Richard.

From feb846cbff9774125d8401dfeacd8a4b9c2dccfa Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Fri, 17 Mar 2023 13:14:49 +0100
Subject: [PATCH] tree-optimization/109170 - bogus use-after-free with
 __builtin_expect
To: gcc-patches@gcc.gnu.org

The following adds a missing range-op for __builtin_expect which
helps -Wuse-after-free to detect the case a realloc original
pointer is used when the result was NULL.  The implementation
should handle all argument one pass-through builtins we handle
in the fnspec machinery.

	tree-optimization/109170
	* gimple-range-op.cc (cfn_pass_through_arg1): New.
	(gimple_range_op_handler::maybe_builtin_call): Handle
	__builtin_expect and similar via cfn_pass_through_arg1
	and inspecting the calls fnspec.
	* builtins.cc (builtin_fnspec): Handle BUILT_IN_EXPECT
	and BUILT_IN_EXPECT_WITH_PROBABILITY.

	* gcc.dg/Wuse-after-free-pr109170.c: New testcase.
---
 gcc/builtins.cc                               |  2 ++
 gcc/gimple-range-op.cc                        | 32 ++++++++++++++++++-
 .../gcc.dg/Wuse-after-free-pr109170.c         | 15 +++++++++
 3 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c

diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index 90246e214d6..56545027297 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -11715,6 +11715,8 @@ builtin_fnspec (tree callee)
       case BUILT_IN_RETURN_ADDRESS:
 	return ".c";
       case BUILT_IN_ASSUME_ALIGNED:
+      case BUILT_IN_EXPECT:
+      case BUILT_IN_EXPECT_WITH_PROBABILITY:
 	return "1cX ";
       /* But posix_memalign stores a pointer into the memory pointed to
 	 by its first argument.  */
diff --git a/gcc/gimple-range-op.cc b/gcc/gimple-range-op.cc
index a5d625387e7..1a00f1690e5 100644
--- a/gcc/gimple-range-op.cc
+++ b/gcc/gimple-range-op.cc
@@ -43,6 +43,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "range.h"
 #include "value-query.h"
 #include "gimple-range.h"
+#include "attr-fnspec.h"
 
 // Given stmt S, fill VEC, up to VEC_SIZE elements, with relevant ssa-names
 // on the statement.  For efficiency, it is an error to not pass in enough
@@ -309,6 +310,26 @@ public:
   }
 } op_cfn_constant_p;
 
+// Implement range operator for integral/pointer functions returning
+// the first argument.
+class cfn_pass_through_arg1 : public range_operator
+{
+public:
+  using range_operator::fold_range;
+  virtual bool fold_range (irange &r, tree, const irange &lh,
+			   const irange &, relation_trio) const
+  {
+    r = lh;
+    return true;
+  }
+  virtual bool op1_range (irange &r, tree, const irange &lhs,
+			  const irange &, relation_trio) const
+  {
+    r = lhs;
+    return true;
+  }
+} op_cfn_pass_through_arg1;
+
 // Implement range operator for CFN_BUILT_IN_SIGNBIT.
 class cfn_signbit : public range_operator_float
 {
@@ -967,6 +988,15 @@ gimple_range_op_handler::maybe_builtin_call ()
       break;
 
     default:
-      break;
+      {
+	unsigned arg;
+	if (gimple_call_fnspec (call).returns_arg (&arg) && arg == 0)
+	  {
+	    m_valid = true;
+	    m_op1 = gimple_call_arg (call, 0);
+	    m_int = &op_cfn_pass_through_arg1;
+	  }
+	break;
+      }
     }
 }
diff --git a/gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c b/gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c
new file mode 100644
index 00000000000..fa7dc66d66c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wuse-after-free-pr109170.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wuse-after-free" } */
+
+unsigned long bufmax = 0;
+unsigned long __open_catalog_bufmax;
+void *realloc(void *, __SIZE_TYPE__);
+void free(void *);
+
+void __open_catalog(char *buf)
+{
+  char *old_buf = buf;
+  buf = realloc (buf, bufmax);
+  if (__builtin_expect ((buf == ((void *)0)), 0))
+    free (old_buf); /* { dg-bogus "used after" } */
+}
-- 
2.35.3