Memory leak with SWIG

Question

I have a class that upon initialization, reserves an array of pointers with "new". Upon destruction, that array of pointers is released with "delete[]".

There's no problem when running the code without python. However, when I "swig'it" and use it as a python module, something weird happens. The destructor is called correctly upon garbage collection, but when doing that, a memory leak occurs! A complete mystery (at least to me). Help highly appreciated!

(1) Preliminaries to get it compiled:

setup.py

from setuptools import setup, Extension, find_packages
import os
import copy
import sys

def make_pps():
  d=[]
  c=[]
  l=[]
  lib=[]
  s=[]
  s+=["-c++"]
  return Extension("_pps",sources=["pps.i","pps.cpp"],include_dirs=d,extra_compile_args=c,extra_link_args=l,libraries=lib,swig_opts=s)

ext_modules=[]
ext_modules.append(make_pps())  

setup(
  name = "pps",
  version = "0.1",
  packages = find_packages(),
  install_requires = ['docutils>=0.3'],
  ext_modules=ext_modules
)

pps.i

%module pps
%{
#define SWIG_FILE_WITH_INIT
%}

%inline %{


class MemoryManager {

public:
  MemoryManager(int n);
  ~MemoryManager();
};

%}

(2) The C++ code itself:

pps.cpp

#include <iostream>                             
#include <stdio.h>
#include <typeinfo>
#include <sys/time.h>
#include <stdint.h>  
#include <cmath>                       

#include <cstring>
#include <string.h>
#include <stdlib.h>

#include<sys/ipc.h> // shared memory
#include<sys/shm.h>


/*
Stand-alone cpp program: 
  - UNComment #define USE_MAIN (see below)
  - compile with

    g++ pps.cpp

  - run with

    ./a.out

    => OK

  - Check memory leaks with

    valgrind ./a.out

    => ALL OK/CLEAN


Python module:
  - Comment (i.e. use) the USE_MAIN switch (see below)
  - compile with

    python3 setup.py build_ext; cp build/lib.linux-x86_64-3.5/_pps.cpython-35m-x86_64-linux-gnu.so ./_pps.so

  - run with

    python3 test.py

    => CRASHHHHH

  - Check memory leaks with

    valgrind python3 test.py

    => Whoa..

  - Try to enable/disable lines marked with "BUG?" .. 

*/

// #define USE_MAIN 1 // UNcomment this line to get stand-alone c-program


using std::cout; 
using std::endl;
using std::string;


class MemoryManager {

public:
  MemoryManager(int n);
  ~MemoryManager();

private:
  int nmax;
  int nc; // next index to be used
  uint nsum;
  int* ids;
  void** buffers;
};


MemoryManager::MemoryManager(int n) : nmax(n),nc(0) {
  cout << "MemoryManager: nmax="<<this->nmax<<"\n";

  this->buffers  =new void*[this->nmax]; // BUG?
  this->ids      =new int  [this->nmax];
  this->nsum     =0;
}


MemoryManager::~MemoryManager() {
  printf("MemoryManager: destructor\n");
  delete[] this->buffers;               // BUG?
  delete[] this->ids;
  printf("MemoryManager: destructor: bye\n");
}


#ifdef USE_MAIN
int main(int argc, char *argv[]) {
  MemoryManager* m;

  m=new MemoryManager(1000);
  delete m;

  m=new MemoryManager(1000);
  delete m;
}
#endif

(3) A test python program:

test.py

from pps import MemoryManager
import time

print("creating MemoryManager")
mem=MemoryManager(1000)
time.sleep(1)
print("clearing MemoryManager")
mem=None
print("creating MemoryManager (again)")
time.sleep(1)
mem=MemoryManager(1000)
time.sleep(1)
print("exit")

Compile with:

python3 setup.py build_ext; cp build/lib.linux-x86_64-3.5/_pps.cpython-35m-x86_64-linux-gnu.so ./_pps.so

Run with:

python3 test.py

EDIT AND OFF-TOPIC

As a question of this characteristics always attracts people who think they can fix everything by using containers instead of raw pointer structures, here is the container version (might be wrong .. not used vectors much, but anyway, its off-topic):

class MemoryManager {

public:
  MemoryManager(int n);
  ~MemoryManager();

private:
  int nmax;
  int nc; // next index to be used
  uint nsum;

  // "ansi" version
  //int* ids;
  //void** buffers;

  // container version
  vector<int>   ids;
  vector<void*> buffers;
  // vector<shared_ptr<int>> buffers; // I feel so modern..
};


MemoryManager::MemoryManager(int n) : nmax(n),nc(0) {
  cout << "MemoryManager: nmax="<<this->nmax<<"\n";

  /* // "ansi" version
  this->buffers  =new void*[this->nmax]; // BUG?
  this->ids      =new int  [this->nmax];
  */

  // container version
  this->ids.reserve(10);
  this->buffers.reserve(10);

  this->nsum     =0;
}


MemoryManager::~MemoryManager() {
  printf("MemoryManager: destructor\n");

  /* // "ansi" version
  delete[] this->buffers;               // BUG?
  delete[] this->ids;
  */
  printf("MemoryManager: destructor: bye\n");
}

Doesn't work either

FINAL REMARKS

Thank you for Flexos for amazing/detailed analysis.

My initial reason for using inconsistent class declaration was, that I routinely don't want to expose all details of my class in python.

I forgot that in Swig interface file, everything that's put in the

%{..}

is prepended into the generated wrapper code .. now that was missing, so the wrapper code got the class declaration only from the %inline section..!

We can still have a minimal part of a the class wrapped, with the following pps.i file:

%module pps
%{
#define SWIG_FILE_WITH_INIT
#include "pps.h"
%}

class MemoryManager {

public:
  MemoryManager(int n);
  ~MemoryManager();
};

Where "pps.h" should have the correct class declaration. Now #included "pps.h" appears in the beginning of "pps_wrap.cpp".

The "class declaration" in "pps.i" only tells Swig what we want to wrap ..

.. right..? (no more memory errors, at least)

Answer 1

You've got some undefined behaviour here. And it's a fun one so let's take a close look. For those of you who want to follow along I've got an ARM device handy so I'll be working with a disassembly from that to show what the impact is.

The root cause of this stems from the fact that you've used %inline incorrectly. What's ended up happening is that you've shown your compiler two different definitions of the class MemoryManager. In your .i file you wrote:

%inline %{
class MemoryManager {    
public:
  MemoryManager(int n);
  ~MemoryManager();
};
%}

What's important in this definition of MemoryManager is two things:

1. Since you used %inline this specific definition of the class will be seen by both SWIG and the C++ compiler that compiles the wrap_pps.cpp translation unit.
2. There are no member variables, public or private included in this definition. So the size/layout is going to be wrong. (And it is a definition of the class not a declaration even though the constructor/destructor are only declarations).

Since there are no member variables (and empty base class optimisation doesn't apply here) the size of this class is at least one byte (so it's uniquely addressable), but there's no guarantee that it's anything more than 1 byte. Which is clearly wrong, because in the translation unit where you define the constructor and destructor you've told the compiler that it's much bigger than just 1 byte. From this point onwards all bets are off and the compiler is free to do whatever it wants. But in this instance let's look at what it's actually done on ARM.

First things first I ran: objdump -d build/temp.linux-armv7l-3.4/pps_wrap.o |c++filt and looked through for the SWIG generated _wrap_new_MemoryManager symbol. This function is the bridge between creating a new Python instance and calling new in C++, we're looking here because that's where the bug manifests itself (in this instance). Most of the instructions are irrelevant, so in the interest of brevity I've snipped a bunch of irrelevant stuff:

00000930 <_wrap_new_MemoryManager>:
     930:       4b20            ldr     r3, [pc, #128]  ; (9b4 <_wrap_new_MemoryManager+0x84>)
     932:       4608            mov     r0, r1
     934:       b570            push    {r4, r5, r6, lr}
     ...
     # [snip]
     ...
     966:       2001            movs    r0, #1  <-- THIS IS IT
     968:       f7ff fffe       bl      0 <operator new(unsigned int)>
     96c:       4631            mov     r1, r6
     96e:       4604            mov     r4, r0
     970:       f7ff fffe       bl      0 <MemoryManager::MemoryManager(int)>

r0 is the first argument of operator new(unsigned int). In the code above the compiler has set it to be only 1 byte being allocated. That's wrong and it's done it because, in this translation unit the definition of MemoryManager only needs 1 byte of storage to satisfy the uniquely addressable requirement.

So when we call MemoryManager::MemoryManager(int) the pointer pointed to by this is only a 1 byte heap allocation. As soon as we read/write past that by we're doing bad things to our heap. At this point all bets really are off. Something else can get allocated there later, anything the Python runtime needs. Or the allocations that your vector/new[] call produce. Or it could just be unmapped memory, or something else all together. But whatever happens it's bad.

For comparison if we compile your C++ with the main function enabled (g++ -Wall -Wextra pps.cpp -DUSE_MAIN -S to sidestep objdump) instead we get this:

main:
        .fnstart
.LFB1121:
        @ args = 0, pretend = 0, frame = 16
        @ frame_needed = 1, uses_anonymous_args = 0
        push    {r4, r7, lr}
        .save {r4, r7, lr}
        .pad #20
        sub     sp, sp, #20
        .setfp r7, sp, #0
        add     r7, sp, #0
        str     r0, [r7, #4]
        str     r1, [r7]
        movs    r0, #20    <-- This looks much more sensible!
.LEHB0:
        bl      operator new(unsigned int)
.LEHE0:
        mov     r4, r0
        mov     r0, r4
        mov     r1, #1000
.LEHB1:
        bl      MemoryManager::MemoryManager(int)

(I was slightly surprised that gcc generated that code at default optimisation settings)

So to actually fix the problem I'd recommend doing one of two things:

1. Put all your C++ inside %inline and don't have any other .cpp file at all.
2. Move the definition of the class into a .h file and use %include + #include to reference that consistent definition everywhere.

For all but the smallest projects number 2 is the best choice in my view. So your SWIG file would become simply:

%module pps
%{
#define SWIG_FILE_WITH_INIT
#include "pps.h"
%}

%include "pps.h"
%}

(You've also violated the rule of three in the self-managed buffer code as well as not providing strong exception safety guarantees that you get for free in the vector based code, so it really is worth sticking with that).