From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4980 invoked by alias); 6 Jan 2015 10:44:47 -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: 34132 Received: (qmail 17796 invoked from network); 6 Jan 2015 10:44:44 -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=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI, SPF_HELO_PASS autolearn=ham version=3.3.2 X-AuditID: cbfec7f5-b7fc86d0000066b7-f8-54abba4099f9 Date: Tue, 06 Jan 2015 10:34:37 +0000 From: Peter Stephenson To: zsh workers Subject: Re: PATCH 13/17: anon funcs: don't leak shf when ctrl-c in () {:} =(sleep 1) Message-id: <20150106103437.7cde520a@pwslap01u.europe.root.pri> In-reply-to: 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> Organization: Samsung Cambridge Solution Centre X-Mailer: Claws Mail 3.7.9 (GTK+ 2.22.0; i386-redhat-linux-gnu) MIME-version: 1.0 Content-type: text/plain; charset=US-ASCII Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupmluLIzCtJLcpLzFFi42I5/e/4NV2HXatDDN6s4rc42PyQyYHRY9XB D0wBjFFcNimpOZllqUX6dglcGec+dLIVdPNXfHixiq2BcQZ3FyMnh4SAicTyFS1MELaYxIV7 69lAbCGBpYwS/x/ZdTFyAdnLmSTWnm5lBUmwCKhKvHx2AayBTcBQYuqm2YwgtghQvPn7P5Yu Rg4OYYFwiUcPWUDCvAL2Esefv2QHsTkFgiXWNBxihJj/nVFid5cyiM0voC9x9e8nqBvsJWZe OcMI0Sso8WPyPbA5zAJaEpu3NbFC2PISm9e8ZZ7AKDALSdksJGWzkJQtYGRexSiaWppcUJyU nmukV5yYW1yal66XnJ+7iRESgF93MC49ZnWIUYCDUYmH16NrdYgQa2JZcWXuIUYJDmYlEd4d 04FCvCmJlVWpRfnxRaU5qcWHGJk4OKUaGAWnlkYc7nYJauF6/kX8lvIqd3nDiZfYApkTjJV6 X+bw2N8wadDe8FNEfa3A9rfFyYfk1hjMunX40opnKxQ2ervwKHZwKBt1cmt9EM6ZUOj6eEHy 8rTnih3RZ+5O5Ui0eP/dMnLC7phnvCvXh6Y8WJh96kV5xIatMdMuTf+2w+m08UN2FllVRSWW 4oxEQy3mouJEAAk60YAeAgAA 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. > 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