Fix memleak in setup

Fix memleak in setup

[Charles Wilson]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

If compress::decompress returns NULL, we need to clean up the allocated 
specialized compress_* objects -- but NOT delete the passed-in 
io_stream*.  The problem occurs Installer::installOne, but is corrected 
by modifying the behavior of compress::decompress and the various 
specialized decompression classes.

Detailed explanation:

 From Installer::installOne:

   if ((try_decompress = compress::decompress (pkgfile)) != NULL)
     {
       // if we get here, then try_decompress has taken ownership
       // of pkgfile

       if ((tarstream = archive::extract (try_decompress)) == NULL)
         {
           // error handling stuff
           delete try_decompress;
           // since try_decompress owns pkgfile, this cleans
           // up both try_decompress and pkgfile.
           return;
         }

        // this is success...tarstream now owns try_decompress,
        // which in turn owns pkgfile.  Eventually, tarstream
        // will be cleaned up along with both try_decompress and
        // pkgfile.
     }
   // however, when the IF statement above fails (returns NULL), then
   // without my patch the internal decompress object will NOT be
   // deleted before compress::decompress returns NULL. So,
   // compress::decompress() needs to delete that object, but since
   // we need pkgfile to still be valid in order to do the test below,
   // we have to tell the internal decompress object to relinquish
   // ownership of pkgfile, and NOT delete it during its own destructor.
   // That's why the code here in Installer::installOne is not changed,
   // but rather the fix is contained wholly in compress::decompress
   // and the specialized decompression classes.
   else if ((tarstream = archive::extract (pkgfile)) == NULL)
     {
       /* Not a compressed tarball, not a plain tarball, give up.  */
       delete pkgfile;
       ...


--
Chuck


[-- Attachment #2: setup-fix-leak.changelog --]
[-- Type: text/plain, Size: 792 bytes --]

2008-06-29  Charles Wilson

	* compress.cc (compress::decompress): clean up concrete
	decompressor objects on failure -- but in that case, do
	NOT destroy original io_stream.
	* compress_bz.h (compress_bz::release_original): new method.
	(owns_original): new member variable.
	* compress_bz.cc (compress_bz::release_original): new method.
	(compress_bz::compress_bz): take ownership of parent by default.
	(compress_bz::~compress_bz): only delete original if 
	owns_original is true.
	* compress_gz.h (compress_gz::release_original): new method.
	(owns_original): new member variable.
	* compress_gz.cc (compress_gz::release_original): new method.
	(compress_gz::construct): take ownership of parent by default.
	(compress_gz::~compress_gz): only delete original if 
	owns_original is true.


[-- Attachment #3: setup-fix-leak.patch --]
[-- Type: text/plain, Size: 3744 bytes --]

Index: compress.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/compress.cc,v
retrieving revision 2.4
diff -u -r2.4 compress.cc
--- compress.cc	27 Dec 2004 14:44:35 -0000	2.4
+++ compress.cc	29 Jun 2008 20:56:42 -0000
@@ -42,6 +42,9 @@
 	  compress_gz *rv = new compress_gz (original);
 	  if (!rv->error ())
 	    return rv;
+	  /* else */
+	  rv->release_original();
+	  delete rv;
 	  return NULL;
 	}
       else if (memcmp (magic, "BZh", 3) == 0)
@@ -49,6 +52,9 @@
 	  compress_bz *rv = new compress_bz (original);
 	  if (!rv->error ())
 	    return rv;
+	  /* else */
+	  rv->release_original();
+	  delete rv;
 	  return NULL;
 	}
     }
Index: compress_bz.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/compress_bz.cc,v
retrieving revision 2.12
diff -u -r2.12 compress_bz.cc
--- compress_bz.cc	30 Jul 2007 22:55:49 -0000	2.12
+++ compress_bz.cc	29 Jun 2008 20:56:43 -0000
@@ -33,6 +33,7 @@
       return;
     }
   original = parent;
+  owns_original = true;
 
   initialisedOk = 0;
   bufN = 0;
@@ -202,10 +203,16 @@
   return 0;
 }
 
+void
+compress_bz::release_original ()
+{
+  owns_original = false;
+}
+
 compress_bz::~compress_bz ()
 {
   if (initialisedOk)
     BZ2_bzDecompressEnd (&strm);
-  if (original)
+  if (original && owns_original)
     delete original;
 }
Index: compress_bz.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/compress_bz.h,v
retrieving revision 2.9
diff -u -r2.9 compress_bz.h
--- compress_bz.h	5 Apr 2005 21:37:41 -0000	2.9
+++ compress_bz.h	29 Jun 2008 20:56:43 -0000
@@ -51,10 +51,12 @@
       * over a WAN :} */
   virtual size_t get_size () {return 0;};
   virtual int get_mtime ();
+  virtual void release_original (); /* give up ownership of original io_stream */
   /* if you are still needing these hints... give up now! */
     virtual ~ compress_bz ();
 private:
   io_stream *original;
+  bool owns_original;
   char peekbuf[512];
   size_t peeklen;
   int lasterr;
Index: compress_gz.cc
===================================================================
RCS file: /cvs/cygwin-apps/setup/compress_gz.cc,v
retrieving revision 2.12
diff -u -r2.12 compress_gz.cc
--- compress_gz.cc	8 Apr 2008 23:50:54 -0000	2.12
+++ compress_gz.cc	29 Jun 2008 20:56:43 -0000
@@ -54,6 +54,7 @@
 compress_gz::construct (io_stream * parent, const char *openmode)
 {
   original = parent;
+  owns_original = true;
   peeklen = 0;
   int err;
   int level = Z_DEFAULT_COMPRESSION;	/* compression level */
@@ -429,6 +430,12 @@
 }
 
 void
+compress_gz::release_original ()
+{
+  owns_original = false;
+}
+
+void
 compress_gz::destroy ()
 {
   if (msg)
@@ -450,7 +457,7 @@
     free (inbuf);
   if (outbuf)
     free (outbuf);
-  if (original)
+  if (original && owns_original)
     delete original;
 }
 
Index: compress_gz.h
===================================================================
RCS file: /cvs/cygwin-apps/setup/compress_gz.h,v
retrieving revision 2.6
diff -u -r2.6 compress_gz.h
--- compress_gz.h	5 Apr 2005 21:37:41 -0000	2.6
+++ compress_gz.h	29 Jun 2008 20:56:43 -0000
@@ -54,6 +54,7 @@
   /* Use seek EOF, then tell (). get_size won't do this incase you are sucking down
    * over a WAN :} */
   virtual size_t get_size () {return 0;};
+  virtual void release_original (); /* give up ownership of original io_stream */
   /* if you are still needing these hints... give up now! */
   virtual ~ compress_gz ();
 private:
@@ -70,6 +71,7 @@
   void destroy ();
   int do_flush (int);
   io_stream *original;
+  bool owns_original;
   /* from zlib */
   z_stream stream;
   int z_err;			/* error code for last stream operation */

Re: Fix memleak in setup

[Charles Wilson]

Charles Wilson wrote:
> If compress::decompress returns NULL, we need to clean up the allocated 
> specialized compress_* objects -- but NOT delete the passed-in 
> io_stream*.  The problem occurs Installer::installOne, but is corrected 
> by modifying the behavior of compress::decompress and the various 
> specialized decompression classes.
> 

Any objections if I check this in?

--
Chuck

Re: Fix memleak in setup

[Brian Dessent]

Charles Wilson wrote:

> Any objections if I check this in?

Looks OK to me, go ahead.

Brian

Re: Fix memleak in setup

[Charles Wilson]

Brian Dessent wrote:
> Charles Wilson wrote:
>> Any objections if I check this in?
> Looks OK to me, go ahead.

Done.

--
Chuck