mem leak while mxmlDelete

[michaelrsweet]

Version: 2.10 Original reporter: Franco yin

test code:

    pDoc = mxmlNewXML("1.0");
    pRoot = mxmlNewElement(pDoc, "root");
    pNode1 = mxmlNewElement(pRoot, "lev1");
    mxmlDelete(pDoc);

mxmlDelete won't free the node "root"

in function mxmlDelete, source code like this:

    if ((next = current->next) == NULL)
    {
      mxml_node_t *temp = current->parent;
                    /* Pointer to parent node */

      if (temp == node)
      {
       /*
        * Got back to the top node...
        */

        next = NULL;
      }
      else if ((next = temp->next) == NULL)
      {
        if ((next = temp->parent) == node)
          next = NULL;
      }
    }

I think it should be like this, notice "if ((next = temp) == node":

    if ((next = current->next) == NULL)
    {
      mxml_node_t *temp = current->parent;
                    /* Pointer to parent node */

      if (temp == node)
      {
       /*
        * Got back to the top node...
        */

        next = NULL;
      }
      else if ((next = temp->next) == NULL)
      {
        if ((next = temp) == node)
          next = NULL;
      }
    }

[michaelrsweet]

Original reporter: Franco yin

sorry? I got the wrong version.sorry

[michaelrsweet]

Original reporter: Thomas Kayne

Yin's report is indeed correct, mxmlDelete in v2.10 leaks memory. It won't deallocate any "in-between" generations in the XML family tree, i.e., pRoot resources won't get released in Yin's example.

Easiest fix appears to be the following little modification:

if ((next = current->next) == NULL)
{
  mxml_node_t *temp = current->parent;
                /* Pointer to parent node */

  if (temp == node)
  {
   /*
    * Got back to the top node...
    */

    next = NULL;
  }
  else
  {
      next = temp;
  }
}

[michaelrsweet]

Original reporter: CT

In my application I confirmed that the change suggested by Thomas Kayne avoids the memory leak. No idea if the patch is appropriate for all uses.

Below is the patch I generated for the change:

--- mxml-node.c.orig    2016-12-17 16:01:46.000000000 -0800
+++ mxml-node.c 2016-12-17 16:07:43.000000000 -0800
@@ -232,10 +232,9 @@

         next = NULL;
       }
-      else if ((next = temp->next) == NULL)
+      else
       {
-   if ((next = temp->parent) == node)
-     next = NULL;
+    next = temp;
       }
     }

[michaelrsweet]

Original reporter: Thomas

Thanks Thomas kayne! your code sovle my memory leak perfectly! and CT's code is unused! i have a test that CT's CODE still lead to memory leak! surgest this point must be changed to new version the sooner the better!

[bipbip59]

sorry for the duplicate and the recursion.

I use the last version (V2.11). I can not understand if this version included or not the patch?

/*
 * 'mxmlDelete()' - Delete a node and all of its children.
 *
 * If the specified node has a parent, this function first removes the
 * node from its parent using the mxmlRemove() function.
 */

void
mxmlDelete(mxml_node_t *node)       /* I - Node to delete */
{
  mxml_node_t   *current,       /* Current node */
        *next;          /* Next node */

#ifdef DEBUG
  fprintf(stderr, "mxmlDelete(node=%p)\n", node);
#endif /* DEBUG */

 /*
  * Range check input...
  */

  if (!node)
    return;

 /*
  * Remove the node from its parent, if any...
  */

  mxmlRemove(node);

 /*
  * Delete children...
  */

  for (current = node->child; current; current = next)
  {
   /*
    * Get the next node...
    */

    if ((next = current->child) != NULL)
    {
     /*
      * Free parent nodes after child nodes have been freed...
      */

      current->child = NULL;
      continue;
    }

    if ((next = current->next) == NULL)
    {
      mxml_node_t *temp = current->parent;
                    /* Pointer to parent node */

      if (temp == node)
      {
       /*
        * Got back to the top node...
        */

        next = NULL;
      }
      else if ((next = temp->next) == NULL)
      {
    if ((next = temp->parent) == node)
      next = NULL;
      }
    }

    mxml_free(current);
  }

 /*
  * Then free the memory used by this node...
  */

  mxml_free(node);
}

If so it does not work at home, if not can you get back the patch that works please? I could in this case validate it with my application. Tahnk's

[bipbip59]

Whatever your answer, I think I have found a solution without recursion.

• As long as a child node has itself a child, this "child child" is moved after the child node to be deleted.
• The nud child who has no child is free.
• We go to the next child node ...

void mxmlDelete(mxml_node_t *node)
{
   mxml_node_t  *current,*next;
   if (!node)  return;
  while ((current = node->child)!=NULL)
  {
      while ((next=current->child)!=NULL)
      {
         mxmlRemove(next);
         mxmlAdd(node,MXML_ADD_AFTER,current,next);
      }
      mxmlRemove(current);
      mxml_free(current);
  }
  mxmlRemove(node);
  mxml_free(node);
}

[michaelrsweet]

Fixed in git repo:

    if ((next = current->next) == NULL)
    {
     /*
      * Next node is the parent, which we'll free as needed...
      */

      if ((next = current->parent) == node)
        next = NULL;
    }