tatyana-krasnukha updated this revision to Diff 187153. tatyana-krasnukha retitled this revision from "Check pointer results on nullptr before using them" to "[lldb-mi] Check raw pointers before passing them to std::string ctor/assignment". tatyana-krasnukha added a comment. Herald added subscribers: jfb, mgorny.
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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55653/new/ https://reviews.llvm.org/D55653 Files: tools/lldb-mi/MICmdCmdMiscellanous.cpp tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp tools/lldb-mi/MICmnLLDBDebugger.cpp tools/lldb-mi/MICmnMIOutOfBandRecord.cpp tools/lldb-mi/MICmnMIResultRecord.cpp tools/lldb-mi/MIDriverMgr.cpp tools/lldb-mi/MIUtilString.cpp tools/lldb-mi/MIUtilString.h unittests/tools/CMakeLists.txt unittests/tools/lldb-mi/CMakeLists.txt unittests/tools/lldb-mi/utils/CMakeLists.txt unittests/tools/lldb-mi/utils/StringTest.cpp
Index: unittests/tools/lldb-mi/utils/StringTest.cpp =================================================================== --- unittests/tools/lldb-mi/utils/StringTest.cpp +++ unittests/tools/lldb-mi/utils/StringTest.cpp @@ -0,0 +1,32 @@ +//===-- LLGSTest.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)); +} + Index: unittests/tools/lldb-mi/utils/CMakeLists.txt =================================================================== --- unittests/tools/lldb-mi/utils/CMakeLists.txt +++ unittests/tools/lldb-mi/utils/CMakeLists.txt @@ -0,0 +1,12 @@ +add_library(lldb-mi-utils OBJECT + ${LLDB_PROJECT_ROOT}/tools/lldb-mi/MIUtilString.cpp + ) + +add_lldb_unittest(LLDBMiUtilTests + StringTest.cpp + + LINK_COMPONENTS + Support + ) + +target_sources(LLDBMiUtilTests PRIVATE $<TARGET_OBJECTS:lldb-mi-utils>) Index: unittests/tools/lldb-mi/CMakeLists.txt =================================================================== --- unittests/tools/lldb-mi/CMakeLists.txt +++ unittests/tools/lldb-mi/CMakeLists.txt @@ -0,0 +1,2 @@ +include_directories(${LLDB_PROJECT_ROOT}/tools/lldb-mi) +add_subdirectory(utils) Index: unittests/tools/CMakeLists.txt =================================================================== --- unittests/tools/CMakeLists.txt +++ unittests/tools/CMakeLists.txt @@ -1,3 +1,4 @@ +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 Index: tools/lldb-mi/MIUtilString.h =================================================================== --- tools/lldb-mi/MIUtilString.h +++ tools/lldb-mi/MIUtilString.h @@ -33,7 +33,13 @@ 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); Index: tools/lldb-mi/MIUtilString.cpp =================================================================== --- tools/lldb-mi/MIUtilString.cpp +++ tools/lldb-mi/MIUtilString.cpp @@ -37,7 +37,8 @@ // Return: None. // Throws: None. //-- -CMIUtilString::CMIUtilString(const char *vpData) : std::string(vpData) {} +CMIUtilString::CMIUtilString(const char *vpData) + : std::string(WithNullAsEmpty(vpData)) {} //++ //------------------------------------------------------------------------------------ @@ -58,7 +59,7 @@ // Throws: None. //-- CMIUtilString &CMIUtilString::operator=(const char *vpRhs) { - assign(vpRhs); + assign(WithNullAsEmpty(vpRhs)); return *this; } @@ -103,12 +104,10 @@ 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); @@ -127,8 +126,8 @@ 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); @@ -160,7 +159,8 @@ 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; @@ -457,7 +457,7 @@ // 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; Index: tools/lldb-mi/MIDriverMgr.cpp =================================================================== --- tools/lldb-mi/MIDriverMgr.cpp +++ tools/lldb-mi/MIDriverMgr.cpp @@ -493,7 +493,8 @@ 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); } Index: tools/lldb-mi/MICmnMIResultRecord.cpp =================================================================== --- tools/lldb-mi/MICmnMIResultRecord.cpp +++ tools/lldb-mi/MICmnMIResultRecord.cpp @@ -53,7 +53,8 @@ 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)); } //++ Index: tools/lldb-mi/MICmnMIOutOfBandRecord.cpp =================================================================== --- tools/lldb-mi/MICmnMIOutOfBandRecord.cpp +++ tools/lldb-mi/MICmnMIOutOfBandRecord.cpp @@ -109,8 +109,10 @@ //-- 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)); } //++ Index: tools/lldb-mi/MICmnLLDBDebugger.cpp =================================================================== --- tools/lldb-mi/MICmnLLDBDebugger.cpp +++ tools/lldb-mi/MICmnLLDBDebugger.cpp @@ -43,10 +43,11 @@ 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; Index: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp =================================================================== --- tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp +++ tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp @@ -375,11 +375,9 @@ // 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) Index: tools/lldb-mi/MICmdCmdMiscellanous.cpp =================================================================== --- tools/lldb-mi/MICmdCmdMiscellanous.cpp +++ tools/lldb-mi/MICmdCmdMiscellanous.cpp @@ -339,7 +339,9 @@ 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);
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
