From 178ef391c7f0e035de074b4d0617ca6daae0498b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Th=C3=BCmmel?= Date: Wed, 28 Nov 2018 17:16:12 +0100 Subject: [PATCH] Fix of important memory leak in Python module Dear all, As described in the issue https://github.com/syoyo/tinyobjloader/issues/188#issue-385341218 there is a memory leak in the function pyLoadObj(). The reference count for all created Python objects must be decreased after they are fed into the lists and dictionaries. The only exception from this is the function PyTuple_SetItem() in pyTupleFromfloat3(), which decreases the reference counter of the Python object *tuple automatically by calling this function. In all other cases like PyList_Insert(), the references are only borrowed and not decreased. Therefore, each created Python object will remain in the heap, since there is one reference to it left in the counter. These facts are explained in https://docs.python.org/2/c-api/intro.html#reference-counts in detail. Best regards Martin. P.S. sorry, that i did not put that much effort into a more readable code and just inserted the Py_DECREF() function at every necessary position. --- python/main.cpp | 148 +++++++++++++++++++++++++----------------------- 1 file changed, 77 insertions(+), 71 deletions(-) diff --git a/python/main.cpp b/python/main.cpp index 4f1d0e0..1922a32 100644 --- a/python/main.cpp +++ b/python/main.cpp @@ -18,11 +18,9 @@ typedef std::vector vecti; PyObject* pyTupleFromfloat3(float array[3]) { int i; PyObject* tuple = PyTuple_New(3); - for (i = 0; i <= 2; i++) { PyTuple_SetItem(tuple, i, PyFloat_FromDouble(array[i])); } - return tuple; } @@ -30,32 +28,27 @@ extern "C" { static PyObject* pyLoadObj(PyObject* self, PyObject* args) { PyObject *rtndict, *pyshapes, *pymaterials, *pymaterial_indices, *attribobj, *current, *meshobj; - char const* current_name; char const* filename; vectd vect; std::vector indices; std::vector face_verts; - tinyobj::attrib_t attrib; std::vector shapes; std::vector materials; if (!PyArg_ParseTuple(args, "s", &filename)) return NULL; - std::string err; - tinyobj::LoadObj(&attrib, &shapes, &materials, &err, filename); - + std::string err, warn; + tinyobj::LoadObj(&attrib, &shapes, &materials, &warn, &err, filename); pyshapes = PyDict_New(); pymaterials = PyDict_New(); pymaterial_indices = PyList_New(0); rtndict = PyDict_New(); - attribobj = PyDict_New(); for (int i = 0; i <= 2; i++) { current = PyList_New(0); - switch (i) { case 0: current_name = "vertices"; @@ -72,124 +65,137 @@ static PyObject* pyLoadObj(PyObject* self, PyObject* args) { } for (vectd::iterator it = vect.begin(); it != vect.end(); it++) { - PyList_Insert(current, it - vect.begin(), PyFloat_FromDouble(*it)); + PyObject* value = PyFloat_FromDouble(*it); + PyList_Insert(current, it - vect.begin(), value); + Py_DECREF(value); } - PyDict_SetItemString(attribobj, current_name, current); + Py_DECREF(current); } - for (std::vector::iterator shape = shapes.begin(); - shape != shapes.end(); shape++) { + for (std::vector::iterator shape = shapes.begin(); shape != shapes.end(); shape++) { meshobj = PyDict_New(); tinyobj::mesh_t cm = (*shape).mesh; { current = PyList_New(0); - for (size_t i = 0; i < cm.indices.size(); i++) { // Flatten index array: v_idx, vn_idx, vt_idx, v_idx, vn_idx, vt_idx, - // ... - PyList_Insert(current, 3 * i + 0, - PyLong_FromLong(cm.indices[i].vertex_index)); - PyList_Insert(current, 3 * i + 1, - PyLong_FromLong(cm.indices[i].normal_index)); - PyList_Insert(current, 3 * i + 2, - PyLong_FromLong(cm.indices[i].texcoord_index)); + PyObject* value = PyLong_FromLong(cm.indices[i].vertex_index); + PyList_Insert(current, 3 * i + 0, value); + Py_DECREF(value); + value = PyLong_FromLong(cm.indices[i].normal_index); + PyList_Insert(current, 3 * i + 1, value); + Py_DECREF(value); + value = PyLong_FromLong(cm.indices[i].texcoord_index); + PyList_Insert(current, 3 * i + 2, value); + Py_DECREF(value); } - PyDict_SetItemString(meshobj, "indices", current); + Py_DECREF(current); } { current = PyList_New(0); - for (size_t i = 0; i < cm.num_face_vertices.size(); i++) { // Widen data type to long. - PyList_Insert(current, i, PyLong_FromLong(cm.num_face_vertices[i])); + PyObject* value = PyLong_FromLong(cm.num_face_vertices[i]); + PyList_Insert(current, i, value); + Py_DECREF(value); } - PyDict_SetItemString(meshobj, "num_face_vertices", current); + Py_DECREF(current); } { current = PyList_New(0); - for (size_t i = 0; i < cm.material_ids.size(); i++) { - PyList_Insert(current, i, PyLong_FromLong(cm.material_ids[i])); + PyObject* value = PyLong_FromLong(cm.material_ids[i]); + PyList_Insert(current, i, value); + Py_DECREF(value); } - PyDict_SetItemString(meshobj, "material_ids", current); + Py_DECREF(current); } - PyDict_SetItemString(pyshapes, (*shape).name.c_str(), meshobj); + Py_DECREF(meshobj); } - for (std::vector::iterator mat = materials.begin(); - mat != materials.end(); mat++) { + for (std::vector::iterator mat = materials.begin(); mat != materials.end(); mat++) { PyObject* matobj = PyDict_New(); PyObject* unknown_parameter = PyDict_New(); - for (std::map::iterator p = - mat->unknown_parameter.begin(); - p != mat->unknown_parameter.end(); ++p) { - PyDict_SetItemString(unknown_parameter, p->first.c_str(), - PyUnicode_FromString(p->second.c_str())); + for (std::map::iterator p = mat->unknown_parameter.begin(); p != mat->unknown_parameter.end(); ++p) { + PyObject* value = PyUnicode_FromString(p->second.c_str()); + PyDict_SetItemString(unknown_parameter, p->first.c_str(), value); + Py_DECREF(value); } - PyDict_SetItemString(matobj, "shininess", - PyFloat_FromDouble(mat->shininess)); - PyDict_SetItemString(matobj, "ior", PyFloat_FromDouble(mat->ior)); - PyDict_SetItemString(matobj, "dissolve", - PyFloat_FromDouble(mat->dissolve)); - PyDict_SetItemString(matobj, "illum", PyLong_FromLong(mat->illum)); - PyDict_SetItemString(matobj, "ambient_texname", - PyUnicode_FromString(mat->ambient_texname.c_str())); - PyDict_SetItemString(matobj, "diffuse_texname", - PyUnicode_FromString(mat->diffuse_texname.c_str())); - PyDict_SetItemString(matobj, "specular_texname", - PyUnicode_FromString(mat->specular_texname.c_str())); - PyDict_SetItemString( - matobj, "specular_highlight_texname", - PyUnicode_FromString(mat->specular_highlight_texname.c_str())); - PyDict_SetItemString(matobj, "bump_texname", - PyUnicode_FromString(mat->bump_texname.c_str())); - PyDict_SetItemString( - matobj, "displacement_texname", - PyUnicode_FromString(mat->displacement_texname.c_str())); - PyDict_SetItemString(matobj, "alpha_texname", - PyUnicode_FromString(mat->alpha_texname.c_str())); + PyObject* value = PyFloat_FromDouble(mat->shininess); + PyDict_SetItemString(matobj, "shininess", value); + Py_DECREF(value); + value = PyFloat_FromDouble(mat->ior); + PyDict_SetItemString(matobj, "ior", value); + Py_DECREF(value); + value = PyFloat_FromDouble(mat->dissolve); + PyDict_SetItemString(matobj, "dissolve", value); + Py_DECREF(value); + value = PyLong_FromLong(mat->illum); + PyDict_SetItemString(matobj, "illum", value); + Py_DECREF(value); + value = PyUnicode_FromString(mat->ambient_texname.c_str()); + PyDict_SetItemString(matobj, "ambient_texname", value); + Py_DECREF(value); + value = PyUnicode_FromString(mat->diffuse_texname.c_str()); + PyDict_SetItemString(matobj, "diffuse_texname", value); + Py_DECREF(value); + value = PyUnicode_FromString(mat->specular_texname.c_str()); + PyDict_SetItemString(matobj, "specular_texname", value); + Py_DECREF(value); + value = PyUnicode_FromString(mat->specular_highlight_texname.c_str()); + PyDict_SetItemString(matobj, "specular_highlight_texname", value); + Py_DECREF(value); + value = PyUnicode_FromString(mat->bump_texname.c_str()); + PyDict_SetItemString(matobj, "bump_texname", value); + Py_DECREF(value); + value = PyUnicode_FromString(mat->displacement_texname.c_str()); + PyDict_SetItemString(matobj, "displacement_texname", value); + Py_DECREF(value); + value = PyUnicode_FromString(mat->alpha_texname.c_str()); + PyDict_SetItemString(matobj, "alpha_texname", value); + Py_DECREF(value); PyDict_SetItemString(matobj, "ambient", pyTupleFromfloat3(mat->ambient)); PyDict_SetItemString(matobj, "diffuse", pyTupleFromfloat3(mat->diffuse)); - PyDict_SetItemString(matobj, "specular", - pyTupleFromfloat3(mat->specular)); - PyDict_SetItemString(matobj, "transmittance", - pyTupleFromfloat3(mat->transmittance)); - PyDict_SetItemString(matobj, "emission", - pyTupleFromfloat3(mat->emission)); + PyDict_SetItemString(matobj, "specular", pyTupleFromfloat3(mat->specular)); + PyDict_SetItemString(matobj, "transmittance", pyTupleFromfloat3(mat->transmittance)); + PyDict_SetItemString(matobj, "emission", pyTupleFromfloat3(mat->emission)); PyDict_SetItemString(matobj, "unknown_parameter", unknown_parameter); - + Py_DECREF(unknown_parameter); PyDict_SetItemString(pymaterials, mat->name.c_str(), matobj); - PyList_Append(pymaterial_indices, PyUnicode_FromString(mat->name.c_str())); + Py_DECREF(matobj); + value = PyUnicode_FromString(mat->name.c_str()); + PyList_Append(pymaterial_indices, value); + Py_DECREF(value); } PyDict_SetItemString(rtndict, "shapes", pyshapes); + Py_DECREF(pyshapes); PyDict_SetItemString(rtndict, "materials", pymaterials); + Py_DECREF(pymaterials); PyDict_SetItemString(rtndict, "material_indices", pymaterial_indices); + Py_DECREF(pymaterial_indices); PyDict_SetItemString(rtndict, "attribs", attribobj); - + Py_DECREF(attribobj); return rtndict; } static PyMethodDef mMethods[] = { - {"LoadObj", pyLoadObj, METH_VARARGS}, {NULL, NULL, 0, NULL} - }; #if PY_MAJOR_VERSION >= 3 -static struct PyModuleDef moduledef = {PyModuleDef_HEAD_INIT, "tinyobjloader", - NULL, -1, mMethods}; +static struct PyModuleDef moduledef = {PyModuleDef_HEAD_INIT, "tinyobjloader", NULL, -1, mMethods}; PyMODINIT_FUNC PyInit_tinyobjloader(void) { return PyModule_Create(&moduledef);