Multiple memory leak in ALL versions of python-apt

[Evan Mezeske]

Binary package hint: python-apt

This bug report references code from python/acquire.cc and python/generic.h. I think it applies to *all* versions of python-apt.

As far as I can tell, every single use of the CppPyObject<T> template with a pointer type for the template parameter T results in a memory leak. To give a concrete example, consider the use of the template in python/acquire.cc:

    pkgAcqFile *af = new pkgAcqFile(/* ... */);
    CppPyObject<pkgAcqFile*> *AcqFileObj = CppPyObject_NEW<pkgAcqFile*>(&PkgAcquireFileType);
    AcqFileObj->Object = af;

The function registered to deallocate the CppPyObject<pkgAcqFile*> is CppDealloc<>(). Here is its existing implementation:

    template <class T>
    void CppDealloc(PyObject *Obj)
    {
       GetCpp<T>(Obj).~T();
       PyMem_DEL(Obj);
    }

Notice that it calls the destructor for the contained object. That works when T is a non-pointer type. However, when T is a pointer type, the deconstructor being called is *that of the pointer, not what it points to*. I.e. the destructor call resembles this:

    typedef char* T;
    T a = new char;
    a.~T();

Which of course doesn't really make sense. Calling a.~T() doesn't free the memory pointed to by a. So, when CppDealloc<>() is called with a pointer type, you probably want this:

    template <class T>
    void CppDealloc(PyObject *Obj)
    {
       delete GetCpp<T>(Obj)
       PyMem_DEL(Obj);
    }

I see several ways to fix this:
    1) don't use pointers with CppPyObject<>
    2) add a template specialization like CppPyObject<T*> that uses delete instead of calling the destructor directly
    3) use smart pointers (std::auto_ptr, or better, boost::shared_ptr) instead of naked pointers

The second option would be easy to implement, but what if you wanted a CppPyObject to contain a pointer without deleting it? The first option is too restrictive. Probably, the third option makes the most sense.

[Julian Andres Klode (juliank)]

> 2) add a template specialization like CppPyObject<T*> that uses delete instead of calling the destructor directly
The problem here is that AFAIK there is no way to define such template specializations for functions. We could still introduce new functions CppDeallocPtr() CppOwnedDeallocPtr().

> 3) use smart pointers (std::auto_ptr, or better, boost::shared_ptr) instead of naked pointers
If python-apt were to use smart pointers, this could be a problem for applications wanting to provide their objects in python-apt using the planned C++ API of the upcoming python-apt 0.8. Furthemore it does not make sense to have two reference counters for basically one 'thing' (the CppPyObject itself and the pointer to the C++ object).

> The second option would be easy to implement, but what if you wanted a CppPyObject to contain a pointer without > deleting it? The first option is too restrictive. Probably, the third option makes the most sense.

The third option makes no sense because we would force all programs wanting to export their objects to python to use smart pointers. The second option makes most sense here, but we should introduce a new member 'ShouldDealloc' or similar which can be set in CppPyObject_NEW()/CppOwnedPyObject_NEW() and prevents the deallocation. This way, providing a C++ API still works and the program is still responsible for deleting the object, instead of some class somewhere.

I have committed the attached patch to the jak branch and it will be part of the 0.7.92 release.

[Julian Andres Klode (juliank)]

BTW; here is the log message for this patch:

revno: 271
committer: Julian Andres Klode <email address hidden>
branch nick: jak
timestamp: Fri 2009-06-12 18:56:23 +0200
message:
  Bugfix: Delete pointers correctly, fixing memory leaks. (LP: #370149)

  We previously called the destructor of the pointer. This resulted in no
  object using pointers being deallocated.

  This patch introduces CppDeallocPtr() and CppOwnedDeallocPtr() which do
  the same as the other CppDealloc() and CppOwnedDealloc(), but use 'delete'
  on the pointer instead of the deconstructor.

  Furthermore, this patch also changes AcquireFile to be a CppOwnedPyObject,
  owned by the Acquire object. Without this change, deleting the Acquire
  object would cause a crash when AcquireFile is deallocated.

[Julian Andres Klode (juliank)]

The freeze exception request for 0.7.93.4 is in bug #531518. It's not possible to backport the needed fixes to older versions of python-apt, BTW.

[Julian Andres Klode (juliank)]

This bug was fixed in the package python-apt - 0.7.94.2ubuntu1 (as part of the merge)

---------------

python-apt (0.7.92) experimental; urgency=low

  * New features:
    - Provide a C++ API in the package python-apt-dev (Closes: #334923).
    - Add apt_pkg.HashString and apt_pkg.IndexRecords (Closes: #456141).
    - Add apt_pkg.Policy class (Closes: #382725).
    - Add apt_pkg.Hashes class.
    - Allow types providing __new__() to be subclassed.
    - Add apt_pkg.DepCache.mark_auto() and apt.Package.mark_auto() methods to
      mark a package as automatically installed.
    - Make AcquireFile a subclass of AcquireItem, thus inheriting attributes.
    - New progress handling in apt.progress.base and apt.progress.text. Still
      missing Qt4 progress handlers.
    - Classes in apt_inst (Closes: #536096)
      + You can now use apt_inst.DebFile.data to access the data.tar.* member
        regardless of its compression (LP: #44493)

  * Unification of dependency handling:
    - apt_pkg.parse_[src_]depends() now use CompType instead of CompTypeDeb
      (i.e. < instead of <<) to match the interface of Version.depends_list_str
    - apt_pkg.SourceRecords.build_depends matches exactly the interface of
      Version.depends_list_str just with different keys (e.g. Build-Depends).
      + Closes: #468123 - there is no need anymore for binding CompType or
        CompTypeDeb, because we don't return integer values for CompType
        anymore.

  * Bugfixes:
    - Delete pointers correctly, fixing memory leaks (LP: #370149).
    - Drop open() and close() in apt_pkg.Cache as they cause segfaults.
    - Raise ValueError in AcquireItem if the Acquire process is shut down
      instead of segfaulting.

  * Other stuff:
    - Merge releases 0.7.10.4 - 0.7.12.1 from unstable.
    - Merge Configuration,ConfigurationPtr,ConfigurationSub into one type.
    - Simplify the whole build process by using a single setup.py.
    - The documentation has been restructured and enhanced with tutorials.
    - Only recommend lsb-release instead of depending on it. Default to
      Debian unstable if lsb_release is not available.

 -- Julian Andres Klode <email address hidden> Tue, 18 Aug 2009 16:42:56 +0200