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 <rguent...@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
