Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -31,6 +31,7 @@ #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" +#include "NotNullTerminatedResultCheck.h" #include "ParentVirtualCallCheck.h" #include "PosixReturnCheck.h" #include "SizeofContainerCheck.h" @@ -106,6 +107,8 @@ "bugprone-multiple-statement-macro"); CheckFactories.registerCheck( "bugprone-narrowing-conversions"); + CheckFactories.registerCheck( + "bugprone-not-null-terminated-result"); CheckFactories.registerCheck( "bugprone-parent-virtual-call"); CheckFactories.registerCheck( Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -23,6 +23,7 @@ MisplacedWideningCastCheck.cpp MoveForwardingReferenceCheck.cpp MultipleStatementMacroCheck.cpp + NotNullTerminatedResultCheck.cpp ParentVirtualCallCheck.cpp PosixReturnCheck.cpp SizeofContainerCheck.cpp Index: clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.h @@ -0,0 +1,67 @@ +//===--- NotNullTerminatedResultCheck.h - clang-tidy ------------*- 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOT_NULL_TERMINATED_RESULT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOT_NULL_TERMINATED_RESULT_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds function calls where it is possible to cause a not null-terminated +/// result. Usually the proper length of a string is 'strlen(src) + 1' or +/// equal length of this expression, because the null terminator needs an extra +/// space. Without the null terminator it can result in undefined behaviour +/// when the string is read. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-not-null-terminated-result.html +class NotNullTerminatedResultCheck : public ClangTidyCheck { +public: + NotNullTerminatedResultCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; + +private: + // If non-zero it is specifying if the target environment is considered to + // implement '_s' suffixed memory and string handler functions which are safer + // than older version (e.g. 'memcpy_s()'). The default value is '1'. + const int WantToUseSafeFunctions; + + bool UseSafeFunctions = false; + + void memoryHandlerFunctionFix( + StringRef Name, const ast_matchers::MatchFinder::MatchResult &Result); + void memcpyFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag); + void memcpy_sFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag); + void memchrFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result); + void memmoveFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag); + void strerror_sFix(const ast_matchers::MatchFinder::MatchResult &Result); + void ncmpFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result); + void xfrmFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result); +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NOT_NULL_TERMINATED_RESULT_H Index: clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp @@ -0,0 +1,1006 @@ +//===--- NotNullTerminatedResultCheck.cpp - clang-tidy ----------*- 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 "NotNullTerminatedResultCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "clang/Lex/PPCallbacks.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +constexpr llvm::StringLiteral FunctionExprName = "FunctionExpr"; +constexpr llvm::StringLiteral CastExprName = "CastExpr"; +constexpr llvm::StringLiteral UnknownDestName = "UnknownDest"; +constexpr llvm::StringLiteral DestArrayTyName = "DestArrayTy"; +constexpr llvm::StringLiteral DestVarDeclName = "DestVarDecl"; +constexpr llvm::StringLiteral DestMallocExprName = "DestMalloc"; +constexpr llvm::StringLiteral DestExprName = "DestExpr"; +constexpr llvm::StringLiteral SrcVarDeclName = "SrcVarDecl"; +constexpr llvm::StringLiteral SrcExprName = "SrcExpr"; +constexpr llvm::StringLiteral LengthExprName = "LengthExpr"; +constexpr llvm::StringLiteral WrongLengthExprName = "WrongLength"; +constexpr llvm::StringLiteral UnknownLengthName = "UnknownLength"; + +enum class LengthHandleKind { Increase, Decrease }; + +namespace { +static Preprocessor *PP; +} // namespace + +// Returns the expression of destination's capacity which is part of a +// 'VariableArrayType', 'ConstantArrayTypeLoc' or an argument of a 'malloc()' +// family function call. +static const Expr *getDestCapacityExpr(const MatchFinder::MatchResult &Result) { + if (const auto *DestMalloc = Result.Nodes.getNodeAs(DestMallocExprName)) + return DestMalloc; + + if (const auto *DestVAT = + Result.Nodes.getNodeAs(DestArrayTyName)) + return DestVAT->getSizeExpr(); + + if (const auto *DestVD = Result.Nodes.getNodeAs(DestVarDeclName)) + if (const TypeLoc DestTL = DestVD->getTypeSourceInfo()->getTypeLoc()) + if (const auto DestCTL = DestTL.getAs()) + return DestCTL.getSizeExpr(); + + return nullptr; +} + +// Returns the length of \p E as an 'IntegerLiteral' or a 'StringLiteral' +// without the null-terminator. +static int getLength(const Expr *E, const MatchFinder::MatchResult &Result) { + if (!E) + return 0; + + Expr::EvalResult Length; + E = E->IgnoreImpCasts(); + + if (const auto *LengthDRE = dyn_cast(E)) + if (const auto *LengthVD = dyn_cast(LengthDRE->getDecl())) + if (!isa(LengthVD)) + if (const Expr *LengthInit = LengthVD->getInit()) + if (LengthInit->EvaluateAsInt(Length, *Result.Context)) + return Length.Val.getInt().getZExtValue(); + + if (const auto *LengthIL = dyn_cast(E)) + return LengthIL->getValue().getZExtValue(); + + if (const auto *StrDRE = dyn_cast(E)) + if (const auto *StrVD = dyn_cast(StrDRE->getDecl())) + if (const Expr *StrInit = StrVD->getInit()) + if (const auto *StrSL = + dyn_cast(StrInit->IgnoreImpCasts())) + return StrSL->getLength(); + + if (const auto *SrcSL = dyn_cast(E)) + return SrcSL->getLength(); + + return 0; +} + +// Returns the capacity of the destination array. +// For example in 'char dest[13]; memcpy(dest, ...)' it returns 13. +static int getDestCapacity(const MatchFinder::MatchResult &Result) { + if (const auto *DestCapacityExpr = getDestCapacityExpr(Result)) + return getLength(DestCapacityExpr, Result); + + return 0; +} + +// Returns the 'strlen()' if it is the given length. +static const CallExpr *getStrlenExpr(const MatchFinder::MatchResult &Result) { + if (const auto *StrlenExpr = + Result.Nodes.getNodeAs(WrongLengthExprName)) + if (const Decl *D = StrlenExpr->getCalleeDecl()) + if (const FunctionDecl *FD = D->getAsFunction()) + if (const IdentifierInfo *II = FD->getIdentifier()) + if (II->isStr("strlen") || II->isStr("wcslen")) + return StrlenExpr; + + return nullptr; +} + +// Returns the length which is given in the memory/string handler function. +// For example in 'memcpy(dest, "foobar", 3)' it returns 3. +static int getGivenLength(const MatchFinder::MatchResult &Result) { + if (Result.Nodes.getNodeAs(UnknownLengthName)) + return 0; + + if (int Length = + getLength(Result.Nodes.getNodeAs(WrongLengthExprName), Result)) + return Length; + + if (int Length = + getLength(Result.Nodes.getNodeAs(LengthExprName), Result)) + return Length; + + // Special case, for example 'strlen("foo")'. + if (const CallExpr *StrlenCE = getStrlenExpr(Result)) + if (const Expr *Arg = StrlenCE->getArg(0)->IgnoreImpCasts()) + if (int ArgLength = getLength(Arg, Result)) + return ArgLength; + + return 0; +} + +// Returns a string representation of \p E. +static StringRef exprToStr(const Expr *E, + const MatchFinder::MatchResult &Result) { + if (!E) + return ""; + + return Lexer::getSourceText( + CharSourceRange::getTokenRange(E->getSourceRange()), + *Result.SourceManager, Result.Context->getLangOpts(), 0); +} + +// Returns the proper token based end location of \p E. +static SourceLocation exprLocEnd(const Expr *E, + const MatchFinder::MatchResult &Result) { + return Lexer::getLocForEndOfToken(E->getEndLoc(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); +} + +//===----------------------------------------------------------------------===// +// Rewrite decision helper functions. +//===----------------------------------------------------------------------===// + +// Increment by integer '1' can result in overflow if it is the maximal value. +// After that it would be extended to 'size_t' and its value would be wrong, +// therefore we have to inject '+ 1UL' instead. +static bool isInjectUL(const MatchFinder::MatchResult &Result) { + return getGivenLength(Result) == std::numeric_limits::max(); +} + +// If the capacity of the destination array is unknown it is denoted as unknown. +static bool isKnownDest(const MatchFinder::MatchResult &Result) { + return !Result.Nodes.getNodeAs(UnknownDestName); +} + +// True if the capacity of the destination array is based on the given length, +// therefore we assume that it cannot overflow (e.g. 'malloc(given_length + 1)' +static bool isDestBasedOnGivenLength(const MatchFinder::MatchResult &Result) { + StringRef DestCapacityExprStr = + exprToStr(getDestCapacityExpr(Result), Result).trim(); + StringRef LengthExprStr = + exprToStr(Result.Nodes.getNodeAs(LengthExprName), Result).trim(); + + return DestCapacityExprStr != "" && LengthExprStr != "" && + DestCapacityExprStr.contains(LengthExprStr); +} + +// Writing and reading from the same memory cannot remove the null-terminator. +static bool isDestAndSrcEquals(const MatchFinder::MatchResult &Result) { + if (const auto *DestDRE = Result.Nodes.getNodeAs(DestExprName)) + if (const auto *SrcDRE = Result.Nodes.getNodeAs(SrcExprName)) + return DestDRE->getDecl()->getCanonicalDecl() == + SrcDRE->getDecl()->getCanonicalDecl(); + + return false; +} + +// For example 'std::string str = "foo"; memcpy(dst, str.data(), str.length())'. +static bool isStringDataAndLength(const MatchFinder::MatchResult &Result) { + const auto *DestExpr = + Result.Nodes.getNodeAs(DestExprName); + const auto *SrcExpr = Result.Nodes.getNodeAs(SrcExprName); + const auto *LengthExpr = + Result.Nodes.getNodeAs(WrongLengthExprName); + + StringRef DestStr = "", SrcStr = "", LengthStr = ""; + if (DestExpr) + if (const CXXMethodDecl *DestMD = DestExpr->getMethodDecl()) + DestStr = DestMD->getName(); + + if (SrcExpr) + if (const CXXMethodDecl *SrcMD = SrcExpr->getMethodDecl()) + SrcStr = SrcMD->getName(); + + if (LengthExpr) + if (const CXXMethodDecl *LengthMD = LengthExpr->getMethodDecl()) + LengthStr = LengthMD->getName(); + + return (LengthStr == "length" || LengthStr == "size") && + (SrcStr == "data" || DestStr == "data"); +} + +static bool +isGivenLengthEqualToSrcLength(const MatchFinder::MatchResult &Result) { + if (Result.Nodes.getNodeAs(UnknownLengthName)) + return false; + + if (isStringDataAndLength(Result)) + return true; + + int GivenLength = getGivenLength(Result); + int SrcLength = getLength(Result.Nodes.getNodeAs(SrcExprName), Result); + + if (GivenLength != 0 && SrcLength != 0 && GivenLength == SrcLength) + return true; + + if (const auto *LengthExpr = Result.Nodes.getNodeAs(LengthExprName)) + if (dyn_cast(LengthExpr->IgnoreParenImpCasts())) + return false; + + // Check the strlen()'s argument's 'VarDecl' is equal to the source 'VarDecl'. + if (const CallExpr *StrlenCE = getStrlenExpr(Result)) + if (const auto *ArgDRE = + dyn_cast(StrlenCE->getArg(0)->IgnoreImpCasts())) + if (const auto *SrcVD = Result.Nodes.getNodeAs(SrcVarDeclName)) + return dyn_cast(ArgDRE->getDecl()) == SrcVD; + + return false; +} + +static bool isCorrectGivenLength(const MatchFinder::MatchResult &Result) { + if (Result.Nodes.getNodeAs(UnknownLengthName)) + return false; + + return !isGivenLengthEqualToSrcLength(Result); +} + +// If we rewrite the function call we need to create extra space to hold the +// null terminator. The new necessary capacity overflows without that '+ 1' +// size and we need to correct the given capacity. +static bool isDestCapacityOverflows(const MatchFinder::MatchResult &Result) { + if (!isKnownDest(Result)) + return true; + + const Expr *DestCapacityExpr = getDestCapacityExpr(Result); + int DestCapacity = getLength(DestCapacityExpr, Result); + int GivenLength = getGivenLength(Result); + + if (GivenLength != 0 && DestCapacity != 0) + return isGivenLengthEqualToSrcLength(Result) && DestCapacity == GivenLength; + + // Assume that the destination array's capacity cannot overflow if the + // expression of the memory allocation contains '+ 1'. + StringRef DestCapacityExprStr = exprToStr(DestCapacityExpr, Result); + if (DestCapacityExprStr.contains("+1") || DestCapacityExprStr.contains("+ 1")) + return false; + + return true; +} + +static bool +isFixedGivenLengthAndUnknownSrc(const MatchFinder::MatchResult &Result) { + if (Result.Nodes.getNodeAs(WrongLengthExprName)) + return !getLength(Result.Nodes.getNodeAs(SrcExprName), Result); + + return false; +} + +//===----------------------------------------------------------------------===// +// Code injection functions. +//===----------------------------------------------------------------------===// + +// Increase or decrease \p LengthExpr by one. +static void lengthExprHandle(const Expr *LengthExpr, + LengthHandleKind LengthHandle, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + LengthExpr = LengthExpr->IgnoreParenImpCasts(); + + // See whether we work with a macro. + bool IsMacroDefinition = false; + StringRef LengthExprStr = exprToStr(LengthExpr, Result); + Preprocessor::macro_iterator It = PP->macro_begin(); + while (It != PP->macro_end() && !IsMacroDefinition) { + if (It->first->getName() == LengthExprStr) + IsMacroDefinition = true; + + ++It; + } + + // Try to obtain an 'IntegerLiteral' and adjust it. + if (!IsMacroDefinition) { + if (const auto *LengthIL = dyn_cast(LengthExpr)) { + size_t NewLength = LengthIL->getValue().getZExtValue() + + (LengthHandle == LengthHandleKind::Increase + ? (isInjectUL(Result) ? 1UL : 1) + : -1); + + const auto NewLengthFix = FixItHint::CreateReplacement( + LengthIL->getSourceRange(), + (Twine(NewLength) + (isInjectUL(Result) ? "UL" : "")).str()); + Diag << NewLengthFix; + return; + } + } + + // Try to obtain and remove the '+ 1' string as a decrement fix. + const auto *BO = dyn_cast(LengthExpr); + if (BO && BO->getOpcode() == BO_Add && + LengthHandle == LengthHandleKind::Decrease) { + const Expr *LhsExpr = BO->getLHS()->IgnoreImpCasts(); + const Expr *RhsExpr = BO->getRHS()->IgnoreImpCasts(); + + if (const auto *LhsIL = dyn_cast(LhsExpr)) { + if (LhsIL->getValue().getZExtValue() == 1) { + Diag << FixItHint::CreateRemoval( + {LhsIL->getBeginLoc(), + RhsExpr->getBeginLoc().getLocWithOffset(-1)}); + return; + } + } + + if (const auto *RhsIL = dyn_cast(RhsExpr)) { + if (RhsIL->getValue().getZExtValue() == 1) { + Diag << FixItHint::CreateRemoval( + {LhsExpr->getEndLoc().getLocWithOffset(1), RhsIL->getEndLoc()}); + return; + } + } + } + + // Try to inject the '+ 1'/'- 1' string. + bool NeedInnerParen = BO && BO->getOpcode() != BO_Add; + + if (NeedInnerParen) + Diag << FixItHint::CreateInsertion(LengthExpr->getBeginLoc(), "("); + + SmallString<8> Injection; + if (NeedInnerParen) + Injection += ')'; + Injection += LengthHandle == LengthHandleKind::Increase ? " + 1" : " - 1"; + if (isInjectUL(Result)) + Injection += "UL"; + + Diag << FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), Injection); +} + +static void lengthArgHandle(LengthHandleKind LengthHandle, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *LengthExpr = Result.Nodes.getNodeAs(LengthExprName); + lengthExprHandle(LengthExpr, LengthHandle, Result, Diag); +} + +static void lengthArgPosHandle(unsigned ArgPos, LengthHandleKind LengthHandle, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + lengthExprHandle(FunctionExpr->getArg(ArgPos), LengthHandle, Result, Diag); +} + +// The string handler functions are only operates with plain 'char'/'wchar_t' +// without 'unsigned/signed', therefore we need to cast it. +static bool isDestExprFix(const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *Dest = Result.Nodes.getNodeAs(DestExprName); + if (!Dest) + return false; + + std::string TempTyStr = Dest->getType().getAsString(); + StringRef TyStr = TempTyStr; + if (TyStr.startswith("char") || TyStr.startswith("wchar_t")) + return false; + + Diag << FixItHint::CreateInsertion(Dest->getBeginLoc(), "(char *)"); + return true; +} + +// If the destination array is the same length as the given length we have to +// increase the capacity by one to create space for the the null terminator. +static bool isDestCapacityFix(const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + bool IsOverflows = isDestCapacityOverflows(Result); + if (IsOverflows) + if (const Expr *CapacityExpr = getDestCapacityExpr(Result)) + lengthExprHandle(CapacityExpr, LengthHandleKind::Increase, Result, Diag); + + return IsOverflows; +} + +static void removeArg(int ArgPos, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + // This is the following structure: (src, '\0', strlen(src)) + // ArgToRemove: ~~~~~~~~~~~ + // LHSArg: ~~~~ + // RemoveArgFix: ~~~~~~~~~~~~~ + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + const Expr *ArgToRemove = FunctionExpr->getArg(ArgPos); + const Expr *LHSArg = FunctionExpr->getArg(ArgPos - 1); + const auto RemoveArgFix = FixItHint::CreateRemoval( + SourceRange(exprLocEnd(LHSArg, Result), + exprLocEnd(ArgToRemove, Result).getLocWithOffset(-1))); + Diag << RemoveArgFix; +} + +static void renameFunc(StringRef NewFuncName, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + int FuncNameLength = + FunctionExpr->getDirectCallee()->getIdentifier()->getLength(); + SourceRange FuncNameRange( + FunctionExpr->getBeginLoc(), + FunctionExpr->getBeginLoc().getLocWithOffset(FuncNameLength - 1)); + + const auto FuncNameFix = + FixItHint::CreateReplacement(FuncNameRange, NewFuncName); + Diag << FuncNameFix; +} + +static void renameMemcpy(StringRef Name, bool IsCopy, bool IsSafe, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + SmallString<10> NewFuncName; + NewFuncName = (Name[0] != 'w') ? "str" : "wcs"; + NewFuncName += IsCopy ? "cpy" : "ncpy"; + NewFuncName += IsSafe ? "_s" : ""; + renameFunc(NewFuncName, Result, Diag); +} + +static void insertDestCapacityArg(bool IsOverflows, StringRef Name, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + SmallString<64> NewSecondArg; + + if (int DestLength = getDestCapacity(Result)) { + NewSecondArg = Twine(IsOverflows ? DestLength + 1 : DestLength).str(); + } else { + NewSecondArg = + (Twine(exprToStr(getDestCapacityExpr(Result), Result)) + + (IsOverflows ? (!isInjectUL(Result) ? " + 1" : " + 1UL") : "")) + .str(); + } + + NewSecondArg += ", "; + const auto InsertNewArgFix = FixItHint::CreateInsertion( + FunctionExpr->getArg(1)->getBeginLoc(), NewSecondArg); + Diag << InsertNewArgFix; +} + +static void insertNullTerminatorExpr(StringRef Name, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + int FuncLocStartColumn = Result.SourceManager->getPresumedColumnNumber( + FunctionExpr->getBeginLoc()); + SourceRange SpaceRange( + FunctionExpr->getBeginLoc().getLocWithOffset(-FuncLocStartColumn + 1), + FunctionExpr->getBeginLoc()); + StringRef SpaceBeforeStmtStr = Lexer::getSourceText( + CharSourceRange::getCharRange(SpaceRange), *Result.SourceManager, + Result.Context->getLangOpts(), 0); + + SmallString<128> NewAddNullTermExprStr; + NewAddNullTermExprStr = + (Twine('\n') + SpaceBeforeStmtStr + + exprToStr(Result.Nodes.getNodeAs(DestExprName), Result) + "[" + + exprToStr(Result.Nodes.getNodeAs(LengthExprName), Result) + + "] = " + ((Name[0] != 'w') ? "\'\\0\';" : "L\'\\0\';")) + .str(); + + const auto AddNullTerminatorExprFix = FixItHint::CreateInsertion( + exprLocEnd(FunctionExpr, Result).getLocWithOffset(1), + NewAddNullTermExprStr); + Diag << AddNullTerminatorExprFix; +} + +//===----------------------------------------------------------------------===// +// Checker logic with the matchers. +//===----------------------------------------------------------------------===// + +NotNullTerminatedResultCheck::NotNullTerminatedResultCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + WantToUseSafeFunctions(Options.get("WantToUseSafeFunctions", 1)) {} + +void NotNullTerminatedResultCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "WantToUseSafeFunctions", WantToUseSafeFunctions); +} + +void NotNullTerminatedResultCheck::registerPPCallbacks( + const SourceManager &SM, Preprocessor *pp, Preprocessor *ModuleExpanderPP) { + PP = pp; +} + +namespace { +AST_MATCHER_P(Expr, hasDefinition, ast_matchers::internal::Matcher, + InnerMatcher) { + const Expr *SimpleNode = &Node; + SimpleNode = SimpleNode->IgnoreParenImpCasts(); + + if (InnerMatcher.matches(*SimpleNode, Finder, Builder)) + return true; + + auto DREHasInit = ignoringImpCasts( + declRefExpr(to(varDecl(hasInitializer(ignoringImpCasts(InnerMatcher)))))); + + if (DREHasInit.matches(*SimpleNode, Finder, Builder)) + return true; + + const char *const VarDeclName = "variable-declaration"; + auto DREHasDefinition = ignoringImpCasts(declRefExpr( + allOf(to(varDecl().bind(VarDeclName)), + hasAncestor(compoundStmt(hasDescendant(binaryOperator( + hasLHS(declRefExpr(to(varDecl(equalsBoundNode(VarDeclName))))), + hasRHS(ignoringImpCasts(InnerMatcher))))))))); + + if (DREHasDefinition.matches(*SimpleNode, Finder, Builder)) + return true; + + return false; +} +} // namespace + +void NotNullTerminatedResultCheck::registerMatchers(MatchFinder *Finder) { + auto IncOp = + binaryOperator(hasOperatorName("+"), + hasEitherOperand(ignoringParenImpCasts(integerLiteral()))); + + auto DecOp = + binaryOperator(hasOperatorName("-"), + hasEitherOperand(ignoringParenImpCasts(integerLiteral()))); + + auto HasIncOp = anyOf(ignoringImpCasts(IncOp), hasDescendant(IncOp)); + auto HasDecOp = anyOf(ignoringImpCasts(DecOp), hasDescendant(DecOp)); + + auto Container = ignoringImpCasts(cxxMemberCallExpr(hasDescendant(declRefExpr( + hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(recordDecl( + hasAnyName("::std::vector", "::std::list", "::std::deque")))))))))); + + auto StringTy = type(hasUnqualifiedDesugaredType(recordType( + hasDeclaration(cxxRecordDecl(hasName("::std::basic_string")))))); + + auto AnyOfStringTy = + anyOf(hasType(StringTy), hasType(qualType(pointsTo(StringTy)))); + + auto CharTyArray = hasType(qualType(hasCanonicalType( + arrayType(hasElementType(isAnyCharacter())).bind(DestArrayTyName)))); + + auto CharTyPointer = hasType( + qualType(hasCanonicalType(pointerType(pointee(isAnyCharacter()))))); + + auto AnyOfCharTy = anyOf(CharTyArray, CharTyPointer); + + //===--------------------------------------------------------------------===// + // The following six cases match problematic length expressions. + //===--------------------------------------------------------------------===// + + // - Example: char src[] = "foo"; strlen(src); + auto Strlen = + callExpr(callee(functionDecl(hasAnyName("::strlen", "::wcslen")))) + .bind(WrongLengthExprName); + + // - Example: std::string str = "foo"; str.size(); + auto SizeOrLength = + cxxMemberCallExpr( + allOf(on(expr(AnyOfStringTy).bind("Foo")), + has(memberExpr(member(hasAnyName("size", "length")))))) + .bind(WrongLengthExprName); + + // - Example: char src[] = "foo"; sizeof(src); + auto SizeOfCharExpr = unaryExprOrTypeTraitExpr(has(expr(AnyOfCharTy))); + + auto WrongLength = + ignoringImpCasts(anyOf(Strlen, SizeOrLength, hasDescendant(Strlen), + hasDescendant(SizeOrLength))); + + // - Example: length = strlen(src); + auto DREWithoutInc = + ignoringImpCasts(declRefExpr(to(varDecl(hasInitializer(WrongLength))))); + + auto AnyOfCallOrDREWithoutInc = anyOf(DREWithoutInc, WrongLength); + + // - Example: int getLength(const char *str) { return strlen(str); } + auto CallExprReturnWithoutInc = ignoringImpCasts(callExpr(callee(functionDecl( + hasBody(has(returnStmt(hasReturnValue(AnyOfCallOrDREWithoutInc)))))))); + + // - Example: int length = getLength(src); + auto DREHasReturnWithoutInc = ignoringImpCasts( + declRefExpr(to(varDecl(hasInitializer(CallExprReturnWithoutInc))))); + + auto AnyOfWrongLengthInit = + anyOf(WrongLength, AnyOfCallOrDREWithoutInc, CallExprReturnWithoutInc, + DREHasReturnWithoutInc); + + //===--------------------------------------------------------------------===// + // The following five cases match the 'destination' array length's + // expression which is used in 'memcpy()' and 'memmove()' matchers. + //===--------------------------------------------------------------------===// + + // Note: Sometimes the size of char is explicitly written out. + auto SizeExpr = anyOf(SizeOfCharExpr, integerLiteral(equals(1))); + + auto MallocLengthExpr = allOf( + callee(functionDecl( + hasAnyName("::alloca", "::calloc", "malloc", "realloc"))), + hasAnyArgument(allOf(unless(SizeExpr), expr().bind(DestMallocExprName)))); + + // - Example: (char *)malloc(length); + auto DestMalloc = anyOf(callExpr(MallocLengthExpr), + hasDescendant(callExpr(MallocLengthExpr))); + + // - Example: new char[length]; + auto DestCXXNewExpr = ignoringImpCasts( + cxxNewExpr(hasArraySize(expr().bind(DestMallocExprName)))); + + auto AnyOfDestInit = anyOf(DestMalloc, DestCXXNewExpr); + + // - Example: char dest[13]; or char dest[length]; + auto DestArrayTyDecl = declRefExpr( + to(anyOf(varDecl(CharTyArray).bind(DestVarDeclName), + varDecl(hasInitializer(AnyOfDestInit)).bind(DestVarDeclName)))); + + // - Example: foo[bar[baz]].qux; (or just ParmVarDecl) + auto DestUnknownDecl = + declRefExpr(allOf(to(varDecl(AnyOfCharTy).bind(DestVarDeclName)), + expr().bind(UnknownDestName))) + .bind(DestExprName); + + auto AnyOfDestDecl = ignoringImpCasts( + anyOf(allOf(hasDefinition(anyOf(AnyOfDestInit, DestArrayTyDecl, + hasDescendant(DestArrayTyDecl))), + expr().bind(DestExprName)), + anyOf(DestUnknownDecl, hasDescendant(DestUnknownDecl)))); + + auto NullTerminatorExpr = binaryOperator( + hasLHS(anyOf(hasDescendant(declRefExpr( + to(varDecl(equalsBoundNode(DestVarDeclName))))), + hasDescendant(declRefExpr(equalsBoundNode(DestExprName))))), + hasRHS(ignoringImpCasts( + anyOf(characterLiteral(equals(0U)), integerLiteral(equals(0)))))); + + auto SrcDecl = declRefExpr( + allOf(to(decl().bind(SrcVarDeclName)), + anyOf(hasAncestor(cxxMemberCallExpr().bind(SrcExprName)), + expr().bind(SrcExprName)))); + + auto AnyOfSrcDecl = + ignoringImpCasts(anyOf(stringLiteral().bind(SrcExprName), + hasDescendant(stringLiteral().bind(SrcExprName)), + SrcDecl, hasDescendant(SrcDecl))); + + //===--------------------------------------------------------------------===// + // Match the problematic function calls. + //===--------------------------------------------------------------------===// + + struct CallContext { + CallContext(StringRef Name, Optional DestinationPos, + Optional SourcePos, unsigned LengthPos, + bool WithIncrease) + : Name(Name), DestinationPos(DestinationPos), SourcePos(SourcePos), + LengthPos(LengthPos), WithIncrease(WithIncrease){}; + + StringRef Name; + Optional DestinationPos; + Optional SourcePos; + unsigned LengthPos; + bool WithIncrease; + }; + + auto MatchDestination = [=](CallContext CC) { + return hasArgument(*CC.DestinationPos, + allOf(AnyOfDestDecl, + unless(hasAncestor(compoundStmt( + hasDescendant(NullTerminatorExpr)))), + unless(Container))); + }; + + auto MatchSource = [=](CallContext CC) { + return hasArgument(*CC.SourcePos, AnyOfSrcDecl); + }; + + auto MatchGivenLength = [=](CallContext CC) { + return hasArgument( + CC.LengthPos, + allOf( + anyOf( + ignoringImpCasts(integerLiteral().bind(WrongLengthExprName)), + allOf(unless(hasDefinition(SizeOfCharExpr)), + allOf(CC.WithIncrease + ? ignoringImpCasts(hasDefinition(HasIncOp)) + : ignoringImpCasts(allOf( + unless(hasDefinition(HasIncOp)), + anyOf(hasDefinition(binaryOperator().bind( + UnknownLengthName)), + hasDefinition(anything())))), + AnyOfWrongLengthInit))), + expr().bind(LengthExprName))); + }; + + auto MatchCall = [=](CallContext CC) { + std::string CharHandlerFuncName = "::" + CC.Name.str(); + + // Try to match with 'wchar_t' based function calls. + std::string WcharHandlerFuncName = + "::" + (CC.Name.startswith("mem") ? "w" + CC.Name.str() + : "wcs" + CC.Name.substr(3).str()); + + return allOf(callee(functionDecl( + hasAnyName(CharHandlerFuncName, WcharHandlerFuncName))), + MatchGivenLength(CC)); + }; + + auto Match = [=](CallContext CC) { + if (CC.DestinationPos && CC.SourcePos) + return allOf(MatchCall(CC), MatchDestination(CC), MatchSource(CC)); + + if (CC.DestinationPos && !CC.SourcePos) + return allOf(MatchCall(CC), MatchDestination(CC), + hasArgument(*CC.DestinationPos, anything())); + + if (!CC.DestinationPos && CC.SourcePos) + return allOf(MatchCall(CC), MatchSource(CC), + hasArgument(*CC.SourcePos, anything())); + + llvm_unreachable("Unhandled match"); + }; + + // void *memcpy(void *dest, const void *src, size_t count) + auto Memcpy = Match({"memcpy", 0, 1, 2, false}); + + // errno_t memcpy_s(void *dest, size_t ds, const void *src, size_t count) + auto Memcpy_s = Match({"memcpy_s", 0, 2, 3, false}); + + // void *memchr(const void *src, int c, size_t count) + auto Memchr = Match({"memchr", None, 0, 2, false}); + + // void *memmove(void *dest, const void *src, size_t count) + auto Memmove = Match({"memmove", 0, 1, 2, false}); + + // errno_t memmove_s(void *dest, size_t ds, const void *src, size_t count) + auto Memmove_s = Match({"memmove_s", 0, 2, 3, false}); + + // int strncmp(const char *str1, const char *str2, size_t count); + auto StrncmpRHS = Match({"strncmp", None, 1, 2, true}); + auto StrncmpLHS = Match({"strncmp", None, 0, 2, true}); + + // size_t strxfrm(char *dest, const char *src, size_t count); + auto Strxfrm = Match({"strxfrm", 0, 1, 2, false}); + + // errno_t strerror_s(char *buffer, size_t bufferSize, int errnum); + auto Strerror_s = Match({"strerror_s", 0, None, 1, false}); + + auto AnyOfMatchers = anyOf(Memcpy, Memcpy_s, Memmove, Memmove_s, StrncmpRHS, + StrncmpLHS, Strxfrm, Strerror_s); + + Finder->addMatcher(callExpr(AnyOfMatchers).bind(FunctionExprName), this); + + // Need to remove the CastExpr from 'memchr()' as 'strchr()' returns 'char *'. + Finder->addMatcher( + callExpr(Memchr, + unless(hasAncestor(castExpr(unless(implicitCastExpr()))))) + .bind(FunctionExprName), + this); + Finder->addMatcher( + castExpr(allOf(unless(implicitCastExpr()), + has(callExpr(Memchr).bind(FunctionExprName)))) + .bind(CastExprName), + this); +} + +void NotNullTerminatedResultCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + if (FunctionExpr->getBeginLoc().isMacroID()) + return; + + if (WantToUseSafeFunctions && PP->isMacroDefined("__STDC_LIB_EXT1__")) { + Optional AreSafeFunctionsWanted; + + Preprocessor::macro_iterator It = PP->macro_begin(); + while (It != PP->macro_end() && !AreSafeFunctionsWanted.hasValue()) { + if (It->first->getName() == "__STDC_WANT_LIB_EXT1__") { + const auto *MI = PP->getMacroInfo(It->first); + const auto &T = MI->tokens().back(); + StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength()); + llvm::APInt IntValue; + ValueStr.getAsInteger(10, IntValue); + AreSafeFunctionsWanted = IntValue.getZExtValue(); + } + + ++It; + } + + if (AreSafeFunctionsWanted.hasValue()) + UseSafeFunctions = AreSafeFunctionsWanted.getValue(); + } + + StringRef Name = FunctionExpr->getDirectCallee()->getName(); + if (Name.startswith("mem") || Name.startswith("wmem")) + memoryHandlerFunctionFix(Name, Result); + else if (Name == "strerror_s") + strerror_sFix(Result); + else if (Name.endswith("ncmp")) + ncmpFix(Name, Result); + else if (Name.endswith("xfrm")) + xfrmFix(Name, Result); +} + +void NotNullTerminatedResultCheck::memoryHandlerFunctionFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + if (isCorrectGivenLength(Result)) + return; + + if (Name.endswith("chr")) { + memchrFix(Name, Result); + return; + } + + if ((Name.contains("cpy") || Name.contains("move")) && + (isDestAndSrcEquals(Result) || isFixedGivenLengthAndUnknownSrc(Result))) + return; + + auto Diag = + diag(Result.Nodes.getNodeAs(FunctionExprName)->getBeginLoc(), + "the result from calling '%0' is not null-terminated") + << Name; + + if (Name.endswith("cpy")) { + memcpyFix(Name, Result, Diag); + } else if (Name.endswith("cpy_s")) { + memcpy_sFix(Name, Result, Diag); + } else if (Name.endswith("move")) { + memmoveFix(Name, Result, Diag); + } else if (Name.endswith("move_s")) { + isDestCapacityFix(Result, Diag); + lengthArgHandle(LengthHandleKind::Increase, Result, Diag); + } +} + +void NotNullTerminatedResultCheck::memcpyFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + bool IsOverflows = isDestCapacityFix(Result, Diag); + bool IsDestFixed = isDestExprFix(Result, Diag); + + bool IsCopy = + isGivenLengthEqualToSrcLength(Result) || isDestBasedOnGivenLength(Result); + + bool IsSafe = UseSafeFunctions && IsOverflows && isKnownDest(Result) && + !isDestBasedOnGivenLength(Result); + + bool IsDestLengthNotRequired = + IsSafe && getLangOpts().CPlusPlus && + Result.Nodes.getNodeAs(DestArrayTyName) && !IsDestFixed; + + renameMemcpy(Name, IsCopy, IsSafe, Result, Diag); + + if (IsSafe && !IsDestLengthNotRequired) + insertDestCapacityArg(IsOverflows, Name, Result, Diag); + + if (IsCopy) + removeArg(2, Result, Diag); + + if (!IsCopy && !IsSafe) + insertNullTerminatorExpr(Name, Result, Diag); +} + +void NotNullTerminatedResultCheck::memcpy_sFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + bool IsOverflows = isDestCapacityFix(Result, Diag); + bool IsDestFixed = isDestExprFix(Result, Diag); + + bool RemoveDestLength = getLangOpts().CPlusPlus && + Result.Nodes.getNodeAs(DestArrayTyName) && + !IsDestFixed; + bool IsCopy = isGivenLengthEqualToSrcLength(Result); + bool IsSafe = IsOverflows; + + renameMemcpy(Name, IsCopy, IsSafe, Result, Diag); + + if (!IsSafe || (IsSafe && RemoveDestLength)) + removeArg(1, Result, Diag); + else if (IsOverflows && isKnownDest(Result)) + lengthArgPosHandle(1, LengthHandleKind::Increase, Result, Diag); + + if (IsCopy) + removeArg(3, Result, Diag); + + if (!IsCopy && !IsSafe) + insertNullTerminatorExpr(Name, Result, Diag); +} + +void NotNullTerminatedResultCheck::memchrFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + if (const auto GivenCL = dyn_cast(FunctionExpr->getArg(1))) + if (GivenCL->getValue() != 0) + return; + + auto Diag = diag(FunctionExpr->getArg(2)->IgnoreParenCasts()->getBeginLoc(), + "the length is too short to include the null terminator"); + + if (const auto *CastExpr = Result.Nodes.getNodeAs(CastExprName)) { + const auto CastRemoveFix = FixItHint::CreateRemoval( + SourceRange(CastExpr->getBeginLoc(), + FunctionExpr->getBeginLoc().getLocWithOffset(-1))); + Diag << CastRemoveFix; + } + + StringRef NewFuncName = (Name[0] != 'w') ? "strchr" : "wcschr"; + renameFunc(NewFuncName, Result, Diag); + removeArg(2, Result, Diag); +} + +void NotNullTerminatedResultCheck::memmoveFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + bool IsOverflows = isDestCapacityFix(Result, Diag); + + if (UseSafeFunctions && isKnownDest(Result)) { + renameFunc((Name[0] != 'w') ? "memmove_s" : "wmemmove_s", Result, Diag); + insertDestCapacityArg(IsOverflows, Name, Result, Diag); + } + + lengthArgHandle(LengthHandleKind::Increase, Result, Diag); +} + +void NotNullTerminatedResultCheck::strerror_sFix( + const MatchFinder::MatchResult &Result) { + auto Diag = + diag(Result.Nodes.getNodeAs(FunctionExprName)->getBeginLoc(), + "the result from calling 'strerror_s' is not null-terminated and " + "missing the last character of the error message"); + + isDestCapacityFix(Result, Diag); + lengthArgHandle(LengthHandleKind::Increase, Result, Diag); +} + +void NotNullTerminatedResultCheck::ncmpFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + const auto *FunctionExpr = Result.Nodes.getNodeAs(FunctionExprName); + const Expr *FirstArgExpr = FunctionExpr->getArg(0)->IgnoreImpCasts(); + const Expr *SecondArgExpr = FunctionExpr->getArg(1)->IgnoreImpCasts(); + bool IsLengthTooLong = false; + + if (const CallExpr *StrlenExpr = getStrlenExpr(Result)) { + const Expr *LengthExprArg = StrlenExpr->getArg(0); + StringRef FirstExprStr = exprToStr(FirstArgExpr, Result).trim(); + StringRef SecondExprStr = exprToStr(SecondArgExpr, Result).trim(); + StringRef LengthArgStr = exprToStr(LengthExprArg, Result).trim(); + IsLengthTooLong = + LengthArgStr == FirstExprStr || LengthArgStr == SecondExprStr; + } else { + int SrcLength = + getLength(Result.Nodes.getNodeAs(SrcExprName), Result); + int GivenLength = getGivenLength(Result); + if (SrcLength != 0 && GivenLength != 0) + IsLengthTooLong = GivenLength > SrcLength; + } + + if (!IsLengthTooLong && !isStringDataAndLength(Result)) + return; + + auto Diag = diag(FunctionExpr->getArg(2)->IgnoreParenCasts()->getBeginLoc(), + "comparison length is too long and might lead to a " + "buffer overflow"); + + lengthArgHandle(LengthHandleKind::Decrease, Result, Diag); +} + +void NotNullTerminatedResultCheck::xfrmFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + if (!isDestCapacityOverflows(Result)) + return; + + auto Diag = + diag(Result.Nodes.getNodeAs(FunctionExprName)->getBeginLoc(), + "the result from calling '%0' is not null-terminated") + << Name; + + isDestCapacityFix(Result, Diag); + lengthArgHandle(LengthHandleKind::Increase, Result, Diag); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -164,6 +164,15 @@ Finds instances where variables with static storage are initialized dynamically in header files. +- New :doc:`bugprone-not-null-terminated-result + ` check + + Finds function calls where it is possible to cause a not null-terminated + result. Usually the proper length of a string is ``strlen(src) + 1`` or equal + length of this expression, because the null terminator needs an extra space. + Without the null terminator it can result in undefined behaviour when the + string is read. + - New :doc:`linuxkernel-must-use-errs ` check. Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst @@ -0,0 +1,127 @@ +.. title:: clang-tidy - bugprone-not-null-terminated-result + +bugprone-not-null-terminated-result +=================================== + +Finds function calls where it is possible to cause a not null-terminated result. +Usually the proper length of a string is ``strlen(src) + 1`` or equal length of +this expression, because the null terminator needs an extra space. Without the +null terminator it can result in undefined behaviour when the string is read. + +The following and their respective ``wchar_t`` based functions are checked: + +``memcpy``, ``memcpy_s``, ``memchr``, ``memmove``, ``memmove_s``, +``strerror_s``, ``strncmp``, ``strxfrm`` + +The following is a real-world example where the programmer forgot to increase +the passed third argument, which is ``size_t length``. That is why the length +of the allocated memory is not enough to hold the null terminator. + + .. code-block:: c + + static char *stringCpy(const std::string &str) { + char *result = reinterpret_cast(malloc(str.size())); + memcpy(result, str.data(), str.size()); + return result; + } + +In addition to issuing warnings, fix-it rewrites all the necessary code. It also +tries to adjust the capacity of the destination array: + + .. code-block:: c + + static char *stringCpy(const std::string &str) { + char *result = reinterpret_cast(malloc(str.size() + 1)); + strcpy(result, str.data()); + return result; + } + +Note: It cannot guarantee to rewrite every of the path-sensitive memory + allocations. + +.. _MemcpyTransformation: + +Transformation rules of 'memcpy()' +---------------------------------- + +It is possible to rewrite the ``memcpy()`` and ``memcpy_s()`` calls as the +following four functions: ``strcpy()``, ``strncpy()``, ``strcpy_s()``, +``strncpy_s()``, where the latter two are the safer versions of the former two. +It rewrites the ``wchar_t`` based memory handler functions respectively. + +Rewrite based on the destination array +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- If copy to the destination array cannot overflow [1] the new function should + be the older copy function (ending with ``cpy``), because it is more + efficient than the safe version. + +- If copy to the destination array can overflow [1] and + ``AreSafeFunctionsAvailable`` is set to ``Yes``, ``y`` or non-zero and it is + possible to obtain the capacity of the destination array then the new function + could be the safe version (ending with ``cpy_s``). + +- If the new function is could be safe version and C++ files are analysed and + the destination array is plain ``char``/``wchar_t`` without ``un/signed`` then + the length of the destination array can be omitted. + +- If the new function is could be safe version and the destination array is + ``un/signed`` it needs to be casted to plain ``char *``/``wchar_t *``. + +[1] It is possible to overflow: + - If the capacity of the destination array is unknown. + - If the given length is equal to the destination array's capacity. + +Rewrite based on the length of the source string +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- If the given length is ``strlen(source)`` or equal length of this expression + then the new function should be the older copy function (ending with ``cpy``), + as it is more efficient than the safe version (ending with ``cpy_s``). + +- Otherwise we assume that the programmer wanted to copy 'N' characters, so the + new function is ``ncpy``-like which copies 'N' characters. + +Transformations with 'strlen()' or equal length of this expression +------------------------------------------------------------------ + +It transforms the ``wchar_t`` based memory and string handler functions +respectively (where only ``strerror_s`` does not have ``wchar_t`` based alias). + +Memory handler functions +^^^^^^^^^^^^^^^^^^^^^^^^ + +- ``memcpy``: Visit the + :ref:`Transformation rules of 'memcpy()'` section. + +- ``memchr``: + - Usually there is a C-style cast and it is needed to be removed, because the + new function ``strchr``'s return type is correct. + - The given length is going to be removed. + +- ``memmove``: + - If safe functions are available the new function is ``memmove_s``, which has + a new second argument which is the length of the destination array, it is + adjusted, and the length of the source string is incremented by one. + - If safe functions are not available the given length is incremented by one. + +- ``memmove_s``: given length is incremented by one. + +String handler functions +^^^^^^^^^^^^^^^^^^^^^^^^ + +- ``strerror_s``: given length is incremented by one. + +- ``strncmp``: If the third argument is the first or the second argument's + ``length + 1`` it has to be truncated without the ``+ 1`` operation. + +- ``strxfrm``: given length is incremented by one. + +Options +------- + +.. option:: WantToUseSafeFunctions + + An integer non-zero value specifying if the target environment is considered + to implement '_s' suffixed memory and string handler functions which are + safer than older versions (e.g. 'memcpy_s()'). The default value is ``1``. Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -59,6 +59,7 @@ bugprone-misplaced-widening-cast bugprone-move-forwarding-reference bugprone-multiple-statement-macro + bugprone-not-null-terminated-result bugprone-parent-virtual-call bugprone-posix-return bugprone-sizeof-container Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-c.h =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-c.h @@ -0,0 +1,39 @@ +//===- not-null-terminated-result-c.h - Helper header -------------*- 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 +// +//===----------------------------------------------------------------------===// +// +// This header helps to maintain every function call checked by the +// NotNullTerminatedResult checker. +// +//===----------------------------------------------------------------------===// + +#pragma clang system_header + +typedef unsigned int size_t; +typedef int errno_t; + +size_t strlen(const char *str); +void *malloc(size_t size); +char *strerror(int errnum); +errno_t strerror_s(char *buffer, size_t bufferSize, int errnum); + +char *strcpy(char *dest, const char *src); +errno_t strcpy_s(char *dest, size_t destSize, const char *src); +char *strncpy(char *dest, const char *src, size_t count); +errno_t strncpy_s(char *dest, size_t destSize, const char *src, size_t count); + +void *memcpy(void *dest, const void *src, size_t count); +errno_t memcpy_s(void *dest, size_t destSize, const void *src, size_t count); + +char *strchr(char *str, int c); +int strncmp(const char *str1, const char *str2, size_t count); +size_t strxfrm(char *dest, const char *src, size_t count); + +void *memchr(const void *buffer, int c, size_t count); +void *memmove(void *dest, const void *src, size_t count); +errno_t memmove_s(void *dest, size_t destSize, const void *src, size_t count); +void *memset(void *dest, int c, size_t count); Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-cxx.h =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-not-null-terminated-result/not-null-terminated-result-cxx.h @@ -0,0 +1,65 @@ +//===- not-null-terminated-result-cxx.h - Helper header ---------*- 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 +// +//===----------------------------------------------------------------------===// +// +// This header helps to maintain every function call checked by the +// NotNullTerminatedResult checker. +// +//===----------------------------------------------------------------------===// + +#pragma clang system_header + +#include "not-null-terminated-result-c.h" + +namespace std { +template +struct basic_string { + basic_string(); + const T *data() const; + unsigned long size() const; + unsigned long length() const; +}; +typedef basic_string string; +} // namespace std + +size_t wcslen(const wchar_t *str); + +template +char *strcpy(char (&dest)[size], const char *src); +template +wchar_t *wcscpy(wchar_t (&dest)[size], const wchar_t *src); +wchar_t *wcscpy(wchar_t *dest, const wchar_t *src); + +template +errno_t strcpy_s(char (&dest)[size], const char *src); +template +errno_t wcscpy_s(wchar_t (&dest)[size], const wchar_t *src); +errno_t wcscpy_s(wchar_t *dest, size_t destSize, const wchar_t *src); + +template +char *strncpy(char (&dest)[size], const char *src, size_t count); +template +wchar_t *wcsncpy(wchar_t (&dest)[size], const wchar_t *src, size_t count); +wchar_t *wcsncpy(wchar_t *dest, const wchar_t *src, size_t count); + +template +errno_t strncpy_s(char (&dest)[size], const char *src, size_t count); +template +errno_t wcsncpy_s(wchar_t (&dest)[size], const wchar_t *src, size_t length); +errno_t wcsncpy_s(wchar_t *dest, size_t destSize, const wchar_t *src, size_t c); + +wchar_t *wmemcpy(wchar_t *dest, const wchar_t *src, size_t count); +errno_t wmemcpy_s(wchar_t *dest, size_t destSize, const wchar_t *src, size_t c); + +wchar_t *wcschr(const wchar_t *str, int c); +int wcsncmp(const wchar_t *str1, const wchar_t *str2, size_t count); +size_t wcsxfrm(wchar_t *dest, const wchar_t *src, size_t count); + +void *wmemchr(const void *buffer, int c, size_t count); +void *wmemmove(void *dest, const void *src, size_t count); +errno_t wmemmove_s(void *dest, size_t destSize, const void *src, size_t count); +void *wmemset(void *dest, int c, size_t count); Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c @@ -0,0 +1,84 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-c.h" + +void path_sensitive_unknown_length(char *position, const char *src) { + int length; + length = strlen(src); + position = (char *)memchr(src, '\0', length); +} + +void bad_memchr(char *position, const char *src) { + int length = strlen(src); + position = (char *)memchr(src, '\0', length); + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: the length is too short to include the null terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: position = strchr(src, '\0'); +} + +void good_memchr(char *pos, const char *src) { + pos = strchr(src, '\0'); +} + +void bad_strerror_s(int errno) { + char dest[13]; + int length = strlen(strerror(errno)); + strerror_s(dest, length, errno); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'strerror_s' is not null-terminated and missing the last character of the error message [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest[14]; + // CHECK-FIXES-NEXT: int length = strlen(strerror(errno)); + // CHECK-FIXES-NEXT: strerror_s(dest, length + 1, errno); +} + +void good_strerror_s(int errno) { + char dst[14]; + int length = strlen(strerror(errno)); + strerror_s(dst, length + 1, errno); +} + +int bad_strncmp_1(char *str1, const char *str2) { + int length = strlen(str1) + 1; + return strncmp(str1, str2, length); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncmp(str1, str2, length - 1); +} + +int good_strncmp_1(char *str1, const char *str2) { + int length = strlen(str1) + 1; + return strncmp(str1, str2, length - 1); +} + +int bad_strncmp_2(char *str2) { + return strncmp(str2, "foobar", (strlen("foobar") + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncmp(str2, "foobar", (strlen("foobar"))); +} + +int bad_strncmp_3(char *str3) { + return strncmp(str3, "foobar", 1 + strlen("foobar")); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncmp(str3, "foobar", strlen("foobar")); +} + +int good_strncmp_2_3(char *str) { + return strncmp(str, "foobar", strlen("foobar")); +} + +void bad_strxfrm(const char *long_source_name) { + char long_destination_name[13]; + int very_long_length_definition_name = strlen(long_source_name); + strxfrm(long_destination_name, long_source_name, + very_long_length_definition_name); + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: the result from calling 'strxfrm' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char long_destination_name[14]; + // CHECK-FIXES-NEXT: int very_long_length_definition_name = strlen(long_source_name); + // CHECK-FIXES-NEXT: strxfrm(long_destination_name, long_source_name, + // CHECK-FIXES-NEXT: very_long_length_definition_name + 1); +} + +void good_strxfrm(const char *long_source_name) { + char long_destination_name[14]; + int very_long_length_definition_name = strlen(long_source_name); + strxfrm(long_destination_name, long_source_name, + very_long_length_definition_name + 1); +} Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c @@ -0,0 +1,71 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: [{key: bugprone-not-null-terminated-result.WantToUseSafeFunctions, \ +// RUN: value: 1}]}" \ +// RUN: -- -std=c11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-c.h" + +// The following is not defined therefore the safe functions are unavailable. +// #define __STDC_LIB_EXT1__ 1 + +#define __STDC_WANT_LIB_EXT1__ 1 + +//===----------------------------------------------------------------------===// +// memcpy() - destination array tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_not_just_char_dest(const char *src) { + unsigned char dest00[13]; + memcpy(dest00, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: unsigned char dest00[14]; + // CHECK-FIXES-NEXT: strcpy((char *)dest00, src); +} + +void good_memcpy_not_just_char_dest(const char *src) { + unsigned char dst00[14]; + strcpy((char *)dst00, src); +} + +void bad_memcpy_known_dest(const char *src) { + char dest01[13]; + memcpy(dest01, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strcpy(dest01, src); +} + +void good_memcpy_known_dest(const char *src) { + char dst01[13]; + strcpy(dst01, src); +} + +//===----------------------------------------------------------------------===// +// memcpy() - length tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_full_source_length(const char *src) { + char dest20[13]; + memcpy(dest20, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strcpy(dest20, src); +} + +void good_memcpy_full_source_length(const char *src) { + char dst20[13]; + strcpy(dst20, src); +} + +void bad_memcpy_partial_source_length(const char *src) { + char dest21[13]; + memcpy(dest21, src, strlen(src) - 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncpy(dest21, src, strlen(src) - 1); + // CHECK-FIXES-NEXT: dest21[strlen(src) - 1] = '\0'; +} + +void good_memcpy_partial_source_length(const char *src) { + char dst21[13]; + strncpy(dst21, src, strlen(src) - 1); + dst21[strlen(src) - 1] = '\0'; +} Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp @@ -0,0 +1,124 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c++11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-cxx.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +//===----------------------------------------------------------------------===// +// memcpy() - destination array tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_not_just_char_dest(const char *src) { + unsigned char dest00[13]; + memcpy(dest00, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: unsigned char dest00[14]; + // CHECK-FIXES-NEXT: strcpy_s((char *)dest00, 14, src); +} + +void good_memcpy_not_just_char_dest(const char *src) { + unsigned char dst00[14]; + strcpy_s((char *)dst00, 14, src); +} + +void bad_memcpy_known_dest(const char *src) { + char dest01[13]; + memcpy(dest01, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: dest01[14]; + // CHECK-FIXES-NEXT: strcpy_s(dest01, src); +} + +void good_memcpy_known_dest(const char *src) { + char dst01[14]; + strcpy_s(dst01, src); +} + +//===----------------------------------------------------------------------===// +// memcpy() - length tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_full_source_length(std::string src) { + char *dest20 = reinterpret_cast(malloc(src.size())); + memcpy(dest20, src.data(), src.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: dest20 = reinterpret_cast(malloc(src.size() + 1)); + // CHECK-FIXES-NEXT: strcpy(dest20, src.data()); +} + +void good_memcpy_full_source_length(std::string src) { + char dst20[14]; + strcpy_s(dst20, src.data()); +} + +void bad_memcpy_partial_source_length(const char *src) { + char dest21[13]; + memcpy(dest21, src, strlen(src) - 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest21[14]; + // CHECK-FIXES-NEXT: strncpy_s(dest21, src, strlen(src) - 1); +} + +void good_memcpy_partial_source_length(const char *src) { + char dst21[14]; + strncpy_s(dst21, src, strlen(src) - 1); +} + +//===----------------------------------------------------------------------===// +// memcpy_s() - destination array tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_s_unknown_dest(char *dest40, const char *src) { + memcpy_s(dest40, 13, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strcpy_s(dest40, 13, src); +} + +void good_memcpy_s_unknown_dest(char *dst40, const char *src) { + strcpy_s(dst40, 13, src); +} + +void bad_memcpy_s_known_dest(const char *src) { + char dest41[13]; + memcpy_s(dest41, 13, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest41[14]; + // CHECK-FIXES: strcpy_s(dest41, src); +} + +void good_memcpy_s_known_dest(const char *src) { + char dst41[14]; + strcpy_s(dst41, src); +} + +//===----------------------------------------------------------------------===// +// memcpy_s() - length tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_s_full_source_length(const char *src) { + char dest60[13]; + memcpy_s(dest60, 13, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest60[14]; + // CHECK-FIXES-NEXT: strcpy_s(dest60, src); +} + +void good_memcpy_s_full_source_length(const char *src) { + char dst60[14]; + strcpy_s(dst60, src); +} + +void bad_memcpy_s_partial_source_length(const char *src) { + char dest61[13]; + memcpy_s(dest61, 13, src, strlen(src) - 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest61[14]; + // CHECK-FIXES-NEXT: strncpy_s(dest61, src, strlen(src) - 1); +} + +void good_memcpy_s_partial_source_length(const char *src) { + char dst61[14]; + strncpy_s(dst61, src, strlen(src) - 1); +} Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c @@ -0,0 +1,112 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-c.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +#define SRC_LENGTH 3 +#define SRC "foo" + +//===----------------------------------------------------------------------===// +// False positive suppression. +//===----------------------------------------------------------------------===// + +void good_memcpy_known_src() { + char dest[13]; + char src[] = "foobar"; + memcpy(dest, src, sizeof(src)); +} + +void good_memcpy_null_terminated(const char *src) { + char dest[13]; + const int length = strlen(src); + memcpy(dest, src, length); + dest[length] = '\0'; +} + +void good_memcpy_proper_length(const char *src) { + char *dest = 0; + int length = strlen(src) + 1; + dest = (char *)malloc(length); + memcpy(dest, src, length); +} + +void may_bad_memcpy_unknown_length(const char *src, int length) { + char dest[13]; + memcpy(dest, src, length); +} + +void may_bad_memcpy_const_length(const char *src) { + char dest[13]; + memcpy(dest, src, 12); +} + +//===----------------------------------------------------------------------===// +// Special cases. +//===----------------------------------------------------------------------===// + +void bad_memcpy_unknown_dest(char *dest01, const char *src) { + memcpy(dest01, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strcpy(dest01, src); +} + +void good_memcpy_unknown_dest(char *dst01, const char *src) { + strcpy(dst01, src); +} + +void bad_memcpy_variable_array(int dest_length) { + char dest02[dest_length + 1]; + memcpy(dest02, "foobarbazqux", strlen("foobarbazqux")); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strcpy(dest02, "foobarbazqux"); +} + +void good_memcpy_variable_array(int dest_length) { + char dst02[dest_length + 1]; + strcpy(dst02, "foobarbazqux"); +} + +void bad_memcpy_equal_src_length_and_length() { + char dest03[13]; + const char *src = "foobarbazqux"; + memcpy(dest03, src, 12); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strcpy(dest03, src); +} + +void good_memcpy_equal_src_length_and_length() { + char dst03[13]; + const char *src = "foobarbazqux"; + strcpy(dst03, src); +} + +void bad_memcpy_dest_size_overflows(const char *src) { + const int length = strlen(src); + char *dest04 = (char *)malloc(length); + memcpy(dest04, src, length); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char *dest04 = (char *)malloc(length + 1); + // CHECK-FIXES-NEXT: strcpy(dest04, src); +} + +void good_memcpy_dest_size_overflows(const char *src) { + const int length = strlen(src); + char *dst04 = (char *)malloc(length + 1); + strcpy(dst04, src); +} + +void bad_memcpy_macro() { + char dest05[SRC_LENGTH]; + memcpy(dest05, SRC, SRC_LENGTH); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest05[SRC_LENGTH + 1]; + // CHECK-FIXES-NEXT: strcpy(dest05, SRC); +} + +void good_memcpy_macro() { + char dst05[SRC_LENGTH + 1]; + strcpy(dst05, SRC); +} Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c @@ -0,0 +1,124 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-c.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +//===----------------------------------------------------------------------===// +// memcpy() - destination array tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_not_just_char_dest(const char *src) { + unsigned char dest00[13]; + memcpy(dest00, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: unsigned char dest00[14]; + // CHECK-FIXES-NEXT: strcpy_s((char *)dest00, 14, src); +} + +void good_memcpy_not_just_char_dest(const char *src) { + unsigned char dst00[14]; + strcpy_s((char *)dst00, 14, src); +} + +void bad_memcpy_known_dest(const char *src) { + char dest01[13]; + memcpy(dest01, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest01[14]; + // CHECK-FIXES: strcpy_s(dest01, 14, src); +} + +void good_memcpy_known_dest(const char *src) { + char dst01[14]; + strcpy_s(dst01, 14, src); +} + +//===----------------------------------------------------------------------===// +// memcpy() - length tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_full_source_length(const char *src) { + char dest20[13]; + memcpy(dest20, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest20[14]; + // CHECK-FIXES-NEXT: strcpy_s(dest20, 14, src); +} + +void good_memcpy_full_source_length(const char *src) { + char dst20[14]; + strcpy_s(dst20, 14, src); +} + +void bad_memcpy_partial_source_length(const char *src) { + char dest21[13]; + memcpy(dest21, src, strlen(src) - 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest21[14]; + // CHECK-FIXES-NEXT: strncpy_s(dest21, 14, src, strlen(src) - 1); +} + +void good__memcpy_partial_source_length(const char *src) { + char dst21[14]; + strncpy_s(dst21, 14, src, strlen(src) - 1); +} + +//===----------------------------------------------------------------------===// +// memcpy_s() - destination array tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_s_unknown_dest(char *dest40, const char *src) { + memcpy_s(dest40, 13, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strcpy_s(dest40, 13, src); +} + +void good_memcpy_s_unknown_dest(char *dst40, const char *src) { + strcpy_s(dst40, 13, src); +} + +void bad_memcpy_s_known_dest(const char *src) { + char dest41[13]; + memcpy_s(dest41, 13, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest41[14]; + // CHECK-FIXES-NEXT: strcpy_s(dest41, 14, src); +} + +void good_memcpy_s_known_dest(const char *src) { + char dst41[14]; + strcpy_s(dst41, 14, src); +} + +//===----------------------------------------------------------------------===// +// memcpy_s() - length tests +//===----------------------------------------------------------------------===// + +void bad_memcpy_s_full_source_length(const char *src) { + char dest60[13]; + memcpy_s(dest60, 13, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest60[14]; + // CHECK-FIXES-NEXT: strcpy_s(dest60, 14, src); +} + +void good_memcpy_s_full_source_length(const char *src) { + char dst60[14]; + strcpy_s(dst60, 14, src); +} + +void bad_memcpy_s_partial_source_length(const char *src) { + char dest61[13]; + memcpy_s(dest61, 13, src, strlen(src) - 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest61[14]; + // CHECK-FIXES-NEXT: strncpy_s(dest61, 14, src, strlen(src) - 1); +} + +void good_memcpy_s_partial_source_length(const char *src) { + char dst61[14]; + strncpy_s(dst61, 14, src, strlen(src) - 1); +} Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-strlen.c =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-strlen.c @@ -0,0 +1,118 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-c.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +void bad_memchr_1(char *position, const char *src) { + position = (char *)memchr(src, '\0', strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: the length is too short to include the null terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: position = strchr(src, '\0'); +} + +void good_memchr_1(char *pos, const char *src) { + pos = strchr(src, '\0'); +} + +void bad_memchr_2(char *position) { + position = (char *)memchr("foobar", '\0', 6); + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: the length is too short to include the null terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: position = strchr("foobar", '\0'); +} + +void good_memchr_2(char *pos) { + pos = strchr("foobar", '\0'); +} + +void bad_memmove(const char *src) { + char dest[13]; + memmove(dest, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memmove' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest[14]; + // CHECK-FIXES-NEXT: memmove_s(dest, 14, src, strlen(src) + 1); +} + +void good_memmove(const char *src) { + char dst[14]; + memmove_s(dst, 13, src, strlen(src) + 1); +} + +void bad_memmove_s(char *dest, const char *src) { + memmove_s(dest, 13, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memmove_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memmove_s(dest, 13, src, strlen(src) + 1); +} + +void good_memmove_s_1(char *dest, const char *src) { + memmove_s(dest, 13, src, strlen(src) + 1); +} + +void bad_strerror_s(int errno) { + char dest[13]; + strerror_s(dest, strlen(strerror(errno)), errno); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'strerror_s' is not null-terminated and missing the last character of the error message [bugprone-not-null-terminated-result] + // CHECK-FIXES: char dest[14]; + // CHECK-FIXES-NEXT: strerror_s(dest, strlen(strerror(errno)) + 1, errno); +} + +void good_strerror_s(int errno) { + char dst[14]; + strerror_s(dst, strlen(strerror(errno)) + 1, errno); +} + +int bad_strncmp_1(char *str0, const char *str1) { + return strncmp(str0, str1, (strlen(str0) + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncmp(str0, str1, (strlen(str0))); +} + +int bad_strncmp_2(char *str2, const char *str3) { + return strncmp(str2, str3, 1 + strlen(str2)); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncmp(str2, str3, strlen(str2)); +} + +int good_strncmp_1_2(char *str4, const char *str5) { + return strncmp(str4, str5, strlen(str4)); +} + +int bad_strncmp_3(char *str6) { + return strncmp(str6, "string", 7); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncmp(str6, "string", 6); +} + +int good_strncmp_3(char *str7) { + return strncmp(str7, "string", 6); +} + +void bad_strxfrm_1(const char *long_source_name) { + char long_destination_array_name[13]; + strxfrm(long_destination_array_name, long_source_name, + strlen(long_source_name)); + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: the result from calling 'strxfrm' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char long_destination_array_name[14]; + // CHECK-FIXES-NEXT: strxfrm(long_destination_array_name, long_source_name, + // CHECK-FIXES-NEXT: strlen(long_source_name) + 1); +} + +void good_strxfrm_1(const char *long_source_name) { + char long_destination_array_name[14]; + strxfrm(long_destination_array_name, long_source_name, + strlen(long_source_name) + 1); +} + +void bad_strxfrm_2() { + char long_destination_array_name1[16]; + strxfrm(long_destination_array_name1, "long_source_name", 16); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'strxfrm' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: char long_destination_array_name1[17]; + // CHECK-FIXES: strxfrm(long_destination_array_name1, "long_source_name", 17); +} + +void good_strxfrm_2() { + char long_destination_array_name2[17]; + strxfrm(long_destination_array_name2, "long_source_name", 17); +} Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp @@ -0,0 +1,106 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c++11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-cxx.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +void bad_wmemchr_1(wchar_t *position, const wchar_t *src) { + position = (wchar_t *)wmemchr(src, L'\0', wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: the length is too short to include the null terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: position = wcschr(src, L'\0'); +} + +void good_wmemchr_1(wchar_t *pos, const wchar_t *src) { + pos = wcschr(src, L'\0'); +} + +void bad_wmemchr_2(wchar_t *position) { + position = (wchar_t *)wmemchr(L"foobar", L'\0', 6); + // CHECK-MESSAGES: :[[@LINE-1]]:51: warning: the length is too short to include the null terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: position = wcschr(L"foobar", L'\0'); +} + +void good_wmemchr_2(wchar_t *pos) { + pos = wcschr(L"foobar", L'\0'); +} + + +void bad_wmemmove(const wchar_t *src) { + wchar_t dest[13]; + wmemmove(dest, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemmove' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t dest[14]; + // CHECK-FIXES-NEXT: wmemmove_s(dest, 14, src, wcslen(src) + 1); +} + +void good_wmemmove(const wchar_t *src) { + wchar_t dst[14]; + wmemmove_s(dst, 13, src, wcslen(src) + 1); +} + +void bad_wmemmove_s(wchar_t *dest, const wchar_t *src) { + wmemmove_s(dest, 13, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemmove_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wmemmove_s(dest, 13, src, wcslen(src) + 1); +} + +void good_wmemmove_s_1(wchar_t *dest, const wchar_t *src) { + wmemmove_s(dest, 13, src, wcslen(src) + 1); +} + +int bad_wcsncmp_1(wchar_t *wcs0, const wchar_t *wcs1) { + return wcsncmp(wcs0, wcs1, (wcslen(wcs0) + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcsncmp(wcs0, wcs1, (wcslen(wcs0))); +} + +int bad_wcsncmp_2(wchar_t *wcs2, const wchar_t *wcs3) { + return wcsncmp(wcs2, wcs3, 1 + wcslen(wcs2)); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcsncmp(wcs2, wcs3, wcslen(wcs2)); +} + +int good_wcsncmp_1_2(wchar_t *wcs4, const wchar_t *wcs5) { + return wcsncmp(wcs4, wcs5, wcslen(wcs4)); +} + +int bad_wcsncmp_3(wchar_t *wcs6) { + return wcsncmp(wcs6, L"string", 7); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: comparison length is too long and might lead to a buffer overflow [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcsncmp(wcs6, L"string", 6); +} + +int good_wcsncmp_3(wchar_t *wcs7) { + return wcsncmp(wcs7, L"string", 6); +} + +void bad_wcsxfrm_1(const wchar_t *long_source_name) { + wchar_t long_destination_array_name[13]; + wcsxfrm(long_destination_array_name, long_source_name, + wcslen(long_source_name)); + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: the result from calling 'wcsxfrm' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t long_destination_array_name[14]; + // CHECK-FIXES-NEXT: wcsxfrm(long_destination_array_name, long_source_name, + // CHECK-FIXES-NEXT: wcslen(long_source_name) + 1); +} + +void good_wcsxfrm_1(const wchar_t *long_source_name) { + wchar_t long_destination_array_name[14]; + wcsxfrm(long_destination_array_name, long_source_name, + wcslen(long_source_name) + 1); +} + +void bad_wcsxfrm_2() { + wchar_t long_destination_array_name1[16]; + wcsxfrm(long_destination_array_name1, L"long_source_name", 16); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wcsxfrm' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t long_destination_array_name1[17]; + // CHECK-FIXES: wcsxfrm(long_destination_array_name1, L"long_source_name", 17); +} + +void good_wcsxfrm_2() { + wchar_t long_destination_array_name2[17]; + wcsxfrm(long_destination_array_name2, L"long_source_name", 17); +} Index: clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp @@ -0,0 +1,111 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c++11 -I %S/Inputs/bugprone-not-null-terminated-result + +#include "not-null-terminated-result-cxx.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +//===----------------------------------------------------------------------===// +// wmemcpy() - destination array tests +//===----------------------------------------------------------------------===// + +void bad_wmemcpy_known_dest(const wchar_t *src) { + wchar_t dest01[13]; + wmemcpy(dest01, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t dest01[14]; + // CHECK-FIXES-NEXT: wcscpy_s(dest01, src); +} + +void good_wmemcpy_known_dest(const wchar_t *src) { + wchar_t dst01[14]; + wcscpy_s(dst01, src); +} + +//===----------------------------------------------------------------------===// +// wmemcpy() - length tests +//===----------------------------------------------------------------------===// + +void bad_wmemcpy_full_source_length(const wchar_t *src) { + wchar_t dest20[13]; + wmemcpy(dest20, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t dest20[14]; + // CHECK-FIXES-NEXT: wcscpy_s(dest20, src); +} + +void good_wmemcpy_full_source_length(const wchar_t *src) { + wchar_t dst20[14]; + wcscpy_s(dst20, src); +} + +void bad_wmemcpy_partial_source_length(const wchar_t *src) { + wchar_t dest21[13]; + wmemcpy(dest21, src, wcslen(src) - 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t dest21[14]; + // CHECK-FIXES-NEXT: wcsncpy_s(dest21, src, wcslen(src) - 1); +} + +void good_wmemcpy_partial_source_length(const wchar_t *src) { + wchar_t dst21[14]; + wcsncpy_s(dst21, src, wcslen(src) - 1); +} + +//===----------------------------------------------------------------------===// +// wmemcpy_s() - destination array tests +//===----------------------------------------------------------------------===// + +void bad_wmemcpy_s_unknown_dest(wchar_t *dest40, const wchar_t *src) { + wmemcpy_s(dest40, 13, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcscpy_s(dest40, 13, src); +} + +void good_wmemcpy_s_unknown_dest(wchar_t *dst40, const wchar_t *src) { + wcscpy_s(dst40, 13, src); +} + +void bad_wmemcpy_s_known_dest(const wchar_t *src) { + wchar_t dest41[13]; + wmemcpy_s(dest41, 13, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t dest41[14]; + // CHECK-FIXES-NEXT: wcscpy_s(dest41, src); +} + +void good_wmemcpy_s_known_dest(const wchar_t *src) { + wchar_t dst41[13]; + wcscpy_s(dst41, src); +} + +//===----------------------------------------------------------------------===// +// wmemcpy_s() - length tests +//===----------------------------------------------------------------------===// + +void bad_wmemcpy_s_full_source_length(const wchar_t *src) { + wchar_t dest60[13]; + wmemcpy_s(dest60, 13, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t dest60[14]; + // CHECK-FIXES-NEXT: wcscpy_s(dest60, src); +} + +void good_wmemcpy_s_full_source_length(const wchar_t *src) { + wchar_t dst60[13]; + wcscpy_s(dst60, src); +} + +void bad_wmemcpy_s_partial_source_length(const wchar_t *src) { + wchar_t dest61[13]; + wmemcpy_s(dest61, 13, src, wcslen(src) - 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wchar_t dest61[14]; + // CHECK-FIXES-NEXT: wcsncpy_s(dest61, src, wcslen(src) - 1); +} + +void good_wmemcpy_s_partial_source_length(const wchar_t *src) { + wchar_t dst61[13]; + wcsncpy_s(dst61, src, wcslen(src) - 1); +}