Possible memory leak in hist.c

Possible memory leak in hist.c

[Vincent Lefevre]

Hi,

Say that savehistfile is called with err = 0 and that unlink(tmpfile)
fails (hist.c, line 2204). Then it seems that tmpfile will never be
free'd because all the "free(tmpfile);" are in conditions that will
always be false.

Wouldn't it be simpler to replace all the "free(tmpfile);" by

  if (tmpfile) {
    free(tmpfile);
  }

at the end?

-- 
Vincent Lefèvre <vincent@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / Arenaire project (LIP, ENS-Lyon)

Re: Possible memory leak in hist.c

[Peter Stephenson]

Vincent Lefevre wrote:
> Hi,
> 
> Say that savehistfile is called with err = 0 and that unlink(tmpfile)
> fails (hist.c, line 2204). Then it seems that tmpfile will never be
> free'd because all the "free(tmpfile);" are in conditions that will
> always be false.
> 
> Wouldn't it be simpler to replace all the "free(tmpfile);" by
> 
>   if (tmpfile) {
>     free(tmpfile);
>   }
> 
> at the end?

It does look suspicious.  However, we're currently using tmpfile to
indicate whether we should be reporting an error about the temporary
file, so without more work it looks like we can't actually remove the
early frees.  They should certainly be setting tmpfile to NULL, though.

Index: Src/hist.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/hist.c,v
retrieving revision 1.73
diff -u -r1.73 hist.c
--- Src/hist.c	11 Mar 2008 10:00:39 -0000	1.73
+++ Src/hist.c	14 Apr 2008 13:31:43 -0000
@@ -2214,6 +2214,7 @@
 #endif
 	     && sb.st_uid != euid) {
 		free(tmpfile);
+		tmpfile = NULL;
 		if (err) {
 		    if (isset(APPENDHISTORY) || isset(INCAPPENDHISTORY)
 		     || isset(SHAREHISTORY))
@@ -2292,6 +2293,7 @@
 		if (rename(tmpfile, unmeta(fn)) < 0)
 		    zerr("can't rename %s.new to $HISTFILE", fn);
 		free(tmpfile);
+		tmpfile = NULL;
 	    }
 
 	    if (writeflags & HFILE_SKIPOLD
@@ -2317,12 +2319,13 @@
 	ret = -1;
 
     if (ret < 0 && err) {
-	if (tmpfile) {
+	if (tmpfile)
 	    zerr("failed to write history file %s.new: %e", fn, errno);
-	    free(tmpfile);
-	} else
+	else
 	    zerr("failed to write history file %s: %e", fn, errno);
     }
+    if (tmpfile)
+	free(tmpfile);
 
     unlockhistfile(fn);
 }



-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Possible memory leak in hist.c

[Vincent Lefevre]

On 2008-04-14 14:34:27 +0100, Peter Stephenson wrote:
> It does look suspicious.  However, we're currently using tmpfile to
> indicate whether we should be reporting an error about the temporary
> file, so without more work it looks like we can't actually remove the
> early frees.

If fact there are two "free(tmpfile);": one of them is followed
by a "err = 0" and for the other one, ret remains >= 0, so that
if tmpfile is freed, then no more messages can be output.

>  They should certainly be setting tmpfile to NULL, though.

This solution may be cleaner, though.

-- 
Vincent Lefèvre <vincent@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / Arenaire project (LIP, ENS-Lyon)