llvm-mt: Fix memory management in WindowsManifestMergerImpl::getMergedManifest

[vitalybuka]

Summary

xmlDoc needs to be released with xmlFreeDoc.
XML_PARSE_NODICT is needed for safe moving nodes between documents.
Buffer returned from xmlDocDumpFormatMemoryEnc needs xmlFree, but it needs
outlive users of getMergedManifest results.

[vitalybuka]

clang-format

[rnk]

Thanks for the fix!

[vitalybuka]

Redo

[vitalybuka]

Please take another look.

[vitalybuka]

lib/WindowsManifest/WindowsManifestMerger.cpp

680 ↗

(On Diff #113562)

Another option is getMemBufferCopy and xmlNodeCopy(CombinedRoot)
More copying but then we will have less state and support of merge() calls getMergedManifest()

[zturner]

XmlDeleter class looks nice. lgtm

[ecbeckmann]

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.

Also out of curiosity how did you discover the presence of memory problems? I've looked on msan and nothing seems to be there.

[vitalybuka]

In 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

[ecbeckmann]

In D37321#859285, @vitalybuka wrote:

In 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.

[vitalybuka]

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.

So I'll submit the patch?
I'll try to enable this on our asan bot to detect problems in future.

[ecbeckmann]

In D37321#859384, @vitalybuka wrote:

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.

So I'll submit the patch?
I'll try to enable this on our asan bot to detect problems in future.

Sorry for late response. Sounds good.

[vitalybuka]

I'll try to enable this on our asan bot to detect problems in future.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast
"check-llvm asan" and "check-clang asan" now run this code

llvm/trunk/lib/WindowsManifest/WindowsManifestMerger.cpp

public:
   std::unique_ptr<MemoryBuffer> getMergedManifest();
 
 private:
   static void errorCallback(void *Ctx, const char *Format, ...);
   Error getParseError();
 #if LLVM_LIBXML2_ENABLED
   xmlDocPtr CombinedDoc = nullptr;
   std::vector<xmlDocPtr> MergedDocs;
+
+  bool Merged = false;
+  struct XmlDeleter {
+    void operator()(xmlChar *Ptr) { xmlFree(Ptr); }
+    void operator()(xmlDoc *Ptr) { xmlFreeDoc(Ptr); }
+  };
+  int BufferSize = 0;
+  std::unique_ptr<xmlChar, XmlDeleter> Buffer;
 #endif
   bool ParseErrorOccurred = false;
 };
 
 #if LLVM_LIBXML2_ENABLED
 
 static const std::pair<StringRef, StringRef> MtNsHrefsPrefixes[] = {
     {"urn:schemas-microsoft-com:asm.v1", "ms_asmv1"},

 
 WindowsManifestMerger::WindowsManifestMergerImpl::~WindowsManifestMergerImpl() {
   for (auto &Doc : MergedDocs)
     xmlFreeDoc(Doc);
 }
 
 Error WindowsManifestMerger::WindowsManifestMergerImpl::merge(
     const MemoryBuffer &Manifest) {
+  if (Merged)
+    return make_error<WindowsManifestError>(
+        "merge after getMergedManifest is not supported");
   if (Manifest.getBufferSize() == 0)
     return make_error<WindowsManifestError>(
         "attempted to merge empty manifest");
   xmlSetGenericErrorFunc((void *)this,
                          WindowsManifestMergerImpl::errorCallback);
-  xmlDocPtr ManifestXML =
-      xmlReadMemory(Manifest.getBufferStart(), Manifest.getBufferSize(),
-                    "manifest.xml", nullptr, XML_PARSE_NOBLANKS);
+  xmlDocPtr ManifestXML = xmlReadMemory(
+      Manifest.getBufferStart(), Manifest.getBufferSize(), "manifest.xml",
+      nullptr, XML_PARSE_NOBLANKS | XML_PARSE_NODICT);
   xmlSetGenericErrorFunc(nullptr, nullptr);
   if (auto E = getParseError())
     return E;
   xmlNodePtr AdditionalRoot = xmlDocGetRootElement(ManifestXML);
   stripComments(AdditionalRoot);
   setAttributeNamespaces(AdditionalRoot);
   if (CombinedDoc == nullptr) {
     CombinedDoc = ManifestXML;
if (CombinedDoc == nullptr) {
     }
   }
   MergedDocs.push_back(ManifestXML);
   return Error::success();
 }
 
 std::unique_ptr<MemoryBuffer>
 WindowsManifestMerger::WindowsManifestMergerImpl::getMergedManifest() {
-  unsigned char *XmlBuff;
-  int BufferSize = 0;
-  if (CombinedDoc) {
+  if (!Merged) {
+    Merged = true;
+
+    if (!CombinedDoc)
+      return nullptr;
+
     xmlNodePtr CombinedRoot = xmlDocGetRootElement(CombinedDoc);
     std::vector<xmlNsPtr> RequiredPrefixes;
     checkAndStripPrefixes(CombinedRoot, RequiredPrefixes);
-    std::unique_ptr<xmlDoc> OutputDoc(xmlNewDoc((const unsigned char *)"1.0"));
+    std::unique_ptr<xmlDoc, XmlDeleter> OutputDoc(
+        xmlNewDoc((const unsigned char *)"1.0"));
     xmlDocSetRootElement(OutputDoc.get(), CombinedRoot);
+    assert(0 == xmlDocGetRootElement(CombinedDoc));
+
     xmlKeepBlanksDefault(0);
-    xmlDocDumpFormatMemoryEnc(OutputDoc.get(), &XmlBuff, &BufferSize, "UTF-8",
-                              1);
+    xmlChar *Buff = nullptr;
+    xmlDocDumpFormatMemoryEnc(OutputDoc.get(), &Buff, &BufferSize, "UTF-8", 1);
+    Buffer.reset(Buff);
   }
-  if (BufferSize == 0)
-    return nullptr;
-  return MemoryBuffer::getMemBuffer(
-      StringRef(FROM_XML_CHAR(XmlBuff), (size_t)BufferSize));
+
+  return BufferSize ? MemoryBuffer::getMemBuffer(StringRef(
+                          FROM_XML_CHAR(Buffer.get()), (size_t)BufferSize))
+                    : nullptr;
 }
 
 #else
 
 WindowsManifestMerger::WindowsManifestMergerImpl::~WindowsManifestMergerImpl() {
 }
 
 Error WindowsManifestMerger::WindowsManifestMergerImpl::merge(