On Fri, Mar 17, 2023 at 01:55:51PM +0000, Richard Biener wrote:
> > 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.
BUILT_IN_ASSUME_ALIGNED doesn't do that intentionally.
E.g. tree-object-size.cc (pass_through_call) comments on this:
unsigned rf = gimple_call_return_flags (call);
if (rf & ERF_RETURNS_ARG)
{
unsigned argnum = rf & ERF_RETURN_ARG_MASK;
if (argnum < gimple_call_num_args (call))
return gimple_call_arg (call, argnum);
}
/* __builtin_assume_aligned is intentionally not marked RET1. */
if (gimple_call_builtin_p (call, BUILT_IN_ASSUME_ALIGNED))
return gimple_call_arg (call, 0);
The reason is that we don't want passes to propagate the argument to the
return value but use a different SSA_NAME there, so that we can have
there different alignment info.
And as you show on the testcases, it probably isn't a good idea for
BUILT_IN_EXPECT* either.
So, perhaps use op_cfn_pass_through_arg1 for the ERF_RETURNS_ARG functions
and BUILT_IN_EXPECT* ?
> >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
Jakub
