[ldns-users] contrib/python: memory leak in ldns_pkt.new_query

[Karel Slany]

Hello,

the patch in the attachment solves the memory leak and the owner problem.

Someone at NLnet Labs, please, apply the patch onto the ldns svn.


Using '%newobject ldns_pkt_query_new;' solved only one half of the
problem. Dname has to be cloned before being used in new_query because
ldns_pkt_query_new() uses the rr_name pointer directly instead of
creating a copy. Double free is the result as the same rr_name object is
being destroyed once in dname and second in pkt.

The patch creates a new wrapper function that automatically clones the
dname rr_name before calling ldns_pkt_query_new().

Best regards,
K.

Am 07.08.2013 11:26, schrieb Karel Slany:
> Hello Johannes,
> 
> I'll have look at it.
> 
> K.
> 
> Am 06.08.2013 19:59, schrieb Johannes Naab:
>> Hi,
>>
>> The python wrapper for ldns appears to leak memory when packets are
>> created by ldns_pkt.new_query.
>>
>> The following is a short example (tested in ldns-1.6.16, Python 2.7.5
>> Linux 3.10, amd64):
>> #!/usr/bin/env python2.7
>> import ldns
>>
>> dname = ldns.ldns_dname("test.nic.cz")
>>
>> def leak():
>>     pkt = ldns.ldns_pkt.new_query(dname, ldns.LDNS_RR_TYPE_A,
>> ldns.LDNS_RR_CLASS_IN, 0)
>>
>> while True:
>>     leak()
>>
>>
>> Root of the problem seems to be, that the pointer to the ldns_pkt struct
>> is not considered owned by the swig wrapper, and thus the pkt struct is
>> not freed upon garbage collection in python.
>>
>> import ldns
>> dname = ldns.ldns_dname("test.nic.cz")
>> pkt = ldns.ldns_pkt.new_query(dname, ldns.LDNS_RR_TYPE_A,
>>                               ldns.LDNS_RR_CLASS_IN, 0)
>> print pkt.thisown
>>
>> => False.
>>
>> If new_query is replaced by new_query_frm_str the packet is "owned" by
>> the swig wrapper, and the packet does not leak.
>>
>>
>> pkt = ldns.ldns_pkt.new_query_frm_str(str(dname), ldns.LDNS_RR_TYPE_A,
>>                                       ldns.LDNS_RR_CLASS_IN, 0)
>> print pkt.thisown
>>
>> => True
>>
>> I tried to convince swig that the struct should be owned by the wrapper
>> by inserting a "%newobject ldns_pkt_query_new;" below line 44 in
>> contrib/python/ldns_packet.i. This made swig own the pointer, but
>> freeing the packet on garbage collection made python crash (memory
>> corruption, double free). My knowledge and understanding of swig is limited.
>>
>>
>> Johannes
>> _______________________________________________
>> ldns-users mailing list
>> ldns-users at open.nlnetlabs.nl
>> http://open.nlnetlabs.nl/mailman/listinfo/ldns-users
>>
> 
> _______________________________________________
> ldns-users mailing list
> ldns-users at open.nlnetlabs.nl
> http://open.nlnetlabs.nl/mailman/listinfo/ldns-users
> 

-------------- next part --------------
Index: trunk/contrib/python/ldns_packet.i
===================================================================
--- trunk/contrib/python/ldns_packet.i	(revision 3860)
+++ trunk/contrib/python/ldns_packet.i	(working copy)
@@ -109,6 +109,18 @@
 
 /* clone data when pushed in */
 
+%newobject _ldns_pkt_query_new;
+%rename (__ldns_pkt_query_new) ldns_pkt_query_new;
+%inline
+%{
+  ldns_pkt * _ldns_pkt_query_new(ldns_rdf *rr_name, ldns_rr_type rr_type,
+      ldns_rr_class rr_class, uint16_t flags)
+  {
+    return ldns_pkt_query_new(ldns_rdf_clone(rr_name), rr_type, rr_class,
+             flags);
+  }
+%}
+
 %rename(__ldns_pkt_push_rr) ldns_pkt_push_rr;
 %inline %{
 bool _ldns_pkt_push_rr(ldns_pkt* p, ldns_pkt_section sec, ldns_rr *rr) {
@@ -170,7 +182,7 @@
                :param flags: packet flags
                :returns: new ldns_pkt object
             """
-            return _ldns.ldns_pkt_query_new(rr_name, rr_type, rr_class, flags)
+            return _ldns._ldns_pkt_query_new(rr_name, rr_type, rr_class, flags)
 
         @staticmethod
         def new_query_frm_str(rr_name, rr_type, rr_class, flags, raiseException = True):