[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

this should be xmlFree, per the function documentation.

157–159

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

4534

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

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

4534

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/include/lldb/Host/XML.h

public:
   bool NameIs(const char *name) const;
 
   XMLNode GetParent() const;
 
   XMLNode GetSibling() const;
 
   XMLNode GetChild() const;
 
-  llvm::StringRef GetAttributeValue(const char *name,
-                                    const char *fail_value = nullptr) const;
+  std::string GetAttributeValue(const char *name,
 
   bool GetAttributeValueAsUnsigned(const char *name, uint64_t &value,
                                    uint64_t fail_value = 0, int base = 0) const;
 
   XMLNode FindFirstChildElementWithName(const char *name) const;
 
   XMLNode GetElementForPath(const NamePath &path);
 

lldb/source/Host/common/XML.cpp

if (IsValid())
     return XMLNode(m_node->children);
   else
     return XMLNode();
 #else
   return XMLNode();
 #endif
 }
 
-llvm::StringRef XMLNode::GetAttributeValue(const char *name,
-                                           const char *fail_value) const {
-  const char *attr_value = nullptr;
+std::string XMLNode::GetAttributeValue(const char *name,
+  std::string attr_value;
 #if LLDB_ENABLE_LIBXML2
-
-  if (IsValid())
-    attr_value = (const char *)xmlGetProp(m_node, (const xmlChar *)name);
-  else
+  if (IsValid()) {
+    xmlChar *value = xmlGetProp(m_node, (const xmlChar *)name);
+    if (value) {
+      attr_value = (const char *)value;
+      free(value);
+    }
+  } else {
+    if (fail_value)
       attr_value = fail_value;
+  }
 #else
+  if (fail_value)
     attr_value = fail_value;
 #endif
-  if (attr_value)
-    return llvm::StringRef(attr_value);
-  else
-    return llvm::StringRef();
+  return attr_value;
 }
 
 bool XMLNode::GetAttributeValueAsUnsigned(const char *name, uint64_t &value,
                                           uint64_t fail_value, int base) const {
   value = fail_value;
-  return llvm::to_integer(GetAttributeValue(name, ""), value, base);
+  std::string attr_str = GetAttributeValue(name, "");
+  llvm::StringRef attr(attr_str);
+  return llvm::to_integer(attr, value, base);
 }
 
 void XMLNode::ForEachChildNode(NodeCallback const &callback) const {
 #if LLDB_ENABLE_LIBXML2
   if (IsValid())
     GetChild().ForEachSiblingNode(callback);
 #endif
 }

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

if (target_node) {
       target_node.ForEachChildElement([&target_info, &feature_nodes](
                                           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");
+          std::string href = node.GetAttributeValue("href");
           if (!href.empty())
-            target_info.includes.push_back(href.str());
+            target_info.includes.push_back(href);
         } 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 (target_node) {
       // <feature> element.
       XMLNode feature_node = xml_document.GetRootElement("feature");
       if (feature_node) {
         feature_nodes.push_back(feature_node);
         feature_node.ForEachChildElement([&target_info](
                                         const XMLNode &node) -> bool {
           llvm::StringRef name = node.GetName();
           if (name == "xi:include" || name == "include") {
-            llvm::StringRef href = node.GetAttributeValue("href");
+            std::string href = node.GetAttributeValue("href");
             if (!href.empty())
-              target_info.includes.push_back(href.str());
+              target_info.includes.push_back(href);
             }
             return true;
           });
       }
     }
 
     // gdbserver does not implement the LLDB packets used to determine host
     // or process architecture.  If that is the case, attempt to use
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");
+    std::string main_lm_str = root_element.GetAttributeValue("main-lm");
+    llvm::StringRef main_lm(main_lm_str);
     // 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;
 
if (!root_element)
       return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                      "Error finding library-list xml element");
 
     // FIXME: we're silently ignoring invalid data here
     root_element.ForEachChildElementWithName(
         "library", [log, &list](const XMLNode &library) -> bool {
           LoadedModuleInfoList::LoadedModuleInfo module;
 
-          llvm::StringRef name = library.GetAttributeValue("name");
-          module.set_name(name.str());
+          std::string name = library.GetAttributeValue("name");
+          module.set_name(name);
 
           // The base address of a given library will be the address of its
           // first section. Most remotes send only one section for Windows
           // targets for example.
           const XMLNode &section =
               library.FindFirstChildElementWithName("section");
-          llvm::StringRef address = section.GetAttributeValue("address");
+          std::string address_str = section.GetAttributeValue("address");
+          llvm::StringRef address(address_str);
           uint64_t address_value = LLDB_INVALID_ADDRESS;
           llvm::to_integer(address, address_value);
           module.set_base(address_value);
           // These addresses are absolute values.
           module.set_base_is_offset(false);
 
           if (log) {
             std::string name;