[Libguestfs] [PATCH v2] v2v: Free XML objects in the correct order.

[Pino Toscano]

In data giovedì 25 giugno 2015 21:35:41, Richard W.M. Jones ha scritto:
> If you free an xmlDocPtr before any xmlXPathObjectPtrs that reference
> the doc, you'll get valgrind errors like this:
> 
>   ==7390== Invalid read of size 4
>   ==7390==    at 0x4EB8BC6: xmlXPathFreeNodeSet (xpath.c:4185)
>   ==7390==    by 0x4EB8CC5: xmlXPathFreeObject (xpath.c:5492)
>   ==7390==    by 0x400A56: main (in /tmp/test)
>   ==7390==  Address 0x60c0928 is 8 bytes inside a block of size 120 free'd
>   ==7390==    at 0x4C29D2A: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>   ==7390==    by 0x4E8784F: xmlFreeNodeList (tree.c:3683)
>   ==7390==    by 0x4E87605: xmlFreeDoc (tree.c:1242)
>   ==7390==    by 0x400A4A: main (in /tmp/test)
> 
> The following simple test program demonstrates the problem:
> 
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <assert.h>
>   #include <libxml/xpath.h>
> 
>   int
>   main (int argc, char *argv[])
>   {
>     xmlDocPtr doc;
>     xmlXPathContextPtr xpathctx;
>     xmlXPathObjectPtr xpathobj;
> 
>     doc = xmlReadMemory ("<test/>", 7, NULL, NULL, XML_PARSE_NONET);
>     assert (doc);
>     xpathctx = xmlXPathNewContext (doc);
>     assert (xpathctx);
>     xpathobj = xmlXPathEvalExpression (BAD_CAST "/test", xpathctx);
>     assert (xpathobj);
>     xmlFreeDoc (doc);
>     xmlXPathFreeObject (xpathobj);
>     xmlXPathFreeContext (xpathctx);
>     exit (EXIT_SUCCESS);
>   }
> 
> In virt-v2v we were not freeing up objects in the correct order,
> because we didn't express the dependency between objects at the C
> level into the OCaml, where the OCaml garbage collector could see
> those dependencies.  For example code like:
> 
>   let doc = ... in
>   let xpathctx = xpath_new_context doc in
>   let xpathobj = xpath_eval_expression xpathctx "/foo" in
> 
> might end up freeing the 'doc' (xmlDocPtr) if, say, there were no
> further references to it in the code, even though the 'xpathobj'
> (xmlXPathObjectPtr) remains live.
> 
> To avoid this, we change the OCaml-level representation of objects
> like xpathobj so they contain a reference back to the higher-level
> objects (xpathctx & doc).  Therefore holding an xpathobj means that
> the doc cannot be freed.
> 
> However that alone is not quite sufficient.  There is a further
> problem when the program calls Gc.full_major, Gc.compact etc., or even
> just when xpathctx & doc happen to be freed at the same time.  The GC
> won't necessarily free them in the right order as it knows both need
> to be freed but doesn't know that one must be freed before the other.
> 
> To solve this we have to move the finalisers into OCaml code, since
> the OCaml Gc.finalise function comes with an explicit ordering
> guarantee (that finalisers are always called in reverse order that
> they were created), which the C-level finaliser does not.
> ---
> [...]

A bit convoluted, but seems needed if there's no way to do the same at
the C-interface level (maybe manually doing reference counting of
objects, but would seem even more convoluted...)

LGTM.

Thanks,
-- 
Pino Toscano