Re: [RFA] PR python/18565 - make Frame.function work for inline frames

Pedro Alves
2016-08-03T11:35:37Z

On 08/03/2016 09:07 AM, Yao Qi wrote:
> On Mon, Jul 25, 2016 at 12:04 PM, Pedro Alves <palves@redhat.com> wrote:
>>>
>>> The reason I suggested that way is that the exception may be thrown out in
>>> find_frame_funname after the memory is allocated for funname, so we need
>>> xfree in CATCH, and also need xfree afterwards.
>>
>> I disagree. In general, I think that up until the called function does a normal
>
> What do you disagree on?

That it's the caller's responsibility to free an output parameter
of a called function that throws. Or more generally, that the state
of an output parameter as observed in the caller is determinate
when the callee throws.

>
>> return, the memory for output parameters is owned by the called function.
>> A normal return then transfers ownership of the output parameters' memory
>> to the caller.
>
> Yes, so we need xfree after find_frame_funname on normal return.

That's what Tromey's patch does.

> That is what I suggested.

You suggested to free it _also_ when the exception is thrown. That's
where my disagreement lies.

>
> We need to free the memory referenced by output parameter when exception
> is thrown too.

This.

> The point in question is that who is responsible to free the
> memory referenced by output parameter.

Right.

> In Tom's patch, they are freed in
> the caller in normal return, so it is reasonable to free them in the caller in
> exception return as well, because it is not specified that find_frame_funname
> frees the memory on exception.

I don't think it needs to be explicitly specified, because I think it
should be the behavior or any function that has output parameters.

It's unsafe otherwise, because when an exception is thrown from inside
a callee, the caller has no idea whether the output parameter has been
definitely assigned to.

- the callee might throw an exception before the output parameter pointer
is ever written to.
- the output parameter pointer may have been initialized but now be
dangling at the point the exception is thrown inside callee - the
callee freed it before throwing.

So the exception path (usually the cleanup) in the caller could try to use
a dangling pointer (or even a partially constructed object).

Basically, this, where foo returns through an output param:

extern void foo (char **ret);
char *ret;

old_chain = make_cleanup (xfree, ret);
foo (&ret);
do_cleanups (old_chain);

... is as broken as this obviously broken one, which is the exact
same except that it returns through normal return:

extern char *foo (void);
char *ret;

old_chain = make_cleanup (xfree, ret);
ret = foo ();
do_cleanups (old_chain);

>
>>
>> So I think that it's find_frame_funname that should be responsible for making
>> sure that memory for output parameters is cleaned up on exception, or be
>> written in a way that never throws after the memory allocation, which it may be
>> already, but I haven't checked in detail.
>>
>
> If you think it is find_frame_funname's responsibility to free memory on
> exception, that is fine. We should document this behaviour for
> find_frame_funname and guarantee that find_frame_funname behaves
> that way. However, we are not sure current find_frame_funname behaves that
> way, because exception may be thrown in cp_remove_params.

IMO that becomes an unrelated, preexisting problem. I don't think we should
require that all the functions (and their callees, transitively) called by all
patches are first inspected for leaks and fixed.

Thanks,
Pedro Alves