[lldb] fix memory leak in "GetGDBServerRegisterInfoXMLAndProcess"

[yuanzi]

Summary

While running heap checker on a test that uses LLDB API, the following memory leak is found:
RAW: HeapChecker started...
RAW: Leak check _main_ detected leaks of 34 bytes in 4 objects
RAW: The 2 largest leaks:
RAW: Leak of 17 bytes in 2 objects allocated from:
@ 0x7fb93bd20166 NewHook()
@ 0x7fb929372a73 absl::base_internal::MallocHook::InvokeNewHookSlow()
@ 0x5600d1046093 libc_malloc
@ 0x7fb974529c03 xmlStrdup
@ 0x7fb9744c2a0b xmlGetProp
@ 0x7fb9749d9ed6 lldb_private::XMLNode::GetAttributeValue()
@ 0x7fb979043001 std::u::function::policy_invoker<>::__call_impl<>()
@ 0x7fb9749da06d lldb_private::XMLNode::ForEachChildElement()
@ 0x7fb97903c54d lldb_private::process_gdb_remote::ProcessGDBRemote::GetGDBServerRegisterInfoXMLAndProcess()
@ 0x7fb97902cfe4 lldb_private::process_gdb_remote::ProcessGDBRemote::GetGDBServerRegisterInfo()
@ 0x7fb97902c1d0 lldb_private::process_gdb_remote::ProcessGDBRemote::BuildDynamicRegisterInfo()
@ 0x7fb97902e92a lldb_private::process_gdb_remote::ProcessGDBRemote::SetThreadStopInfo()
@ 0x7fb97902db18 lldb_private::process_gdb_remote::ProcessGDBRemote::DoConnectRemote()
@ 0x7fb97584965e lldb_private::Process::ConnectRemote()
@ 0x7fb975839fa6 lldb_private::Platform::DoConnectProcess()
@ 0x7fb97583a39e lldb_private::Platform::ConnectProcessSynchronous()
@ 0x7fb97545b28b CommandObjectProcessConnect::DoExecute()
@ 0x7fb9755a70c9 lldb_private::CommandObjectParsed::Execute()
@ 0x7fb97559c0e9 lldb_private::CommandInterpreter::HandleCommand()
@ 0x7fb975460145 lldb_private::CommandObjectRegexCommand::DoExecute()
@ 0x7fb9755a72d2 lldb_private::CommandObjectRaw::Execute()
@ 0x7fb97559c0e9 lldb_private::CommandInterpreter::HandleCommand()
@ 0x7fb997a5f22e lldb::SBCommandInterpreter::HandleCommand()
@ 0x7fb997a5ef9b lldb::SBCommandInterpreter::HandleCommand()

This change fixes the memory leaks by freeing memory after it is no longer in use.
Tested with "ninja check-lldb".

[JDevlieghere]

We shouldn't have to manage memory at this granularity. Why isn't xmlFreeDoc cleaning this up?

[labath]

In D116707#3223998, @JDevlieghere wrote:

We shouldn't have to manage memory at this granularity. Why isn't xmlFreeDoc cleaning this up?

Because xmlGetProp returns a value that's independent of the containing document.

That said, I agree this is a pretty bad encapsulation breakage (and will not compile in !LLDB_ENABLE_LIBXML builds).

One way to fix this (if we wanted to be fancy) is to have this function return a std::unique_ptr<char> with a custom deleter (xmlFree), but given that this is hardly performance sensitive code, I would just copy the attribute value into a std::string inside GetAttributeValue, and then have it return *that* (after freeing the original string).

In either case, this function only has a handful of callers, so it should be pretty easy to update it to use a saner API.

[yuanzi]

Yeah since xmlGetProp calls xmlGetPropNodeValueInternal, which calls xmlStrdup rather than returning the content or value of the node directly, xmlFreeDoc could not clean it up.
https://github.com/tenderlove/libxml2/blob/master/tree.c

Agree that we need to update the API to return not just a reference to string (llvm::StringRef), but an object that can own string data (std::string).

This change fixes the memory leaks in GetGDBServerRegisterInfoXMLAndProcess by updating GetAttributeValue to return std::string instead of llvm::StringRef.
Tested with ninja check-lldb.

[labath]

Yes, that's the API I had in mind, but check out the inline comments for some problems/improvements.

lldb/source/Host/common/XML.cpp

141 ↗

(On Diff #398009)

this should be xmlFree, per the function documentation.

157–159 ↗

(On Diff #398009)

I don't see why we need these temporary variables. std::string is implicitly convertible to a StringRef, and you don't need the value afterwards, so you should be able to pass the GetAttributeValue straight into the to_integer function. Did that not work for some reason?

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4536

Same here. .empty() and to_integer should work on std::string as well. Constructing a StringRef might make sense if we needed to perform some more complex operations (ones which std::string does not support) but I don't see anything like that here.

[yuanzi]

free->xmlFree, remove conversions from std::string to llvm::StringRef.

[yuanzi]

lldb/source/Host/common/XML.cpp

157–159 ↗

(On Diff #398009)

I did that mainly to ensure the new code behaves exactly the same as the old. Since no complex functions are called, they should work interchangeably! Reverted to previous version.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

4536

Right! Done.

[JDevlieghere]

Thanks! LGTM

[labath]

Cool, thanks. Do you need someone to commit this for you?

[yuanzi]

Thanks for the reviews!

Yeah I do not have commit access. It would be great if someone could help with the commit!

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (target_node) {
                                           const XMLNode &node) -> bool {
         llvm::StringRef name = node.GetName();
         if (name == "architecture") {
           node.GetElementText(target_info.arch);
         } else if (name == "osabi") {
           node.GetElementText(target_info.osabi);
         } else if (name == "xi:include" || name == "include") {
           llvm::StringRef href = node.GetAttributeValue("href");
-          if (!href.empty())
+          if (!href.empty()) {
             target_info.includes.push_back(href.str());
+            xmlFree(const_cast<char *>(href.data()));
+          }
         } else if (name == "feature") {
           feature_nodes.push_back(node);
         } else if (name == "groups") {
           node.ForEachChildElementWithName(
               "group", [&target_info](const XMLNode &node) -> bool {
                 uint32_t set_id = UINT32_MAX;
                 RegisterSetInfo set_info;
 
if (can_use_svr4 && comm.GetQXferLibrariesSVR4ReadSupported()) {
     XMLNode root_element = doc.GetRootElement("library-list-svr4");
     if (!root_element)
       return llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           "Error finding library-list-svr4 xml element");
 
     // main link map structure
     llvm::StringRef main_lm = root_element.GetAttributeValue("main-lm");
     // FIXME: we're silently ignoring invalid data here
     if (!main_lm.empty())
       llvm::to_integer(main_lm, list.m_link_map);
 
     root_element.ForEachChildElementWithName(
         "library", [log, &list](const XMLNode &library) -> bool {
           LoadedModuleInfoList::LoadedModuleInfo module;
 
           // FIXME: we're silently ignoring invalid data here