Re: [xml] Memleak in xmlNewReference

[Daniel Veillard <veillard redhat com>]

On Thu, Jan 24, 2008 at 10:24:50PM +0530, Ashwin wrote:
>    Hi,
>
>     For  the  attached  file  if I do Dom parsing, or use Reader Api's to
>    parse  with purify, a memory leak is shown for memory allocated at the
>    following location in function
>
>    xmlNewReference(tree.c)
>
>    cur = (xmlNodePtr) xmlMalloc(sizeof(xmlNode));

  Damn, right, good spotting.
You seems to have a really large test collection, are you using something
public or a home grown one ? In any case if feasible maybe this should be
added to the officiel regression tests. I used to run against the W3C
set but it's a long time I have not done this anymore.
BTW I assume when you send XML tests like that I can incorporate them
in the suite, if not, please tell e.g. if they are coming from an external
source, thanks !

>    The  cur  node  is  being  allocated  for  the entities present in the
>    document.  The  entities  themselves  are  defined in an external DTD.
>    However  the path given for the DTD is incorrect, so the entity is not
>    resolved.  xmlAddChild  is  called to add this node (cur) to the tree.
>    However  in  this  function  there  is a check to see if the parent is
>    NULL, which happens to be the case. So it returns NULL without freeing
>    the node (cur), hence the leak.
>
>    I think if we free the node inside the null check for parent it should
>    prevent  the  leak,  however  I  am not sure whether this is the right
>    place  to  fix  the problem. I have attached the patch which fixes the
>    problem.

  That doesn't seems the right approach, by freeing cur, there you
change the semantic of the API, that doesn't look good to me. What should
be done instead is check the return value of xmlAddChild() and if NULL
then free the node in the caller. I think that call is done in SAX2.c
at the end of xmlSAX2Reference() . I made that change and the leak is indeed
fixed, I will commit this to SVN later.
  Ideally all xmlAddChild() call in the library should be checked in that
way, to avoid such problems. Maybe you could look at this at least in the
most critical parts like the SAX2 and Include modules.

 thanks for the report,

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/