[lxml-dev] fix for another memory leak

In addition to the memory leak in the Element.get method, I have recently been seeing memory leaks in Element.Attrib.has_key. This results in the following trace from valgrind: ==12133== 8 bytes in 4 blocks are definitely lost in loss record 2 of 48 ==12133== at 0x401B422: malloc (vg_replace_malloc.c:149) ==12133== by 0x453D8B5: xmlStrndup (in /usr/lib/libxml2.so.2.6.23) ==12133== by 0x453D943: xmlStrdup (in /usr/lib/libxml2.so.2.6.23) ==12133== by 0x453E0D1: xmlStrcat (in /usr/lib/libxml2.so.2.6.23) ==12133== by 0x44E98A1: xmlNodeListGetString (in /usr/lib/libxml2.so.2.6.23) ==12133== by 0x44EF6D2: xmlGetNoNsProp (in /usr/lib/libxml2.so.2.6.23) ==12133== by 0x4462ED0: __pyx_f_5etree_7_Attrib_has_key (etree.c:5389) ==12133== by 0x80B6BE3: (within /usr/bin/python2.3) ==12133== by 0x80B8356: PyEval_EvalCodeEx (in /usr/bin/python2.3) ==12133== by 0x80B85D4: PyEval_EvalCode (in /usr/bin/python2.3) ==12133== by 0x80D8EFF: PyRun_InteractiveOneFlags (in /usr/bin/python2.3) ==12133== by 0x80D9018: PyRun_InteractiveLoopFlags (in /usr/bin/python2.3) The following patch appears to fix things for me. It is basically copied from the Element.get fix. -nld Index: src/lxml/etree.pyx =================================================================== --- src/lxml/etree.pyx (revision 23162) +++ src/lxml/etree.pyx (working copy) @@ -855,6 +855,7 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) + tree.xmlFree(result) return result is not NULL def __contains__(self, key):

Narayan Desai wrote:
Hi, sorry for the late reply. Thank you for supplying the patch. I applied a slightly different patch to the SVN trunk, also for the __contains__ method, to keep them from using pointers after calling free. Could you please check if the bug is gone? It would be really nice if you could also test the __contains__ method with Valgrind. Thanks again, Stefan Index: src/lxml/etree.pyx =================================================================== --- src/lxml/etree.pyx (Revision 23034) +++ src/lxml/etree.pyx (Arbeitskopie) @@ -855,7 +855,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False def __contains__(self, key): cdef xmlNs* c_ns @@ -865,7 +869,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False cdef _Attrib _attribFactory(_Document doc, xmlNode* c_node): cdef _Attrib result

Your patch fixes those two problems. I found one more in __getitem__. With the following patch, my code runs clean. (insofar as python can valgrind clean.) -nld Index: src/lxml/etree.pyx =================================================================== --- src/lxml/etree.pyx (revision 23162) +++ src/lxml/etree.pyx (working copy) @@ -775,16 +775,18 @@ def __getitem__(self, key): cdef xmlNs* c_ns - cdef char* result + cdef char* cresult ns, tag = _getNsTag(key) if ns is None: - result = tree.xmlGetNoNsProp(self._c_node, tag) + cresult = tree.xmlGetNoNsProp(self._c_node, tag) else: - result = tree.xmlGetNsProp(self._c_node, tag, ns) - if result is NULL: + cresult = tree.xmlGetNsProp(self._c_node, tag, ns) + if cresult is NULL: # XXX free namespace that is not in use..? raise KeyError, key - return funicode(result) + result = funicode(cresult) + tree.xmlFree(cresult) + return result def __len__(self): cdef int c @@ -855,7 +857,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False def __contains__(self, key): cdef xmlNs* c_ns @@ -865,7 +871,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False cdef _Attrib _attribFactory(_Document doc, xmlNode* c_node): cdef _Attrib result

Narayan Desai wrote:
Hi, sorry for the late reply. Thank you for supplying the patch. I applied a slightly different patch to the SVN trunk, also for the __contains__ method, to keep them from using pointers after calling free. Could you please check if the bug is gone? It would be really nice if you could also test the __contains__ method with Valgrind. Thanks again, Stefan Index: src/lxml/etree.pyx =================================================================== --- src/lxml/etree.pyx (Revision 23034) +++ src/lxml/etree.pyx (Arbeitskopie) @@ -855,7 +855,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False def __contains__(self, key): cdef xmlNs* c_ns @@ -865,7 +869,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False cdef _Attrib _attribFactory(_Document doc, xmlNode* c_node): cdef _Attrib result

Your patch fixes those two problems. I found one more in __getitem__. With the following patch, my code runs clean. (insofar as python can valgrind clean.) -nld Index: src/lxml/etree.pyx =================================================================== --- src/lxml/etree.pyx (revision 23162) +++ src/lxml/etree.pyx (working copy) @@ -775,16 +775,18 @@ def __getitem__(self, key): cdef xmlNs* c_ns - cdef char* result + cdef char* cresult ns, tag = _getNsTag(key) if ns is None: - result = tree.xmlGetNoNsProp(self._c_node, tag) + cresult = tree.xmlGetNoNsProp(self._c_node, tag) else: - result = tree.xmlGetNsProp(self._c_node, tag, ns) - if result is NULL: + cresult = tree.xmlGetNsProp(self._c_node, tag, ns) + if cresult is NULL: # XXX free namespace that is not in use..? raise KeyError, key - return funicode(result) + result = funicode(cresult) + tree.xmlFree(cresult) + return result def __len__(self): cdef int c @@ -855,7 +857,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False def __contains__(self, key): cdef xmlNs* c_ns @@ -865,7 +871,11 @@ result = tree.xmlGetNoNsProp(self._c_node, tag) else: result = tree.xmlGetNsProp(self._c_node, tag, ns) - return result is not NULL + if result is not NULL: + tree.xmlFree(result) + return True + else: + return False cdef _Attrib _attribFactory(_Document doc, xmlNode* c_node): cdef _Attrib result
participants (2)
-
Narayan Desai
-
Stefan Behnel