Page MenuHomePhabricator

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

Authored by vitalybuka on Aug 30 2017, 6:15 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

vitalybuka created this revision.

clang-format

rnk accepted this revision.Aug 30 2017, 7:26 PM

Thanks for the fix!

This revision is now accepted and ready to land.Aug 30 2017, 7:26 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.Aug 31 2017, 8:16 PM
This revision is now accepted and ready to land.Aug 31 2017, 8:16 PM
vitalybuka requested review of this revision.Sep 1 2017, 11:33 AM
vitalybuka retitled this revision from llvm-mt: Fix release of OutputDoc to llvm-mt: Fix memory management in WindowsManifestMergerImpl::getMergedManifest.
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka edited edge metadata.

Please take another look.

vitalybuka added inline comments.Sep 1 2017, 11:38 AM
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 accepted this revision.Sep 1 2017, 1:24 PM

XmlDeleter class looks nice. lgtm

This revision is now accepted and ready to land.Sep 1 2017, 1:24 PM
ecbeckmann edited edge metadata.EditedSep 1 2017, 2:10 PM

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 added a comment.EditedSep 1 2017, 2:26 PM

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

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.

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.

This revision was automatically updated to reflect the committed changes.

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.

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