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

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

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.

> 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