[lldb][CPlusPlusLanguage] Avoid redundant const char* -> StringRef roundtrip

[Michael137]

We've been seen (very sporadic) lifetime issues around this area. Here's an example backtrace:

[  8] 0x0000000188e56743 libsystem_platform.dylib`_sigtramp + 55
[  9] 0x00000001181e041f LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] unsigned long std::1::constexpr_strlen[abi:nn200100]<char>(char const*) + 7 at constexpr_c_functions.h:63:10
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] std::__1::char_traits<char>::length[abi:nn200100](char const*) at char_traits.h:232:12
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] llvm::StringRef::StringRef(char const*) at StringRef.h:90:33
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] llvm::StringRef::StringRef(char const*) at StringRef.h:92:38
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const + 20 at CPlusPlusLanguage.cpp:68:62

Looks like we're calling strlen on a nullptr. I stared at this codepath for a while but am still not sure how that could happen unless the underlying ConstString somehow pointed to corrupted data.

But SymbolNameFitsToLanguage does some roundtripping through a const char* before calling GetManglingScheme. No other callsite does this and it just seems redundant.

This patch cleans this up.

rdar://161128180

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

We've been seen (very sporadic) lifetime issues around this area. Here's an example backtrace:

[  8] 0x0000000188e56743 libsystem_platform.dylib`_sigtramp + 55
[  9] 0x00000001181e041f LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] unsigned long std::1::constexpr_strlen[abi:nn200100]<char>(char const*) + 7 at constexpr_c_functions.h:63:10
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] std::__1::char_traits<char>::length[abi:nn200100](char const*) at char_traits.h:232:12
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] llvm::StringRef::StringRef(char const*) at StringRef.h:90:33
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const [inlined] llvm::StringRef::StringRef(char const*) at StringRef.h:92:38
[  9] 0x00000001181e0418 LLDB`lldb_private::CPlusPlusLanguage::SymbolNameFitsToLanguage(lldb_private::Mangled) const + 20 at CPlusPlusLanguage.cpp:68:62

Looks like we're calling strlen on a nullptr. I stared at this codepath for a while but am still not sure how that could happen unless the underlying ConstString somehow pointed to corrupted data.

But SymbolNameFitsToLanguage does some roundtripping through a const char* before calling GetManglingScheme. No other callsite does this and it just seems redundant.

This patch cleans this up.

rdar://161128180

---

Full diff: https://github.com/llvm/llvm-project/pull/161499.diff

1 Files Affected:

• (modified) lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp (+4-4)

diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
index 4e8a430af8c6c..a2199cb65cd35 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -104,10 +104,10 @@ CPlusPlusLanguage::GetFunctionNameInfo(ConstString name) const {
 }
 
 bool CPlusPlusLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
-  const char *mangled_name = mangled.GetMangledName().GetCString();
-  auto mangling_scheme = Mangled::GetManglingScheme(mangled_name);
-  return mangled_name && (mangling_scheme == Mangled::eManglingSchemeItanium ||
-                          mangling_scheme == Mangled::eManglingSchemeMSVC);
+  auto mangling_scheme =
+      Mangled::GetManglingScheme(mangled.GetMangledName().GetStringRef());
+  return mangling_scheme == Mangled::eManglingSchemeItanium ||
+         mangling_scheme == Mangled::eManglingSchemeMSVC;
 }
 
 ConstString CPlusPlusLanguage::GetDemangledFunctionNameWithoutArguments(

[DavidSpickett]

On the basis of saving a strlen, LGTM.

[DavidSpickett]

We've been seen (very sporadic) lifetime issues around this area.

Could the ConstString not be null terminated and sometimes the data after it happens to be interpretable as a string and sometimes it isn't?

Thinking about what the conclusion is if you don't see anymore crashes after this change. The explicit length in the StringRef might be preventing an over reading strlen.

[DavidSpickett]

Mind you, aren't ConstStrings encoded into some kind of table? This could be a single byte random write from something completely unrelated.

[Michael137]

We've been seen (very sporadic) lifetime issues around this area.

Could the ConstString not be null terminated and sometimes the data after it happens to be interpretable as a string and sometimes it isn't?

Yea that was my running theory too. Could also be a rare case of memory corruption somewhere? Has never been flagged on a sanitized bot as far as I'm aware though. Worth keeping an eye out