Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1)

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Tue, Jan 6, 2015 at 11:34 AM, Peter Stephenson
<p.stephenson@xxxxxxxxxxx> wrote:
> On Tue, 6 Jan 2015 11:14:12 +0100
> Mikael Magnusson <mikachu@xxxxxxxxx> wrote:
>> >> @@ -4461,6 +4461,8 @@ execfuncdef(Estate state, Eprog redir_prog)
>> >>           if (htok && args) {
>> >>               execsubst(args);
>> >>               if (errflag) {
>> >> +                 zsfree(shf->filename);
>> >> +                 zfree(shf, sizeof(*shf));
>> >>                   state->pc = end;
>> >>                   return 1;
>> >>               }
>> >
>> > Can't see how that can be wrong.  Nothing else can take owernship of
>> > shf in the error case.
>>
>> I think what I was unsure of in this one, but forgot to write, is if
>> freeeprog(shf->funcdef) should also be called here? It looks like for
>> !names, it's all allocated on the heap, but then the non-error path
>> does free it explicitly anyway (and it looked like freeeprog just
>> doesn't touch it if it was on the heap), is that just paranoia?
>
> It should certainly be consistently one thing or the other, yes, so
> probably better add it, and if so I suppose the paranoid test for redir
> as well.
>
> There's no zsfree(shf->filename) after the settrap() failure in the
> other branch.  I can't see how that can be anything other than a leak
> --- nothing special happens to shf->filename before shf is freed.

Okay, I've added the freeeprogs to the path in the above patch, and
the zsfree of filename in the trap path.

>> And since we have all these heap vs malloc checks already, why not use
>> zhalloc for shf too?  Although the code might end up neater if we just
>> always do the malloc + free regardless since there's a bunch of
>> almost-identical code for that stuff.
>
> The permanent alloc appears to be needed in the case of names != NULL to
> add the function to the main table --- but the current hotch potch might
> certainly be improved by doing one thing or the other:  always
> permanently allocate everything in one case and always use heap
> allocation in the other; or alternatively always malloc and if necessary
> free.  That needs more thought.
>
> pws

And I suppose I'll leave this for an even colder winter day. Or we
could point potential new contributors looking for areas to help to
this code, as an introductory exercise to set their expectations
right.

-- 
Mikael Magnusson