Re: Memory leaks in apt

[David Kalnischkies <kalnischkies+debian@gmail.com>]

Hello all,
Hi Julian Andres Klode,

(btw: It is not only _config - _error is basicly the same)

i am maybe wrong, but:
The memory is freed by the operation system anyway after the program exits
and both variables are (possibly) used till the end of the program,
so a free/delete just before the final return seems useless to me.
Also to really fix it, we have to free the two variables on all possible
exit points in the program - this could be simplified with a registered
atexit() method, even included in the library for all programs,
but again for me it feels a bit redundant to do all this work just to free
some memory which would be freed by the operation system a few microseconds
later anyway. (I know that this slant could lead to a "APT is not a deamon
so it will exit in a short while anyway -> why fixing any memory leak?" -
but this would be really bad style imo)

This said, it is maybe a good idea to add both deletes at the very end of
a normal run just to please valgrind and to be able to say in (a far away)
future that APT is totally memory leak free. ;)

And to finally add some usefulness to this email:
The attached patch should fix some (very) minor¹ leaks by adding a few
destructors to the classes debReleaseIndex, debSrcRecordParser, metaIndex
and CacheFile (the first three are in apt-pkg [ABI], the last in apt-get)


Best regards / Mit freundlichen Grüßen,

David "DonKult" Kalnischkies


¹ I test it with a simulation of a ~200 package dist-upgrade:
less than 100k bytes saved...

=== modified file 'apt-pkg/deb/debmetaindex.cc'
--- apt-pkg/deb/debmetaindex.cc	2008-11-16 13:58:02 +0000
+++ apt-pkg/deb/debmetaindex.cc	2009-06-16 13:16:15 +0000
@@ -115,6 +115,13 @@
    this->Type = "deb";
 }
 
+debReleaseIndex::~debReleaseIndex()
+{
+   for (vector<const debSectionEntry *>::const_iterator I = SectionEntries.begin();
+	I != SectionEntries.end(); I++)
+      delete *I;
+}
+
 vector <struct IndexTarget *>* debReleaseIndex::ComputeIndexTargets() const
 {
    vector <struct IndexTarget *>* IndexTargets = new vector <IndexTarget *>;

=== modified file 'apt-pkg/deb/debmetaindex.h'
--- apt-pkg/deb/debmetaindex.h	2006-12-14 11:39:29 +0000
+++ apt-pkg/deb/debmetaindex.h	2009-06-16 13:16:15 +0000
@@ -22,6 +22,7 @@
    public:
 
    debReleaseIndex(string URI, string Dist);
+   ~debReleaseIndex();
 
    virtual string ArchiveURI(string File) const {return URI + File;};
    virtual bool GetIndexes(pkgAcquire *Owner, bool GetAll=false) const;

=== modified file 'apt-pkg/deb/debsrcrecords.cc'
--- apt-pkg/deb/debsrcrecords.cc	2008-11-16 13:58:02 +0000
+++ apt-pkg/deb/debsrcrecords.cc	2009-06-16 13:16:15 +0000
@@ -152,3 +152,11 @@
    return true;
 }
 									/*}}}*/
+// SrcRecordParser::~SrcRecordParser - Destructor			/*{{{*/
+// ---------------------------------------------------------------------
+/* */
+debSrcRecordParser::~debSrcRecordParser()
+{
+   delete[] Buffer;
+}
+									/*}}}*/

=== modified file 'apt-pkg/deb/debsrcrecords.h'
--- apt-pkg/deb/debsrcrecords.h	2008-11-16 13:58:02 +0000
+++ apt-pkg/deb/debsrcrecords.h	2009-06-16 13:16:15 +0000
@@ -50,6 +50,7 @@
    debSrcRecordParser(string File,pkgIndexFile const *Index) 
       : Parser(Index), Fd(File,FileFd::ReadOnly), Tags(&Fd,102400), 
         Buffer(0), BufSize(0) {}
+   ~debSrcRecordParser();
 };
 
 #endif

=== modified file 'apt-pkg/metaindex.h'
--- apt-pkg/metaindex.h	2006-12-14 11:39:29 +0000
+++ apt-pkg/metaindex.h	2009-06-16 13:16:15 +0000
@@ -39,7 +39,11 @@
    virtual vector<pkgIndexFile *> *GetIndexFiles() = 0; 
    virtual bool IsTrusted() const = 0;
 
-   virtual ~metaIndex() {};
+   virtual ~metaIndex() {
+      for (vector<pkgIndexFile *>::iterator I = (*Indexes).begin(); I != (*Indexes).end(); ++I)
+	 delete *I;
+      delete Indexes;
+   }
 };
 
 #endif

=== modified file 'cmdline/apt-get.cc'
--- cmdline/apt-get.cc	2009-06-13 16:00:57 +0000
+++ cmdline/apt-get.cc	2009-06-16 13:16:15 +0000
@@ -111,6 +111,9 @@
 	 return Open(true);
    }
    CacheFile() : List(0) {};
+   ~CacheFile() {
+      delete[] List;
+   }
 };
 									/*}}}*/
 // IsAutoInstallOk - implements a few MarkInstall package checks	/*{{{*/