[libvirt] [libvirt-python][PATCH] generator: Free strings after libvirt_charPtrWrap

[Daniel P. Berrange]

On Fri, Sep 12, 2014 at 10:57:26AM +0200, Michal Privoznik wrote:
> Up till bb3301ba the wrapper was freeing the passed strings for us.
> However that changed after the commit. So now we don't free any
> strings which results in memory leaks as reported upstream [1]:
> 
> ==14265== 2,407 bytes in 1 blocks are definitely lost in loss record 1,457 of 1,550
> ==14265==    at 0x4C2845D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==14265==    by 0x5C46624: xdr_string (in /usr/lib64/libc-2.17.so)
> ==14265==    by 0xCFD9FCD: xdr_remote_nonnull_string (remote_protocol.c:31)
> ==14265==    by 0xCFDC2C8: xdr_remote_domain_get_xml_desc_ret (remote_protocol.c:1617)
> ==14265==    by 0xCFF0811: virNetMessageDecodePayload (virnetmessage.c:407)
> ==14265==    by 0xCFE68FB: virNetClientProgramCall (virnetclientprogram.c:379)
> ==14265==    by 0xCFBE8B1: callFull.isra.2 (remote_driver.c:6578)
> ==14265==    by 0xCFC7F04: remoteDomainGetXMLDesc (remote_driver.c:6600)
> ==14265==    by 0xCF8167C: virDomainGetXMLDesc (libvirt.c:4380)
> ==14265==    by 0xCC2C4DF: libvirt_virDomainGetXMLDesc (libvirt.c:1141)
> ==14265==    by 0x4F12B93: PyEval_EvalFrameEx (in /usr/lib64/libpython2.7.so.1.0)
> ==14265==    by 0x4F141AC: PyEval_EvalCodeEx (in /usr/lib64/libpython2.7.so.1.0)
> 
> The python documentation clearly advise us to call free() [2]. From
> an example in their docs:
> 
>     PyObject *res;
>     char *buf = (char *) malloc(BUFSIZ); /* for I/O */
> 
>     if (buf == NULL)
>         return PyErr_NoMemory();
>     ...Do some I/O operation involving buf...
>     res = PyString_FromString(buf);
>     free(buf); /* malloc'ed */
>     return res;
> 
> Moreover, instead of using VIR_FREE() (which we are not exporting),
> I'll just go with bare free().
> 
> 1: https://www.redhat.com/archives/libvir-list/2014-September/msg00736.html
> 2: https://docs.python.org/2/c-api/memory.html
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
> 
> While the fix seems okay, for some reason I'm seeing another valgrind warning
> after the fix is applied:
> 
> ==21247== Invalid read of size 4
> ==21247==    at 0x4EBFB93: PyObject_Free (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4EB613E: insertdict_by_entry (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4EB7BAF: dict_set_item_by_hash_or_entry (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4EBC4CB: _PyModule_Clear (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F2916A: PyImport_Cleanup (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F34B0D: Py_Finalize (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F460A5: Py_Main (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x54462BF: (below main) (in /lib64/libc-2.19.so)
> ==21247==  Address 0x69be020 is 2,016 bytes inside a block of size 4,390 free'd
> ==21247==    at 0x4C2B5AC: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==21247==    by 0x7F1EC2D: libvirt_virDomainGetXMLDesc (in /home/zippy/work/libvirt/libvirt-python.git/build/lib.linux-x86_64-2.7/libvirtmod.so)
> ==21247==    by 0x4F180BB: PyEval_EvalFrameEx (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F199FF: PyEval_EvalCodeEx (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F18AB4: PyEval_EvalFrameEx (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F199FF: PyEval_EvalCodeEx (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F19AF8: PyEval_EvalCode (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F32EBE: run_mod (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F340E1: PyRun_FileExFlags (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F352E6: PyRun_SimpleFileExFlags (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x4F4670E: Py_Main (in /usr/lib64/libpython2.7.so.1.0)
> ==21247==    by 0x54462BF: (below main) (in /lib64/libc-2.19.so)
> 
> This happens when using our examples/dominfo.py.
> 
> 
>  generator.py | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/generator.py b/generator.py
> index a798274..8ee046a 100755
> --- a/generator.py
> +++ b/generator.py
> @@ -708,12 +708,16 @@ def print_function_wrapper(module, name, output, export, include):
>          else:
>              c_call = "\n    c_retval = %s(%s);\n" % (name, c_call)
>          ret_convert = "    py_retval = libvirt_%sWrap((%s) c_retval);\n" % (n,c)
> +        if n == "charPtr":
> +            ret_convert = ret_convert + "    free(c_retval);\n"
>          ret_convert = ret_convert + "    return py_retval;\n"
>      elif ret[0] in py_return_types:
>          (f, t, n, c) = py_return_types[ret[0]]
>          c_return = "    %s c_retval;\n" % (ret[0])
>          c_call = "\n    c_retval = %s(%s);\n" % (name, c_call)
>          ret_convert = "    py_retval = libvirt_%sWrap((%s) c_retval);\n" % (n,c)
> +        if n == "charPtr":
> +            ret_convert = ret_convert + "    free(c_retval);\n"
>          ret_convert = ret_convert + "    return py_retval;\n"
>      else:
>          if ret[0] in skipped_types:

ACK.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|