[PATCH] D37321: llvm-mt: Fix memory management in WindowsManifestMergerImpl::getMergedManifest

Fri Sep 1 15:05:26 PDT 2017

ecbeckmann added a comment.

In https://reviews.llvm.org/D37321#859285, @vitalybuka wrote:

> In https://reviews.llvm.org/D37321#859258, @ecbeckmann wrote:
>
> > Sorry for being so late to respond to this, I've been busy with team match this week.
> >
> > Thanks for catching these errors.  It was my fault for just using unique_ptr instead of explicitly calling xmlFree on the xmlDoc structures.  However, I'm confused about the XML_PARSE_NODICT flag, how will this help?  As far as I can tell it prevents the creation of a new string dictionary for that xmlDoc that is parsed?  Which could be a problem if I had href's and namespaces in one tree point to another.  However I never do this and always duplicate the entire string from one tree to another.
>
>
> As soon as I start to delete OutputDoc correctly xmlFreeDoc(Doc) in ~WindowsManifestMergerImpl() fails. I guess I see this both with or without asan. XML_PARSE_NODICT helps.
>
> > Also out of curiosity how did you discover the presence of memory problems?  I've looked on msan and nothing seems to be there.
>
> check-llvm under msan disables libxml (and other 3rd party deps) as msan needs them instrumented as well.
>  I am using check-llvm with asan

Okay, this makes me think there are deeper memory issues with my implementation.  Technically I should be able to just delete each incoming manifest xml tree after it's merged, instead of keeping it around.  The reason I do currently is because there could still be element, attribute, href, and namespace strings in the incoming doc that I still need to print the final doc.  But if if I make a deep copy of the necessary strings, I should just be able to free each doc right after it is merged.  This seems better than using XML_PARSE_NODICT, because we also address the issue of any other memory I might have failed to copy....I could do this in a subsequent patch.

https://reviews.llvm.org/D37321