C memory leak in libxml2?

I have what appears to be a large memory leak coming from libxml2. While I find this to be difficult to believe, it is what seems to be happening, and I wonder if anyone here has any ideas.

The system is a freebsd 12.0 system that is used in an embedded application. The program is multithreaded and written in C and is compiled using clang.

The sequence of events is this: Upon startup, the program will read in an XML file from disk using this command:
C:
document = xmlReadFile(filename, NULL, 0 );

Along the way, the program will download another config file across the internet. This new config file arrives encrypted.

The program reads this encrypted file into a buffer, then writes it to disk and frees the buffer. The copy on disk is then passed to gpg for decryption via a popen() call, and the stream out of gpg is captured in a memory buffer.

This memory buffer is also written to disk, then the buffer is freed.

The decrypted xml file on disk is then passed through "xmllint --format infile > outfile" using a system() call. There is no memory buffer in use here, at least, not that I define.

The program loads the newly decrypted and lint'ed xml file like this:
C:
new_document = xmlReadFile(filename, NULL, 0 );

So, now there are two XML documents loaded into memory by this program. The program, depending on circumstances, may choose to switch these documents by swapping pointers:
Code:
keepdoc = document;
        keeproot = root;
        document = new_document;
        root = new_root;
        new_document = keepdoc;
        new_root = keeproot;

It ordinarily does this only one time, on the first startup, but might be compelled to do so again at rare intervals (rare meaning months apart).

Usually, it won't swap XML files. In normal operation it will download this new XML file approximately every 4 minutes, and go through the decryption/lint/load process.

After loading the new XML file as new_document, the program will do some validation on it to ensure it is complete and correct. When the file passes validation, the program then will compare it against the existing XML that is loaded as document.

If any differences are found, the section that has the difference in it is moved from new_document to document (replacing the section that is in document) like this:
Code:
xmlNode *copy_new_node(int nargs, char *nodename, ...)
{
    xmlNode *oldnode, *newnode, *copynode, *siblingnode, *parentnode;
    va_list nodenames;
    if(!new_root || !root)
        return(0);
    pthread_mutex_lock(&xml_access_mutex);
    newnode = __find_xml_entry(new_root, nargs, nodename, nodenames);
    oldnode = __find_xml_entry(root, nargs, nodename, nodenames);
    va_end(nodenames);
    if(!newnode) {
        pthread_mutex_unlock(&xml_access_mutex);
        return 0x0;
    }
    siblingnode = oldnode->next;
    parentnode = oldnode->parent;
    xmlUnlinkNode(oldnode);
    xmlFreeNode(oldnode);
    xmlUnlinkNode(newnode);
    if(siblingnode) {
        xmlAddPrevSibling(siblingnode, newnode);
    } else {
        xmlAddChild(parentnode, newnode);
    }
    copynode = xmlDocCopyNode(newnode, document, 1);
    pthread_mutex_unlock(&xml_access_mutex);
    return(copynode);
}

Finally, the new document is cleared:
Code:
void clear_new_xml()
{
    if(new_document)
    {
        pthread_mutex_lock(&xml_access_mutex);
        xmlFreeDoc(new_document);
        pthread_mutex_unlock(&xml_access_mutex);
    }
    new_root = 0;
    new_document = 0;
}

When the new document is cleared, the memory that was allocated for it is not returned to the system. It continues to show with my program with the result that the program's footprint is growing and growing and growing.

I have considered the possibility that the way I am moving the node from document to document could be behind the problem. But I have the same problem whether or not there is a difference in the XML that causes a section to be moved. In other words, the common circumstance is that there is no change to the XML so nothing is moved.

But, also, nothing is released.

Does anyone see what I might be doing wrong, or what I might do to solve this problem?
 
If any differences are found, the section that has the difference in it is moved from new_document to document (replacing the section that is in document) like this:
This design seems superfluous to me. If you want 'document' to have the same DOM content as 'new_document', free the old copy and set the pointer to the new copy. In other words:

Code:
xmlFreeDoc(document);
document = xmlReadFile(filename, NULL, 0 );
root = ...

No need for extra new_document variables.

I'd argue that using a DOM for your in-memory config representation is a bad idea. Isolate the use of XML to deserializing your config. The rest of your code sees a config object without having to walk up and down a DOM tree to find things.

Going further ... a SAX or expat style parser is lighter and therefore more appropriate for an embedded app. My main complaint against the DOM is it isn't memory efficient for this sort of use.
 
Make sure new_document is NULL before calling xmlReadFile.
All variables are initialized before being used. I routinely do that.

This design seems superfluous to me. If you want 'document' to have the same DOM content as 'new_document', free the old copy and set the pointer to the new copy. In other words:

Code:
xmlFreeDoc(document);
document = xmlReadFile(filename, NULL, 0 );
root = ...
But I don't necessarily want the two documents to have the same contents. In fact, a common case is that they will not have the same contents, for particular reasons.

What is required is that certain specific sections of the documents have the same contents, when certain conditions are met. Hence, transferring the section from one document (either by changing pointers or by copying) is the way to go.

I'd argue that using a DOM for your in-memory config representation is a bad idea. Isolate the use of XML to deserializing your config. The rest of your code sees a config object without having to walk up and down a DOM tree to find things.

Going further ... a SAX or expat style parser is lighter and therefore more appropriate for an embedded app. My main complaint against the DOM is it isn't memory efficient for this sort of use.
This system has plenty of memory; I'm not worried about it. As long as it doesn't leak by the bucket, that is. I have to have the config available in a structured format anyway, and the XML representation is convenient because it has to get loaded up by the parser anyway. I looked at SAX and decided it wouldn't be a good choice for my purpose.

What is required here is for libxml2 to release the memory that was allocated. It would appear that this is not happening, and I have no idea why not.
 
All variables are initialized before being used. I routinely do that.
That covers the first time the variable is used. What of all the other times after?

Code:
xmlDoc* new_document = NULL; // init
... other code ...
new_document = xmlReadFile(filename, NULL, 0 ); // safe
... other code ...
new_document = xmlReadFile(filename, NULL, 0 ); // leak
 
That covers the first time the variable is used. What of all the other times after?

Code:
xmlDoc* new_document = NULL; // init
... other code ...
new_document = xmlReadFile(filename, NULL, 0 ); // safe
... other code ...
new_document = xmlReadFile(filename, NULL, 0 ); // leak

That case would indeed be a leak. But the document is freed by clear_new_xml, which then resets the new_document pointer to 0, so that should be OK.

The code that loads new_document starts like this:
Code:
bool new_xml_init()
{
    return(_xml_init(&new_document, &new_root));
}

bool _xml_init(xmlDoc **usedoc, xmlNode **useroot)
{
    if(*usedoc) {
        return(false);
    }
...
and then there is a call to

*usedoc = xmlReadFile(filename, NULL, 0 );

after some appropriate setup stuff is done. So, the invocation will be aborted if new_document is non zero, which would mean a document is already loaded.

The pointers document, new_document, root, and new_root are static globals within the file that contains all the xml parsing routines, thus are not exposed outside that file.

Just noticed a race condition in clear_new_xml.

Where? Other than the mutex calls, the only call is to xmlFreeDoc() and the function won't proceed until that call returns. It is a void function, so it does not return a value.
 
The problem is the if-statement outside the lock (and how the lock is used in general). Two threads reach this point at more or less the same time. Both try to own the lock, one wins. The second thread then owns the lock and does the same thing. This is a "use after free" bug. The call to xmlFreeDoc may tolerate that but what's worse is the first thread may have (re)assigned the pointer to the new document - which the second thread promptly frees. The first thread is unaware and continues to use the document - again a "use after free" bug. The time window is very small but it very much does exist.

If at all possible - make the "new_document" variable local. If it is only needed for a short period of time (eg. load and extract specific elements / attributes), limit its lifetime to just that.

To summarize - the lock should not be around xmlDocFree. It should protect access to the pointer.

  • own the lock
  • use the document pointer
  • re-assign the pointer as needed
  • release the lock

Even if a thread is only reading the document (eg. DOM traversal) - it needs to be protected from another thread's DOM changes or a complete document replacement. This implies new_document and main document pointers both need a lock - which can be the same lock for simplicity.
 
Back
Top