From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12377 invoked by alias); 6 Jan 2015 11:06:36 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 34134 Received: (qmail 14501 invoked from network); 6 Jan 2015 11:06:33 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=F4CMnxOEgNB4KN36RLctX6HGV2mY8+r76Xk2XpV6u1w=; b=A8NTLOoyCXQA9dgl2dzjw7i+SgsHEiFIWepMmFDlxTQzYe0Xw7uSQf8lqmCuN3FA1q zxDm05T6tPp/Dq07xU/wqYa8hu6IbnrNQTeN1cj0qNd12GtyYvc4mlrQ/YnqQNJKMiyM WtTypIZV7Bnd3tu7a5/pSRqLatLUDIRZSzilMrk3d+n9jzsTbLR8ZHBcvZaa3pPWOPOm Y+czVgNT4BIEnG8U0QhzlF3APNo8uZTtMfN//KjcCRvQIkB+Rrho2e1Z4m/YjQnYEW+Z NOeAArxLFIqzxcRRLyqi2eGjRtld2VFFodetJU5FaRdmUKTLADMW4pQeM1bTlwTgK7Ct U4Zw== MIME-Version: 1.0 X-Received: by 10.107.5.7 with SMTP id 7mr84650323iof.1.1420542392428; Tue, 06 Jan 2015 03:06:32 -0800 (PST) In-Reply-To: <20150106103437.7cde520a@pwslap01u.europe.root.pri> References: <1420521949-30483-1-git-send-email-mikachu@gmail.com> <1420521949-30483-14-git-send-email-mikachu@gmail.com> <20150106095449.081ed9a5@pwslap01u.europe.root.pri> <20150106103437.7cde520a@pwslap01u.europe.root.pri> Date: Tue, 6 Jan 2015 12:06:32 +0100 Message-ID: Subject: Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1) From: Mikael Magnusson To: Peter Stephenson Cc: zsh workers Content-Type: text/plain; charset=UTF-8 On Tue, Jan 6, 2015 at 11:34 AM, Peter Stephenson wrote: > On Tue, 6 Jan 2015 11:14:12 +0100 > Mikael Magnusson 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