[Lldb-commits] [PATCH] D116707: [lldb] fix memory leak in "GetGDBServerRegisterInfoXMLAndProcess"

2022-01-05 Thread Lirong Yuan via Phabricator via lldb-commits 
yuanzi created this revision.
yuanzi added a reviewer: LLDB.
yuanzi requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116707

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


Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4352,8 +4352,10 @@
   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(href.data()));
+  }
 } else if (name == "feature") {
   feature_nodes.push_back(node);
 } else if (name == "groups") {


Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4352,8 +4352,10 @@
   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(href.data()));
+  }
 } else if (name == "feature") {
   feature_nodes.push_back(node);
 } else if (name == "groups") {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D116772: [lldb] fix memory leak in "GetGDBServerRegisterInfoXMLAndProcess"

2022-01-06 Thread Lirong Yuan via Phabricator via lldb-commits 
yuanzi created this revision.
yuanzi requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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`:


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116772

Files:
  lldb/include/lldb/Host/XML.h
  lldb/source/Host/common/XML.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4351,9 +4351,9 @@
 } 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") {
@@ -4392,9 +4392,9 @@
 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;
   });
@@ -4530,7 +4530,8 @@
   "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);
@@ -4618,15 +4619,16 @@
 "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 §ion =
   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 = LL

[Lldb-commits] [PATCH] D116707: [lldb] fix memory leak in "GetGDBServerRegisterInfoXMLAndProcess"

2022-01-06 Thread Lirong Yuan via Phabricator via lldb-commits 
yuanzi updated this revision to Diff 398009.
yuanzi edited the summary of this revision.
yuanzi added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116707/new/

https://reviews.llvm.org/D116707

Files:
  lldb/include/lldb/Host/XML.h
  lldb/source/Host/common/XML.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4351,9 +4351,9 @@
 } 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") {
@@ -4392,9 +4392,9 @@
 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;
   });
@@ -4530,7 +4530,8 @@
   "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);
@@ -4618,15 +4619,16 @@
 "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 §ion =
   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);
Index: lldb/source/Host/common/XML.cpp
===
--- lldb/source/Host/common/XML.cpp
+++ lldb/source/Host/common/XML.cpp
@@ -130,28 +130,33 @@
 #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,
+   const char *fail_value) const {
+  std::string attr_value;
 #if LLDB_ENABLE_LIBXML2
-
-  if (IsValid())
-attr_value = (const char *)xmlGetProp(m_node, (const xmlChar *)name);
-  else
-attr_value = fail_value;
+  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
-  attr_value = fail_value;
+  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::GetAttributeValueAsUnsig

[Lldb-commits] [PATCH] D116707: [lldb] fix memory leak in "GetGDBServerRegisterInfoXMLAndProcess"

2022-01-07 Thread Lirong Yuan via Phabricator via lldb-commits 
yuanzi updated this revision to Diff 398193.
yuanzi added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116707/new/

https://reviews.llvm.org/D116707

Files:
  lldb/include/lldb/Host/XML.h
  lldb/source/Host/common/XML.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4351,9 +4351,9 @@
 } 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") {
@@ -4392,9 +4392,9 @@
 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;
   });
@@ -4530,7 +4530,7 @@
   "Error finding library-list-svr4 xml element");
 
 // main link map structure
-llvm::StringRef main_lm = root_element.GetAttributeValue("main-lm");
+std::string 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);
@@ -4618,15 +4618,15 @@
 "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 §ion =
   library.FindFirstChildElementWithName("section");
-  llvm::StringRef address = section.GetAttributeValue("address");
+  std::string address = section.GetAttributeValue("address");
   uint64_t address_value = LLDB_INVALID_ADDRESS;
   llvm::to_integer(address, address_value);
   module.set_base(address_value);
Index: lldb/source/Host/common/XML.cpp
===
--- lldb/source/Host/common/XML.cpp
+++ lldb/source/Host/common/XML.cpp
@@ -130,22 +130,25 @@
 #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,
+   const char *fail_value) const {
+  std::string attr_value;
 #if LLDB_ENABLE_LIBXML2
-
-  if (IsValid())
-attr_value = (const char *)xmlGetProp(m_node, (const xmlChar *)name);
-  else
-attr_value = fail_value;
+  if (IsValid()) {
+xmlChar *value = xmlGetProp(m_node, (const xmlChar *)name);
+if (value) {
+  attr_value = (const char *)value;
+  xmlFree(value);
+}
+  } else {
+if (fail_value)
+  attr_value = fail_value;
+  }
 #else
-  attr_value = fail_value;
+  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,
Index: lldb/include/lldb/Host/XML.h
===
--- lldb/include/lldb/Host/XML.h
+++ lldb/include/lldb/Host/XML.h
@@ -76,8 +76,8 @@
 
   XMLNode GetChild() const;
 
-  llvm::StringRef GetAttributeValue(const char *name,
-const char *fail_value = nullptr) const;
+  std::string GetAttributeValue(const char *name,
+const char *fail_value = nullptr) const;
 
   bool GetAttributeValueAsUnsigned(const char *name, uint64_t &value,
uint64_t fail_value = 0, int base = 0) const

[Lldb-commits] [PATCH] D116707: [lldb] fix memory leak in "GetGDBServerRegisterInfoXMLAndProcess"

2022-01-07 Thread Lirong Yuan via Phabricator via lldb-commits 
yuanzi marked 3 inline comments as done.
yuanzi added inline comments.



Comment at: lldb/source/Host/common/XML.cpp:157-159
+  std::string attr_str = GetAttributeValue(name, "");
+  llvm::StringRef attr(attr_str);
+  return llvm::to_integer(attr, value, base);

labath wrote:
> 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?
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.



Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4534
+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

labath wrote:
> 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.
Right! Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116707/new/

https://reviews.llvm.org/D116707

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D116707: [lldb] fix memory leak in "GetGDBServerRegisterInfoXMLAndProcess"

2022-01-10 Thread Lirong Yuan via Phabricator via lldb-commits 
yuanzi added a comment.

Thanks for the reviews!

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116707/new/

https://reviews.llvm.org/D116707

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits