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

[Andrew MacLeod <amacleod@redhat.com>]

On 3/17/23 08:59, 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?
>
I think thats fine for now.

Im going to address improving dispatch for range-ops in stage 1 when it 
opens.

we want to handle non-standard ops more generally like we did for 
WIDEN_MULT_EXPR, plus we didnt know the actualy requirements for the 
initial cut of vrange ->irange/frange dispatch.   We'll clean that up to 
make adding more range kinds cleaner.

as for gimple_fnspec, im sure we can do something better than what we 
have.  Current range-ops works only with 2 operands, but via this 
mechanism they can be any 2. (I think :-)

We can fold arbitrary statements in 
gimpe-range-fold::fold_using_range(), so it is only the 
op[12]_range/relation  routines we loose... Im not sure if there is 
anything that critical there, but if we find something, well we can look 
at it.

Andrew