[Trivial] Fix a memory leak in _msi.c #4127
Conversation
| @@ -283,6 +283,7 @@ msiobj_dealloc(msiobj* msidb) | |||
| { | |||
| MsiCloseHandle(msidb->h); | |||
| msidb->h = 0; | |||
| PyObject_Del(msidb); | |||
There was a problem hiding this comment.
I'm not sure that calling PyObject_Del() in dealloc is correct. Maybe you should call Py_TYPE(op)->tp_free(op); instead.
There was a problem hiding this comment.
From the documentation of tp_dealloc() (https://docs.python.org/3.7/c-api/typeobj.html#c.PyTypeObject.tp_dealloc):
If the type is not subtypable (doesn’t have the Py_TPFLAGS_BASETYPE flag bit set), it is permissible to call the object deallocator directly instead of via tp_free.
As none of the four types in _msi.c set a tp_free() function, it is correct to call PyObject_Del() here (this is done by many other types).
There was a problem hiding this comment.
Oh ok. I checked and you are right: range_dealloc() also calls PyObject_Del() for example.
I forgot that PyObject_Del() is simply an alias to PyObject_Free(), a memory deallocator.
| @@ -283,6 +283,7 @@ msiobj_dealloc(msiobj* msidb) | |||
| { | |||
| MsiCloseHandle(msidb->h); | |||
| msidb->h = 0; | |||
| PyObject_Del(msidb); | |||
There was a problem hiding this comment.
Oh ok. I checked and you are right: range_dealloc() also calls PyObject_Del() for example.
I forgot that PyObject_Del() is simply an alias to PyObject_Free(), a memory deallocator.
|
Thanks @ZackerySpytz for the PR, and @Haypo for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7. |
|
Thanks @ZackerySpytz for the PR, and @Haypo for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
GH-4308 is a backport of this pull request to the 2.7 branch. |
(cherry picked from commit cb04f75)
(cherry picked from commit cb04f75)
|
GH-4309 is a backport of this pull request to the 3.6 branch. |
|
I merged your fix @ZackerySpytz, thank you for your contribution to Python! I backported the fix to Python 2.7 and 3.6. |
If test_msilib had any non-trivial tests, this memory leak would cause test_msilib to fail when run with the
-Roption.