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

Only that we now _do_ have BUILT_IN_ASSUME_ALIGNED marked as RET1 ...

> 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* ?

But that already causes the problems (I didn't yet finish testing
adding RET1 to BUILT_IN_EXPECT*).  Note FRE currently does not use
returns_arg but I have old patches that do - but those replace
uses after a RET1 function with the return value to also reduce
spilling around a call (they of course CSE equal calls).

Anyway, I suspect the ssa-lim-21.c testcase is just very fragile
while the predict-20.c shows a case which should be handled
better even without the __builtin_expect (which is now gone):

  for (;;)
    {
      if (__builtin_expect (b < 0, 0))
        break;
      if (__builtin_expect (a, 1))
        break;
    }
  int d = b < 0;
  if (__builtin_expect (d, 0))
    asm("");

we should be able to handle

 _1 = PHI <1, 0>
 if (_1 != 0)

by copying the probabilities from the incoming PHI edges here,
possibly even by using ranger to test for known condition
on some edges.

Of course it's bad to regress more here.  I'm considering
backing out the -Wuse-after-free changes.

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
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)