[lldb-mi] Check raw pointers before passing them to std::string ctor/assignment

[tatyana-krasnukha]

Summary

Prevent crashing like llvm.org/pr37054

[tatyana-krasnukha]

Fixed misspelling

[shafik]

Looks like a great fix!

MIDataTypes.h

67 ↗

(On Diff #186892)

Can we use llvm::Optional instead? Then we can use getValueOr().

71 ↗

(On Diff #186892)

If we can not use llvm::Optional then why not just take T *?

[tatyana-krasnukha]

Thanks for the review!

MIDataTypes.h

67 ↗

(On Diff #186892)

Unfortunately no, with Optional<T*> getValueOr doesn't have the constraint that alternative value is not nullptr.
In case of Optional<T> getValueOr returns T by value which is not what we need.

71 ↗

(On Diff #186892)

It is easier to read:

const char *pFileName =
    GetDereferenceable(fileSpec.GetFilename()).Or(*pUnknown);

vs

const char *pFileName = fileSpec.GetFilename();
pFileName = (pFileName != nullptr) ? pFileName : pUnknown;

isn't it?

[JDevlieghere]

In the future please include context with the diff (-U9999 should do the trick), it makes reviewing a lot easier.

Have you considered wrapping raw strings in llvm's StringRefs? It looks like lldb-mi is already using some ADT classes from llvm.
Unfortunately we cannot use those in the SB interface, but they're a lot safer. They have a .str() which does the right thing and returns an empty string when the raw string is a nullptr. Personally I would very much prefer that over the This.or(That) pattern, but if you really wanted that you could have an Optional<StringRef> and do getValueOr() as Shafik suggested.

[tatyana-krasnukha]

Thank you for mentioning StringRef, you gave me the idea to keep pointers check inside the CMIUtilString to obviate undefined behavior. This is the best place to do it, however, a caller still should examine pointers he passes to CMIUtilString::Format as the ellipsis parameter.

[sgraenitz]

Good initiative to fix potential nullptr assignments to std::string.

In D55653#1400513, @tatyana-krasnukha wrote:

a caller still should examine pointers he passes to CMIUtilString::Format as the ellipsis parameter.

So Format() and Printf() don't handle nullptr arguments already?

In D55653#1398660, @JDevlieghere wrote:

Have you considered wrapping raw strings in llvm's StringRefs?

+1 in general (but it may be a bigger effort)

unittests/tools/lldb-mi/CMakeLists.txt

1 ↗

(On Diff #187153)

LLDB_PROJECT_ROOT is rarely used, maybe prefer LLDB_SOURCE_DIR?

unittests/tools/lldb-mi/utils/CMakeLists.txt

3 ↗

(On Diff #187153)

Nice workaround for not having a library target at hand for linking!

unittests/tools/lldb-mi/utils/StringTest.cpp

1 ↗

(On Diff #187153)

update name

[tatyana-krasnukha]

Thanks for the review!

So Format() and Printf() don't handle nullptr arguments already?

They just forward arguments to vsprintf/vsnprintf. Otherwise, the would have to do twice the work parsing format string.

+1 in general (but it may be a bigger effort)

Since lldb-mi has its own wrapper for strings, we would have a conversion const char* -> StringRef -> CMIUtilString everywhere. This looks redundant to me and it is easy to forget converting a result of SB class's function to the StringRef.

[tatyana-krasnukha]

LLDB_PROJECT_ROOT -> LLDB_SOURCE_DIR

[abidh]

The lldb-mi bits look ok to me.

tools/lldb-mi/MIUtilString.h

36 ↗

(On Diff #187613)

May be spurious new line

[tatyana-krasnukha]

Removed new lines from MIUtilString.h

[hintonda]

lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt

12

Just wanted to let you know that using $<TARGET_OBJECTS:...> in anything other than add_library and add_executable wasn't added until cmake 3.9, so I'm unable to configure lldb using the officially supported cmake 3.4.3 version.

Btw, I use cmake 3.4.3 locally so I don't unintentionally use new unsupported features to the clang/llvm cmake files.

[tatyana-krasnukha]

lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt

12

Thank you for pointing to this issue. I had cmake 3.6.2 version installed and, oddly enough, it worked well.
I'd appreciate your review of the patch that fixes the issue.

[hintonda]

lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt

12

I don't have all the various version installed, so I was trying to figure out which version by looking at the online docs. If 3.6.2 works, then it changed somewhere between 3.4.3 and 3.6.2, so I guess you can just use >= 3.6.2 for your test.

Thanks again for looking into this.

lldb/trunk/tools/lldb-mi/MICmdCmdMiscellanous.cpp

if (rSessionInfo.GetProcess().IsValid()) {
       miTuple.Add(miValueResult3);
     }
 
     if (rSessionInfo.GetTarget().IsValid()) {
       lldb::SBTarget sbTrgt = rSessionInfo.GetTarget();
       const char *pDir = sbTrgt.GetExecutable().GetDirectory();
       const char *pFileName = sbTrgt.GetExecutable().GetFilename();
       const CMIUtilString strFile(
-          CMIUtilString::Format("%s/%s", pDir, pFileName));
+          CMIUtilString::Format("%s/%s",
+                                CMIUtilString::WithNullAsEmpty(pDir),
+                                CMIUtilString::WithNullAsEmpty(pFileName)));
       const CMICmnMIValueConst miValueConst4(strFile);
       const CMICmnMIValueResult miValueResult4("executable", miValueConst4);
       miTuple.Add(miValueResult4);
     }
 
     const CMICmnMIValueList miValueList(miTuple);
     const CMICmnMIValueResult miValueResult5("groups", miValueList);
     const CMICmnMIResultRecord miRecordResult(

lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp

bool CMICmnLLDBDebugSessionInfo::MIResponseFormThreadInfo(
   // Add "id"
   const CMIUtilString strId(CMIUtilString::Format("%d", rThread.GetIndexID()));
   const CMICmnMIValueConst miValueConst1(strId);
   const CMICmnMIValueResult miValueResult1("id", miValueConst1);
   vwrMIValueTuple.Add(miValueResult1);
 
   // Add "target-id"
   const char *pThreadName = rThread.GetName();
-  const MIuint len =
-      (pThreadName != nullptr) ? CMIUtilString(pThreadName).length() : 0;
-  const bool bHaveName = ((pThreadName != nullptr) && (len > 0) && (len < 32) &&
-                          CMIUtilString::IsAllValidAlphaAndNumeric(
-                              pThreadName)); // 32 is arbitrary number
+  const MIuint len = CMIUtilString(pThreadName).length();
+  const bool bHaveName = (len > 0) && (len < 32) && // 32 is arbitrary number
+                         CMIUtilString::IsAllValidAlphaAndNumeric(pThreadName);
   const char *pThrdFmt = bHaveName ? "%s" : "Thread %d";
   CMIUtilString strThread;
   if (bHaveName)
     strThread = CMIUtilString::Format(pThrdFmt, pThreadName);
   else
     strThread = CMIUtilString::Format(pThrdFmt, rThread.GetIndexID());
   const CMICmnMIValueConst miValueConst2(strThread);
   const CMICmnMIValueResult miValueResult2("target-id", miValueConst2);

lldb/trunk/tools/lldb-mi/MICmnLLDBDebugger.cpp

if (!value.IsValid())
     return false;
 
   lldb::SBType value_type = value.GetType();
   if (!value_type.IsValid())
     return false;
 
   lldb::BasicType type_code = value_type.GetBasicType();
   if (type_code == lldb::eBasicTypeSignedChar)
-    stream.Printf("%d %s", (int)value.GetValueAsSigned(), value.GetValue());
+    stream.Printf("%d %s", (int)value.GetValueAsSigned(),
+                  CMIUtilString::WithNullAsEmpty(value.GetValue()));
   else if (type_code == lldb::eBasicTypeUnsignedChar)
     stream.Printf("%u %s", (unsigned)value.GetValueAsUnsigned(),
-                  value.GetValue());
+                  CMIUtilString::WithNullAsEmpty(value.GetValue()));
   else
     return false;
 
   return true;
 }
 
 //++
 //------------------------------------------------------------------------------------

lldb/trunk/tools/lldb-mi/MICmnMIOutOfBandRecord.cpp

 // the first
 //          (additional) result i.e. async-record ==>  "*" type.
 // Args:    veType      - (R) A MI Out-of-Band enumeration.
 // Return:  CMIUtilString - The async record text.
 // Throws:  None.
 //--
 static CMIUtilString
 BuildAsyncRecord(CMICmnMIOutOfBandRecord::OutOfBand_e veType) {
-  return CMIUtilString::Format("%s%s", MapOutOfBandToToken(veType),
-                               MapOutOfBandToText(veType));
+  auto Token = MapOutOfBandToToken(veType);
+  auto Text = MapOutOfBandToText(veType);
+  return CMIUtilString::Format("%s%s", CMIUtilString::WithNullAsEmpty(Token),
+                               CMIUtilString::WithNullAsEmpty(Text));
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: CMICmnMIOutOfBandRecord constructor.
 // Type:    Method.
 // Args:    None.
 // Return:  None.

lldb/trunk/tools/lldb-mi/MICmnMIResultRecord.cpp

 //          veType      - (R) A MI result class enumeration.
 // Return:  CMIUtilString & - MI result record mandatory data
 // Throws:  None.
 //--
 static const CMIUtilString
 BuildResultRecord(const CMIUtilString &vrToken,
                   CMICmnMIResultRecord::ResultClass_e veType) {
   const char *pStrResultRecord = MapResultClassToResultClassText(veType);
-  return CMIUtilString::Format("%s^%s", vrToken.c_str(), pStrResultRecord);
+  return CMIUtilString::Format("%s^%s", vrToken.c_str(),
+                               CMIUtilString::WithNullAsEmpty(pStrResultRecord));
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: CMICmnMIResultRecord constructor.
 // Type:    Method.
 // Args:    None.
 // Return:  None.

lldb/trunk/tools/lldb-mi/MIDriverMgr.cpp

bool CMIDriverMgr::ParseArgs(const int argc, const char *argv[],
   // Print application arguments to the Log file
   const bool bHaveArgs(argc >= 2);
   CMIUtilString strArgs(MIRSRC(IDS_MI_APP_ARGS));
   if (!bHaveArgs) {
     strArgs += MIRSRC(IDS_WORD_NONE);
     bOk = bOk && m_pLog->Write(strArgs, CMICmnLog::eLogVerbosity_Log);
   } else {
     for (MIint i = 1; i < argc; i++) {
-      strArgs += CMIUtilString::Format("%d:'%s' ", i, argv[i]);
+      strArgs += CMIUtilString::Format("%d:'%s' ", i,
+                                       CMIUtilString::WithNullAsEmpty(argv[i]));
     }
     bOk = bOk && m_pLog->Write(strArgs, CMICmnLog::eLogVerbosity_Log);
   }
 
   // Look for the command line options
   bool bHaveArgInterpret = false;
   bool bHaveArgVersion = false;
   bool bHaveArgVersionLong = false;

lldb/trunk/tools/lldb-mi/MIUtilString.h

public:
 
   // Static method:
 public:
   static CMIUtilString Format(const char *vFormating, ...);
   static CMIUtilString FormatBinary(const MIuint64 vnDecimal);
   static CMIUtilString FormatValist(const CMIUtilString &vrFormating,
                                     va_list vArgs);
   static bool IsAllValidAlphaAndNumeric(const char *vpText);
+  static const char *WithNullAsEmpty(const char *vpText) {
+    return vpText ? vpText : "";
+  }
   static bool Compare(const CMIUtilString &vrLhs, const CMIUtilString &vrRhs);
   static CMIUtilString ConvertToPrintableASCII(const char vChar,
                                                bool bEscapeQuotes = false);
   static CMIUtilString ConvertToPrintableASCII(const char16_t vChar16,
                                                bool bEscapeQuotes = false);
   static CMIUtilString ConvertToPrintableASCII(const char32_t vChar32,
                                                bool bEscapeQuotes = false);
 

lldb/trunk/tools/lldb-mi/MIUtilString.cpp

 //++
 //------------------------------------------------------------------------------------
 // Details: CMIUtilString constructor.
 // Type:    Method.
 // Args:    vpData  - Pointer to UTF8 text data.
 // Return:  None.
 // Throws:  None.
 //--
-CMIUtilString::CMIUtilString(const char *vpData) : std::string(vpData) {}
+CMIUtilString::CMIUtilString(const char *vpData)
+  : std::string(WithNullAsEmpty(vpData)) {}
 
 //++
 //------------------------------------------------------------------------------------
 // Details: CMIUtilString constructor.
 // Type:    Method.
 // Args:    vpStr  - Text data.
 // Return:  None.
 // Throws:  None.
 //--
 CMIUtilString::CMIUtilString(const std::string &vrStr) : std::string(vrStr) {}
 
 //++
 //------------------------------------------------------------------------------------
 // Details: CMIUtilString assignment operator.
 // Type:    Method.
 // Args:    vpRhs   - Pointer to UTF8 text data.
 // Return:  CMIUtilString & - *this string.
 // Throws:  None.
 //--
 CMIUtilString &CMIUtilString::operator=(const char *vpRhs) {
-  assign(vpRhs);
+  assign(WithNullAsEmpty(vpRhs));
   return *this;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: CMIUtilString assignment operator.
 // Type:    Method.
 // Args:    vrRhs   - The other string to copy from.

 //--
 CMIUtilString CMIUtilString::FormatPriv(const CMIUtilString &vrFormat,
                                         va_list vArgs) {
   CMIUtilString strResult;
   MIint nFinal = 0;
   MIint n = vrFormat.size();
 
   // IOR: mysterious crash in this function on some windows builds not able to
-  // duplicate
-  // but found article which may be related. Crash occurs in vsnprintf() or
-  // va_copy()
+  // duplicate but found article which may be related. Crash occurs in
+  // vsnprintf() or va_copy().
   // Duplicate vArgs va_list argument pointer to ensure that it can be safely
-  // used in
-  // a new frame
+  // used in a new frame.
   // http://julipedia.meroh.net/2011/09/using-vacopy-to-safely-pass-ap.html
   va_list argsDup;
   va_copy(argsDup, vArgs);
 
   // Create a copy va_list to reset when we spin
   va_list argsCpy;
   va_copy(argsCpy, argsDup);
 
   if (n == 0)
     return strResult;
 
   n = n << 4; // Reserve 16 times as much the length of the vrFormat
 
   std::unique_ptr<char[]> pFormatted;
   while (1) {
     pFormatted.reset(new char[n + 1]); // +1 for safety margin
     ::strncpy(&pFormatted[0], vrFormat.c_str(), n);
 
-    //  We need to restore the variable argument list pointer to the start again
-    //  before running vsnprintf() more then once
+    // We need to restore the variable argument list pointer to the start again
+    // before running vsnprintf() more then once
     va_copy(argsDup, argsCpy);
 
     nFinal = ::vsnprintf(&pFormatted[0], n, vrFormat.c_str(), argsDup);
     if ((nFinal < 0) || (nFinal >= n))
       n += abs(nFinal - n + 1);
     else
       break;
   }

 // Args:    vFormat       - (R) Format string data instruction.
 //          ...           - (R) Var list args of any type.
 // Return:  CMIUtilString - Number of splits found in the string data.
 // Throws:  None.
 //--
 CMIUtilString CMIUtilString::Format(const char *vFormating, ...) {
   va_list args;
   va_start(args, vFormating);
-  CMIUtilString strResult = CMIUtilString::FormatPriv(vFormating, args);
+  CMIUtilString strResult =
+      CMIUtilString::FormatPriv(WithNullAsEmpty(vFormating), args);
   va_end(args);
 
   return strResult;
 }
 
 //++
 //------------------------------------------------------------------------------------
 // Details: Perform a snprintf format style on a string data. A new string

 // can be
 //          either upper or lower case.
 // Type:    Static method.
 // Args:    vpText  - (R) The text data to examine.
 // Return:  bool - True = yes all alpha, false = one or more chars is non alpha.
 // Throws:  None.
 //--
 bool CMIUtilString::IsAllValidAlphaAndNumeric(const char *vpText) {
-  const size_t len = ::strlen(vpText);
+  const size_t len = ::strlen(WithNullAsEmpty(vpText));
   if (len == 0)
     return false;
 
   for (size_t i = 0; i < len; i++, vpText++) {
     const char c = *vpText;
     if (::isalnum((int)c) == 0)
       return false;
   }

lldb/trunk/unittests/tools/CMakeLists.txt

+add_subdirectory(lldb-mi)
 if(CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|Linux|NetBSD")
   if ((CMAKE_SYSTEM_NAME MATCHES "Darwin" AND SKIP_TEST_DEBUGSERVER) OR (NOT CMAKE_SYSTEM_NAME MATCHES "Darwin" AND SKIP_LLDB_SERVER_BUILD))
     # These tests are meant to test lldb-server/debugserver in isolation, and
     # don't provide any value if run against a server copied from somewhere.
   else()
     add_subdirectory(lldb-server)
   endif()
 endif()

lldb/trunk/unittests/tools/lldb-mi/CMakeLists.txt

+include_directories(${LLDB_SOURCE_DIR}/tools/lldb-mi)
+add_subdirectory(utils)

lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt

+add_library(lldb-mi-utils OBJECT
+  ${LLDB_SOURCE_DIR}/tools/lldb-mi/MIUtilString.cpp
+  )
+
+add_lldb_unittest(LLDBMiUtilTests
+  StringTest.cpp
+
+  LINK_COMPONENTS
+    Support
+  )
+
+target_sources(LLDBMiUtilTests PRIVATE $<TARGET_OBJECTS:lldb-mi-utils>)

lldb/trunk/unittests/tools/lldb-mi/utils/StringTest.cpp

+//===-- StringTest.cpp ------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MIUtilString.h"
+#include "gtest/gtest.h"
+
+TEST(StringTest, ConstructFromNullptr) {
+  auto str = CMIUtilString(nullptr);
+  EXPECT_TRUE(str.empty());
+  EXPECT_NE(nullptr, str.c_str());
+  EXPECT_EQ(CMIUtilString(""), str);
+}
+
+TEST(StringTest, AssignNullptr) {
+  CMIUtilString str;
+  str = nullptr;
+  EXPECT_TRUE(str.empty());
+  EXPECT_NE(nullptr, str.c_str());
+  EXPECT_EQ(CMIUtilString(""), str);
+}
+
+TEST(StringTest, IsAllValidAlphaAndNumeric) {
+  EXPECT_TRUE(CMIUtilString::IsAllValidAlphaAndNumeric("123abc"));
+  EXPECT_FALSE(CMIUtilString::IsAllValidAlphaAndNumeric(""));
+  EXPECT_FALSE(CMIUtilString::IsAllValidAlphaAndNumeric(nullptr));
+}
+