Re: Possible memory leak in hist.c

[Peter Stephenson <pws@csr.com>]

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