From d7f94e06ca96f475ea94b15ec44b57aac110ff24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Trzci=C5=84ski?= Date: Tue, 24 Oct 2017 11:09:23 +0100 Subject: [PATCH 1/3] Fix memory leaks in enum.cpp Unfortunately due to optimised build of Python3 libraries and executable I got only partial stack from [http://clang.llvm.org/docs/AddressSanitizer.html], however digging into and reducing my code I tracked it down to be issue with `boost/libs/python/src/object/enum.cpp`. It has to bits that leak (and comment mentioning there is one): PyObject *mod = PyObject_GetAttrString( self_, "__module__"); Leaks reference, as it never decreases it. It also stores a new string object under object's `name` that ref count never gets decremented. That commit fixes both issues. --- src/object/enum.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/object/enum.cpp b/src/object/enum.cpp index 3063320cb0..4d73ee23d4 100644 --- a/src/object/enum.cpp +++ b/src/object/enum.cpp @@ -34,11 +34,17 @@ static PyMemberDef enum_members[] = { extern "C" { + static void + enum_dealloc(enum_object* self) + { + Py_XDECREF(self->name); + Py_TYPE(self)->tp_free((PyObject*)self); + } + static PyObject* enum_repr(PyObject* self_) { - // XXX(bhy) Potentional memory leak here since PyObject_GetAttrString returns a new reference - // const char *mod = PyString_AsString(PyObject_GetAttrString( self_, const_cast("__module__"))); PyObject *mod = PyObject_GetAttrString( self_, "__module__"); + object auto_free(handle<>(mod)); enum_object* self = downcast(self_); if (!self->name) { @@ -88,7 +94,7 @@ static PyTypeObject enum_type_object = { const_cast("Boost.Python.enum"), sizeof(enum_object), /* tp_basicsize */ 0, /* tp_itemsize */ - 0, /* tp_dealloc */ + (destructor) enum_dealloc, /* tp_dealloc */ 0, /* tp_print */ 0, /* tp_getattr */ 0, /* tp_setattr */ From 7c3240de7193a0d31325f51175cc1eb6e4211bc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Trzci=C5=84ski?= Date: Tue, 7 Nov 2017 09:17:05 +0000 Subject: [PATCH 2/3] Fix error propagation in from module init. When exception gets set we need to return NULL in Python3 as module pointer. Code in `Python/importdl.c` explicitly checks for this and suppresses exception (so you lose any knowledge of what went wrong): PyObject * _PyImport_LoadDynamicModuleWithSpec(PyObject *spec, FILE *fp) { [...] m = p0(); _Py_PackageContext = oldcontext; if (m == NULL) { if (!PyErr_Occurred()) { PyErr_Format( PyExc_SystemError, "initialization of %s failed without raising an exception", name_buf); } goto error; } else if (PyErr_Occurred()) { PyErr_Clear(); PyErr_Format( PyExc_SystemError, "initialization of %s raised unreported exception", name_buf); m = NULL; goto error; } In above the first case propagates actually raised exception, but only if we returned NULL. Otherwise it overrides it with very unhelpful message of "unreported exception". Fix is to simply return NULL if we got exception raised inside `init_function`. Handling of that case follows what I could find in `Modules/symtablemodule.c` bundled with Python: PyMODINIT_FUNC PyInit__symtable(void) { PyObject *m; [...] if (PyErr_Occurred()) { Py_DECREF(m); m = 0; } return m; } So it checks if exception was raised, and if so it frees module and returns NULL. --- src/module.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/module.cpp b/src/module.cpp index 9628481996..6ed1ae7a80 100644 --- a/src/module.cpp +++ b/src/module.cpp @@ -21,7 +21,14 @@ namespace object m_obj(((borrowed_reference_t*)m)); scope current_module(m_obj); - handle_exception(init_function); + // If an exception happened we have to return NULL + if (handle_exception(init_function)) + { +#if PY_VERSION_HEX >= 0x03000000 + Py_DECREF(m); + m = 0; +#endif + } } return m; From 1d19e6f5300523679e26fe73fff52dc77e92cdea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Trzci=C5=84ski?= Date: Tue, 7 Nov 2017 09:23:42 +0000 Subject: [PATCH 3/3] Fix memory leak in `gcc_demangle` `__cxa_demangle` returns a new pointer that needs to be freed. As much as this is only a minor leak (tenths of bytes per each unique symbol that got resolved) it makes automated memory leak checking tools quite upset. This simply adds a helper that frees memory for names we've kept. Also, removed a lot of trailing whitespace. --- src/converter/type_id.cpp | 45 +++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/src/converter/type_id.cpp b/src/converter/type_id.cpp index c6a8bf7a04..4cdbe4194a 100644 --- a/src/converter/type_id.cpp +++ b/src/converter/type_id.cpp @@ -3,6 +3,7 @@ // accompanying file LICENSE_1_0.txt or copy at // http://www.boost.org/LICENSE_1_0.txt) +#include #include #include #include @@ -18,9 +19,9 @@ #if !defined(__GNUC__) || __GNUC__ >= 3 || __SGI_STL_PORT || __EDG_VERSION__ # include -#else +#else # include -#endif +#endif # ifdef BOOST_PYTHON_HAVE_GCC_CP_DEMANGLE # if defined(__GNUC__) && __GNUC__ >= 3 @@ -34,7 +35,7 @@ class __class_type_info; # include # endif -# endif +# endif #endif /* defined(__QNXNTO__) */ namespace boost { namespace python { @@ -76,18 +77,31 @@ namespace return std::strcmp(x.first,y.first) < 0; } }; - + struct free_mem { free_mem(char*p) : p(p) {} - + ~free_mem() { std::free(p); } char* p; }; + + struct free_list + { + std::vector container; + + ~free_list() + { + BOOST_FOREACH(char * item, container) + { + free(item); + } + } + }; } bool cxxabi_cxa_demangle_is_broken() @@ -112,23 +126,25 @@ namespace detail typedef std::vector< std::pair > mangling_map; - + + static free_list demangler_free_mem; static mangling_map demangler; + mangling_map::iterator p = std::lower_bound( demangler.begin(), demangler.end() , std::make_pair(mangled, (char const*)0) , compare_first_cstring()); - + if (p == demangler.end() || strcmp(p->first, mangled)) { int status; free_mem keeper( cxxabi::__cxa_demangle(mangled, 0, 0, &status) ); - + assert(status != -3); // invalid argument error - + if (status == -1) { throw std::bad_alloc(); @@ -181,10 +197,17 @@ namespace detail } p = demangler.insert(p, std::make_pair(mangled, demangled)); - keeper.p = 0; + // We are keeping demangled reference. + // Transfer ownership to helper list that will free it + // on teardown. + if (keeper.p == demangled) + { + demangler_free_mem.container.push_back(keeper.p); + keeper.p = 0; + } } } - + return p->second; } }