Index: clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tidy/bugprone/BugproneTidyModule.cpp @@ -29,6 +29,7 @@ #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" +#include "NotNullTerminatedResultCheck.h" #include "ParentVirtualCallCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" @@ -95,6 +96,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-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tidy/bugprone/CMakeLists.txt +++ clang-tidy/bugprone/CMakeLists.txt @@ -20,6 +20,7 @@ MisplacedWideningCastCheck.cpp MoveForwardingReferenceCheck.cpp MultipleStatementMacroCheck.cpp + NotNullTerminatedResultCheck.cpp ParentVirtualCallCheck.cpp SizeofContainerCheck.cpp SizeofExpressionCheck.cpp Index: clang-tidy/bugprone/NotNullTerminatedResultCheck.h =================================================================== --- /dev/null +++ clang-tidy/bugprone/NotNullTerminatedResultCheck.h @@ -0,0 +1,63 @@ +//===--------------- NotNullTerminatedResultCheck.h - clang-tidy-*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#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 ``strlen`` or ``wcslen`` function is passed as an +/// argument and caused a not null-terminated result. Depending on the case of +/// use, it may insert or remove the increase operation to ensure the result is +/// null-terminated. +/// +/// 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; + +private: + // If non-zero the target environment implements '_s' suffixed memory and + // character handler functions which are safer than older versions + // (e.g. 'memcpy_s()'). + const int AreSafeFunctionsAvailable; + + 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-tidy/bugprone/NotNullTerminatedResultCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp @@ -0,0 +1,937 @@ +//===------------- NotNullTerminatedResultCheck.cpp - clang-tidy-*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "NotNullTerminatedResultCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +static const char *const FuncExprName = "entire-called-function-expr"; +static const char *const CastExprName = "cast-expr"; +static const char *const ProperDestLengthName = "destination-cannot-overflow"; +static const char *const UnknownDestName = "destination-length-is-unknown"; +static const char *const NotJustCharTyName = "unsigned-or-signed-char"; +static const char *const DestArrayTyName = "destination-is-array-type"; +static const char *const DestVarDeclName = "destination-variable-declaration"; +static const char *const SrcVarDeclName = "source-variable-declaration"; +static const char *const UnknownLengthName = "given-length-is-unknown"; +static const char *const LengthOpName = "length-call-has-binary-operator"; +static const char *const StrlenCallName = "simple-strlen-or-wcslen-call"; +static const char *const DestMallocExprName = "destination-malloc-expr"; +static const char *const DestExprName = "destination-decl-ref-expr"; +static const char *const SrcExprName = "source-expression-or-string-literal"; +static const char *const LengthExprName = "given-length-expression"; + +static SourceLocation exprLocEnd(const Expr *E, + const MatchFinder::MatchResult &Result) { + return Lexer::getLocForEndOfToken(E->getLocEnd(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); +} + +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); +} + +static bool isNoStrlen(const MatchFinder::MatchResult &Result) { + return !Result.Nodes.getNodeAs(StrlenCallName); +} + +static bool isNotPartialLength(const MatchFinder::MatchResult &Result) { + if (const auto *BinOp = Result.Nodes.getNodeAs(LengthOpName)) + return BinOp->getOpcode() != BO_Sub && BinOp->getOpcode() != BO_Div; + + return true; +} + +static bool isKnownDest(const MatchFinder::MatchResult &Result) { + return !Result.Nodes.getNodeAs(UnknownDestName); +} + +static bool isDestAndSrcEquals(const MatchFinder::MatchResult &Result) { + return Result.Nodes.getNodeAs(DestVarDeclName) == + Result.Nodes.getNodeAs(SrcVarDeclName); +} + +static const Expr *getDestExpr(const MatchFinder::MatchResult &Result) { + return Result.Nodes.getNodeAs(DestExprName); +} + +static const Expr *getSrcExpr(const MatchFinder::MatchResult &Result) { + return Result.Nodes.getNodeAs(SrcExprName); +} + +static const Expr *getLengthExpr(const MatchFinder::MatchResult &Result) { + return Result.Nodes.getNodeAs(LengthExprName); +} + +static const Expr *getDestCapacityExpr(const MatchFinder::MatchResult &Result) { + if (const auto *DestMalloc = Result.Nodes.getNodeAs(DestMallocExprName)) + return DestMalloc; + + if (const auto DestTy = Result.Nodes.getNodeAs(DestArrayTyName)) + if (const auto *DestVAT = dyn_cast_or_null(DestTy)) + return DestVAT->getSizeExpr(); + + if (const auto *DestVD = Result.Nodes.getNodeAs(DestVarDeclName)) + if (const auto DestTL = DestVD->getTypeSourceInfo()->getTypeLoc()) + if (const auto DestCTL = DestTL.getAs()) + return DestCTL.getSizeExpr(); + + return nullptr; +} + +static int getLength(const Expr *E, const MatchFinder::MatchResult &Result) { + llvm::APSInt Length; + + if (const auto *LengthDRE = dyn_cast_or_null(E)) + if (const auto *LengthVD = dyn_cast_or_null(LengthDRE->getDecl())) + if (!isa(LengthVD)) + if (const auto *LengthInit = LengthVD->getInit()) + if (LengthInit->EvaluateAsInt(Length, *Result.Context)) + return Length.getZExtValue(); + + if (const auto *LengthIL = dyn_cast_or_null(E)) + return LengthIL->getValue().getZExtValue(); + + if (const auto *StrDRE = dyn_cast_or_null(E)) + if (const auto *StrVD = dyn_cast_or_null(StrDRE->getDecl())) + if (const auto *StrInit = StrVD->getInit()) + if (const auto *StrSL = + dyn_cast_or_null(StrInit->IgnoreImpCasts())) + return StrSL->getLength(); + + if (const auto *SrcSL = dyn_cast_or_null(E)) + return SrcSL->getLength(); + + return 0; +} + +static int getDestCapacity(const MatchFinder::MatchResult &Result) { + if (const auto *DestCapacityExpr = getDestCapacityExpr(Result)) + return getLength(DestCapacityExpr, Result); + + return 0; +} + +static int getGivenLength(const MatchFinder::MatchResult &Result) { + const auto *LengthExpr = Result.Nodes.getNodeAs(LengthExprName); + if (const int Length = getLength(LengthExpr, Result)) + return Length; + + if (const auto *Strlen = dyn_cast_or_null(LengthExpr)) + if (Strlen->getNumArgs() > 0) + if (const auto *StrlenArg = Strlen->getArg(0)->IgnoreImpCasts()) + if (const int StrlenArgLength = getLength(StrlenArg, Result)) + return StrlenArgLength; + + return 0; +} + +//===---------------------------------------------------------------------===// +// Rewrite decision helper functions +//===---------------------------------------------------------------------===// + +static bool isGivenLengthEQToSrcLength(const MatchFinder::MatchResult &Result) { + const int GivenLength = getGivenLength(Result); + + // It is the length without the null terminator. + const int SrcLength = getLength(getSrcExpr(Result), Result); + + if (GivenLength != 0 && GivenLength == SrcLength) + return true; + + const auto *SrcExpr = getSrcExpr(Result); + const auto *LengthExpr = getLengthExpr(Result); + StringRef SrcStr = exprToStr(SrcExpr, Result); + StringRef GivenLengthStr = exprToStr(LengthExpr, Result); + + if (SrcStr.contains(".data") && (GivenLengthStr.contains(".size") || + (GivenLengthStr.contains(".length")))) + return true; + + if (const auto *StrlenExpr = Result.Nodes.getNodeAs(LengthExprName)) + if (const auto *StrlenDRE = dyn_cast_or_null( + StrlenExpr->getArg(0)->IgnoreImpCasts())) + return dyn_cast_or_null(StrlenDRE->getDecl()) == + Result.Nodes.getNodeAs(SrcVarDeclName); + + return false; +} + +static bool isDestCapacityOverflows(const MatchFinder::MatchResult &Result) { + if (!isKnownDest(Result)) + return true; + + const auto *DestCapacityExpr = getDestCapacityExpr(Result); + const auto *LengthExpr = getLengthExpr(Result); + const int DestCapacity = getLength(DestCapacityExpr, Result); + const int GivenLength = getGivenLength(Result); + + if (GivenLength != 0 && DestCapacity != 0) + return isGivenLengthEQToSrcLength(Result) && DestCapacity == GivenLength; + + StringRef DestCapacityExprStr = exprToStr(DestCapacityExpr, Result); + StringRef LengthExprStr = exprToStr(LengthExpr, Result); + if (DestCapacityExprStr != "" && DestCapacityExprStr == LengthExprStr) + return true; + + // Assume that it cannot overflow if the destination length contains '+ 1'. + if (Result.Nodes.getNodeAs(ProperDestLengthName)) + return false; + + if (const auto DestTy = Result.Nodes.getNodeAs(DestArrayTyName)) + if (const auto *DestVAT = dyn_cast_or_null(DestTy)) { + StringRef SizeExprStr = exprToStr(DestVAT->getSizeExpr(), Result); + if (SizeExprStr.contains("+1") || SizeExprStr.contains("+ 1")) + return false; + } + + return true; +} + +static bool isProperDestCapacity(const MatchFinder::MatchResult &Result) { + if (Result.Nodes.getNodeAs(ProperDestLengthName)) + return true; + + StringRef DestCapacityExprStr = + exprToStr(getDestCapacityExpr(Result), Result); + StringRef LengthExprStr = exprToStr(getLengthExpr(Result), Result); + return DestCapacityExprStr != "" && LengthExprStr != "" && + DestCapacityExprStr.contains(LengthExprStr); +} + +static bool isMemcpyRewrite(const MatchFinder::MatchResult &Result) { + if (isDestAndSrcEquals(Result)) + return false; + + StringRef GivenLengthStr = exprToStr(getLengthExpr(Result), Result); + if (isNoStrlen(Result) && !GivenLengthStr.contains(".size") && + !GivenLengthStr.contains(".length") && + !isGivenLengthEQToSrcLength(Result)) + return false; + + return true; +} + +//===---------------------------------------------------------------------===// +// Code injection functions +//===---------------------------------------------------------------------===// + +static void lengthIncrease(const Expr *LengthExpr, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + if (!LengthExpr) + return; + + if (const auto *LengthIL = dyn_cast_or_null(LengthExpr)) { + const int NewLength = LengthIL->getValue().getZExtValue() + 1; + const auto NewValueFix = FixItHint::CreateReplacement( + LengthExpr->getSourceRange(), Twine(NewLength).str()); + Diag << NewValueFix; + return; + } + + const bool NeedInnerParen = + dyn_cast_or_null(LengthExpr) && + cast(LengthExpr)->getOpcode() != BO_Add; + + if (NeedInnerParen) { + const auto InsertFirstParenFix = + FixItHint::CreateInsertion(LengthExpr->getLocStart(), "("); + const auto InsertPlusOneAndSecondParenFix = + FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), ") + 1"); + Diag << InsertFirstParenFix << InsertPlusOneAndSecondParenFix; + } else { + const auto InsertPlusOne = + FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), " + 1"); + Diag << InsertPlusOne; + } +} + +static bool destCapacityFix(const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const bool IsOverflows = isDestCapacityOverflows(Result); + if (IsOverflows) + lengthIncrease(getDestCapacityExpr(Result), Result, Diag); + + return IsOverflows; +} + +static void lengthArgInsertIncrease(int ArgPos, StringRef Name, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + const auto *LengthExpr = + FuncExpr->getArg(ArgPos)->IgnoreParenCasts()->IgnoreImpCasts(); + lengthIncrease(LengthExpr, Result, Diag); +} + +static void lengthArgRemoveInc(int ArgPos, StringRef Name, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + const auto *LengthExpr = FuncExpr->getArg(ArgPos)->IgnoreImpCasts(); + + if (const auto *LengthIL = dyn_cast_or_null(LengthExpr)) { + const int NewLength = LengthIL->getValue().getZExtValue() - 1; + + const auto NewLengthFix = FixItHint::CreateReplacement( + LengthIL->getSourceRange(), Twine(NewLength).str()); + Diag << NewLengthFix; + + return; + } + + // This is the following structure: ((strlen(src) * 2) + 1) + // InnerOpExpr: ~~~~~~~~~~~~^~~ + // OuterOpExpr: ~~~~~~~~~~~~~~~~~~^~~ + if (const auto *OuterOpExpr = + dyn_cast_or_null(LengthExpr->IgnoreParenCasts())) { + const auto *LHSExpr = OuterOpExpr->getLHS(); + const auto *RHSExpr = OuterOpExpr->getRHS(); + const auto *InnerOpExpr = + isa(RHSExpr->IgnoreCasts()) ? LHSExpr : RHSExpr; + + // This is the following structure: ((strlen(src) * 2) + 1) + // LHSRemoveRange: ~~ + // RHSRemoveRange: ~~~~~~ + const auto LHSRemoveRange = + SourceRange(LengthExpr->getLocStart(), + InnerOpExpr->getLocStart().getLocWithOffset(-1)); + const auto RHSRemoveRange = + SourceRange(exprLocEnd(InnerOpExpr, Result), LengthExpr->getLocEnd()); + const auto LHSRemoveFix = FixItHint::CreateRemoval(LHSRemoveRange); + const auto RHSRemoveFix = FixItHint::CreateRemoval(RHSRemoveRange); + + if (LengthExpr->getLocStart() == InnerOpExpr->getLocStart()) + Diag << RHSRemoveFix; + else if (LengthExpr->getLocEnd() == InnerOpExpr->getLocEnd()) + Diag << LHSRemoveFix; + else + Diag << LHSRemoveFix << RHSRemoveFix; + } else { + const auto InsertDecreaseFix = + FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), " - 1"); + Diag << InsertDecreaseFix; + } +} + +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 *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + const auto ArgToRemove = FuncExpr->getArg(ArgPos); + const auto LHSArg = FuncExpr->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 *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + const int FuncNameLength = + FuncExpr->getDirectCallee()->getIdentifier()->getLength(); + const auto FuncNameRange = + SourceRange(FuncExpr->getLocStart(), + FuncExpr->getLocStart().getLocWithOffset(FuncNameLength - 1)); + + const auto FuncNameFix = + FixItHint::CreateReplacement(FuncNameRange, NewFuncName); + Diag << FuncNameFix; +} + +static void insertDestCapacityArg(bool IsOverflows, StringRef Name, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + SmallString<64> NewSecondArg; + + if (const int DestLength = getDestCapacity(Result)) { + NewSecondArg = Twine(IsOverflows ? DestLength + 1 : DestLength).str(); + } else { + NewSecondArg = exprToStr(getDestCapacityExpr(Result), Result); + NewSecondArg += IsOverflows ? " + 1" : ""; + } + + NewSecondArg += ", "; + const auto InsertNewArgFix = FixItHint::CreateInsertion( + FuncExpr->getArg(1)->getLocStart(), NewSecondArg); + Diag << InsertNewArgFix; +} + +static void insertNullTerminatorExpr(StringRef Name, + const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + const int FuncLocStartColumn = + Result.SourceManager->getPresumedColumnNumber(FuncExpr->getLocStart()); + const auto SpaceRange = SourceRange( + FuncExpr->getLocStart().getLocWithOffset(-FuncLocStartColumn + 1), + FuncExpr->getLocStart()); + StringRef SpaceBeforeStmtStr = Lexer::getSourceText( + CharSourceRange::getCharRange(SpaceRange), *Result.SourceManager, + Result.Context->getLangOpts(), 0); + + SmallString<128> NewAddNullTermExprStr; + NewAddNullTermExprStr = "\n"; + NewAddNullTermExprStr += SpaceBeforeStmtStr; + NewAddNullTermExprStr += exprToStr(getDestExpr(Result), Result); + NewAddNullTermExprStr += "["; + NewAddNullTermExprStr += exprToStr(getLengthExpr(Result), Result); + NewAddNullTermExprStr += "] = "; + NewAddNullTermExprStr += (Name[0] != 'w') ? "\'\\0\';" : "L\'\\0\';"; + + const auto AddNullTerminatorExprFix = FixItHint::CreateInsertion( + exprLocEnd(FuncExpr, Result).getLocWithOffset(1), NewAddNullTermExprStr); + Diag << AddNullTerminatorExprFix; +} + +NotNullTerminatedResultCheck::NotNullTerminatedResultCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AreSafeFunctionsAvailable(Options.get("AreSafeFunctionsAvailable", 1)) {} + +void NotNullTerminatedResultCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AreSafeFunctionsAvailable", AreSafeFunctionsAvailable); +} + +void NotNullTerminatedResultCheck::registerMatchers(MatchFinder *Finder) { + const auto StrlenCallWithoutInc = + callExpr(callee(functionDecl(hasAnyName("::strlen", "::wcslen")))) + .bind(StrlenCallName); + + const auto HasIntOperand = + hasEitherOperand(ignoringParenImpCasts(integerLiteral())); + + const auto HasIncOp = binaryOperator(hasOperatorName("+"), HasIntOperand); + + const auto CharTy = + anyOf(asString("char"), asString("wchar_t"), + allOf(anyOf(asString("unsigned char"), asString("signed char")), + type().bind(NotJustCharTyName))); + + const auto CharTyArray = hasType(qualType(hasCanonicalType( + arrayType(hasElementType(CharTy)).bind(DestArrayTyName)))); + + const auto CharTyPointer = + hasType(qualType(hasCanonicalType(pointerType(pointee(CharTy))))); + + const auto AnyOfCharTy = anyOf(CharTyArray, CharTyPointer); + + const auto NullTerminatorExpr = binaryOperator( + hasLHS(hasDescendant( + declRefExpr(to(varDecl(equalsBoundNode(DestVarDeclName)))))), + hasRHS(ignoringImpCasts( + anyOf(characterLiteral(equals(static_cast(0))), + integerLiteral(equals(0)))))); + + //===--------------------------------------------------------------------===// + // The following seven case match 'strlen' or 'wcslen' function calls + // where no increase by one operation found. + //===--------------------------------------------------------------------===// + + // - Example: (strlen(src) * 2) + const auto ComplexCallWithoutInc = + binaryOperator(unless(hasOperatorName("+")), + hasEitherOperand(ignoringImpCasts(StrlenCallWithoutInc))) + .bind(LengthOpName); + + // - Example: (strlen(dest) + strlen(src)) + const auto ComplexTwoCallWithoutInc = + binaryOperator(hasOperatorName("+"), unless(HasIntOperand), + hasLHS(ignoringImpCasts(StrlenCallWithoutInc)), + hasRHS(ignoringImpCasts(StrlenCallWithoutInc))); + + const auto AnyOfCallWithoutInc = ignoringImpCasts(anyOf( + StrlenCallWithoutInc, ComplexCallWithoutInc, ComplexTwoCallWithoutInc)); + + // - Example: length = strlen(src); + const auto DREWithoutInc = ignoringImpCasts( + declRefExpr(to(varDecl(hasInitializer(AnyOfCallWithoutInc))))); + + // - Example: malloc(length * 2); + const auto OpHasDREWithoutInc = binaryOperator( + unless(hasOperatorName("+")), hasEitherOperand(DREWithoutInc)); + + const auto AnyOfCallOrDREWithoutInc = + anyOf(OpHasDREWithoutInc, DREWithoutInc, AnyOfCallWithoutInc); + + // - Example: int getLength(const char *str) { return strlen(str); } + const auto CallExprReturnWithoutInc = + ignoringImpCasts(callExpr(callee(functionDecl(hasBody( + has(returnStmt(hasReturnValue(AnyOfCallOrDREWithoutInc)))))))); + + // - Example: malloc(getLength(str) * 2); + const auto CallExprOpHasReturnWithoutInc = binaryOperator( + unless(hasOperatorName("+")), hasEitherOperand(CallExprReturnWithoutInc)); + + const auto AnyOfCallExprReturnWithoutInc = + anyOf(CallExprReturnWithoutInc, CallExprOpHasReturnWithoutInc); + + // - Example: int length = getLength(src); + const auto DREHasReturnWithoutInc = ignoringImpCasts( + declRefExpr(to(varDecl(hasInitializer(AnyOfCallExprReturnWithoutInc))))); + + const auto LengthWithoutInc = + anyOf(AnyOfCallOrDREWithoutInc, AnyOfCallExprReturnWithoutInc, + DREHasReturnWithoutInc); + + //===--------------------------------------------------------------------===// + // The following five case match 'strlen' or 'wcslen' function calls + // where increase by one operation found. + //===--------------------------------------------------------------------===// + + // - Example: (strlen(src) + 1) + const auto SimpleCallWithInc = allOf( + HasIncOp, + binaryOperator(hasEitherOperand(ignoringImpCasts(StrlenCallWithoutInc)))); + + // - Example: ((strlen(src) / 2) + 1) + const auto ComplexCallWithInc = + allOf(HasIncOp, + binaryOperator(has(binaryOperator( + unless(hasOperatorName("+")), + hasEitherOperand(ignoringImpCasts(StrlenCallWithoutInc)))))); + + // - Example: int length = strlen(src); memcpy(dest, src, length + 1); + const auto OpHasDREWithInc = + binaryOperator(hasEitherOperand(ignoringImpCasts(DREWithoutInc))); + + const auto AnyOfCallWithInc = anyOf(SimpleCallWithInc, ComplexCallWithInc); + + const auto AnyOfCallOrDREWithInc = ignoringImpCasts( + anyOf(OpHasDREWithInc, + declRefExpr(to( + varDecl(hasInitializer(ignoringImpCasts(AnyOfCallWithInc))))), + AnyOfCallWithInc)); + + // - Example: int getLength(const char *str) { return strlen(str) + 1; } + const auto CallExprReturnWithInc = + ignoringImpCasts(callExpr(callee(functionDecl( + hasBody(has(returnStmt(hasReturnValue(AnyOfCallOrDREWithInc)))))))); + + // - Example: int length = getLength(src); + const auto DREHasReturnWithInc = ignoringImpCasts( + declRefExpr(to(varDecl(hasInitializer(CallExprReturnWithInc))))); + + const auto LengthWithInc = + anyOf(AnyOfCallOrDREWithInc, CallExprReturnWithInc, DREHasReturnWithInc); + + //===--------------------------------------------------------------------===// + // The following six case match the 'destination' array length's + // expression which is used in memcpy() and memmove() matchers. + //===--------------------------------------------------------------------===// + + // - Example: (strlen(src) + IntegerLiteral) + const auto ProperLengthExpr = HasIncOp.bind(ProperDestLengthName); + + const auto MallocLengthExpr = hasArgument( + 0, anyOf(allOf(ProperLengthExpr, expr().bind(DestMallocExprName)), + expr().bind(DestMallocExprName))); + + // - Example: (char *)malloc(length); + const auto DestMalloc = anyOf(castExpr(has(callExpr(MallocLengthExpr))), + callExpr(MallocLengthExpr)); + + // - Example: new char[length]; + const auto DestCXXNewExpr = ignoringImpCasts( + cxxNewExpr(hasArraySize(expr().bind(DestMallocExprName)))); + + const auto AnyOfDestInit = anyOf(DestMalloc, DestCXXNewExpr); + + // - Example: char dest[13]; or char dest[length]; + const auto DestArrayTyDecl = + declRefExpr( + to(anyOf( + varDecl(CharTyArray).bind(DestVarDeclName), + varDecl(hasInitializer(AnyOfDestInit)).bind(DestVarDeclName)))) + .bind(DestExprName); + + const auto DestArrayTyDeclMayInBinOp = + anyOf(DestArrayTyDecl, hasDescendant(DestArrayTyDecl)); + + // - Example: foo[bar[baz]].qux; (or just ParmVarDecl) + const auto DestUnknownDecl = + declRefExpr(allOf(to(varDecl(AnyOfCharTy).bind(DestVarDeclName)), + expr().bind(UnknownDestName))) + .bind(DestExprName); + + const auto DestUnknownDeclMayInBinOp = + anyOf(DestUnknownDecl, hasDescendant(DestUnknownDecl)); + + // - Example: char *dest; dest = (char *)malloc(length); + const auto HasArgZeroDestDefinitionBinOp = allOf( + hasArgument(0, + declRefExpr(to(varDecl(AnyOfCharTy).bind(DestVarDeclName)))), + hasAncestor(compoundStmt(hasDescendant(binaryOperator( + hasLHS(declRefExpr(to(varDecl(equalsBoundNode(DestVarDeclName)))) + .bind(DestExprName)), + hasRHS(AnyOfDestInit)))))); + + //===--------------------------------------------------------------------===// + // The following match the 'destination', 'source' and 'length' + // expressions in memcpy() and memmove() calls. + //===--------------------------------------------------------------------===// + + const auto HasArgZeroAnyOfDestDecl = allOf( + anyOf(hasArgument(0, DestArrayTyDeclMayInBinOp), + HasArgZeroDestDefinitionBinOp, + hasArgument(0, DestUnknownDeclMayInBinOp)), + unless(hasAncestor(compoundStmt(hasDescendant(NullTerminatorExpr))))); + + const auto SrcDecl = declRefExpr( + allOf(to(decl().bind(SrcVarDeclName)), + anyOf(hasAncestor(cxxMemberCallExpr().bind(SrcExprName)), + expr().bind(SrcExprName)))); + + const auto SrcDeclMayInBinOp = anyOf(SrcDecl, hasDescendant(SrcDecl)); + + const auto AnyOfSrcDecl = ignoringImpCasts( + anyOf(stringLiteral().bind(SrcExprName), SrcDeclMayInBinOp)); + + // - Example: char src[] = "foo"; sizeof(src); + const auto SizeOfCharExpr = + unaryExprOrTypeTraitExpr(has(expr(hasType(qualType( + hasCanonicalType(anyOf(arrayType(hasElementType(isAnyCharacter())), + pointerType(pointee(isAnyCharacter()))))))))); + + const auto AnyOfLengthExpr = ignoringImpCasts( + allOf(unless(SizeOfCharExpr), unless(hasDescendant(SizeOfCharExpr)), + anyOf(integerLiteral(), ProperLengthExpr, LengthWithoutInc, + expr().bind(UnknownLengthName)), + expr().bind(LengthExprName))); + + //===--------------------------------------------------------------------===// + // The following two + eleven case match problematic function calls. + //===--------------------------------------------------------------------===// + + const auto MemcpyAndMemmoveMatcher = + allOf(callee(functionDecl(hasAnyName("::memcpy", "::wmemcpy", "::memmove", + "::wmemmove"))), + HasArgZeroAnyOfDestDecl, hasArgument(1, AnyOfSrcDecl), + hasArgument(2, AnyOfLengthExpr)); + + const auto Memcpy_sAndMemmove_sMatcher = + allOf(callee(functionDecl(hasAnyName("::memcpy_s", "::wmemcpy_s", + "::memmove_s", "::wmemmove_s"))), + HasArgZeroAnyOfDestDecl, hasArgument(2, AnyOfSrcDecl), + hasArgument(3, AnyOfLengthExpr)); + + enum class StrlenKind { WithInc, WithoutInc }; + enum class SourceArgExistsKind { Exists, NotExists }; + + const auto Matcher = [=](StringRef Name, int ArgCount, int SourcePos, + int LengthPos, StrlenKind LengthKind, + SourceArgExistsKind SrcKind) { + return allOf( + callee(functionDecl(hasName(Name))), argumentCountIs(ArgCount), + anyOf(HasArgZeroAnyOfDestDecl, anything()), + hasArgument(SourcePos, (SrcKind == SourceArgExistsKind::Exists) + ? AnyOfSrcDecl + : anything()), + hasArgument(LengthPos, + (SrcKind == SourceArgExistsKind::Exists) + ? ignoringImpCasts(allOf( + anyOf(integerLiteral(), + (LengthKind == StrlenKind::WithoutInc) + ? ignoringImpCasts(LengthWithoutInc) + : ignoringImpCasts(LengthWithInc)), + expr().bind(LengthExprName))) + : (LengthKind == StrlenKind::WithoutInc) + ? ignoringImpCasts(LengthWithoutInc) + : ignoringImpCasts(LengthWithInc))); + }; + + const auto Memchr = Matcher("::memchr", 3, 0, 2, StrlenKind::WithoutInc, + SourceArgExistsKind::Exists); + const auto Wmemchr = Matcher("::wmemchr", 3, 0, 2, StrlenKind::WithoutInc, + SourceArgExistsKind::Exists); + const auto Memset = Matcher("::memset", 3, 0, 2, StrlenKind::WithInc, + SourceArgExistsKind::NotExists); + const auto Wmemset = Matcher("::wmemset", 3, 0, 2, StrlenKind::WithInc, + SourceArgExistsKind::NotExists); + const auto Strerror_s = + Matcher("::strerror_s", 3, 0, 1, StrlenKind::WithoutInc, + SourceArgExistsKind::NotExists); + const auto StrncmpLHS = Matcher("::strncmp", 3, 0, 2, StrlenKind::WithInc, + SourceArgExistsKind::Exists); + const auto WcsncmpLHS = Matcher("::wcsncmp", 3, 0, 2, StrlenKind::WithInc, + SourceArgExistsKind::Exists); + const auto StrncmpRHS = Matcher("::strncmp", 3, 1, 2, StrlenKind::WithInc, + SourceArgExistsKind::Exists); + const auto WcsncmpRHS = Matcher("::wcsncmp", 3, 1, 2, StrlenKind::WithInc, + SourceArgExistsKind::Exists); + const auto Strxfrm = Matcher("::strxfrm", 3, 1, 2, StrlenKind::WithoutInc, + SourceArgExistsKind::Exists); + const auto Wcsxfrm = Matcher("::wcsxfrm", 3, 1, 2, StrlenKind::WithoutInc, + SourceArgExistsKind::Exists); + + const auto AnyOfMatchers = + anyOf(MemcpyAndMemmoveMatcher, Memcpy_sAndMemmove_sMatcher, Memchr, + Wmemchr, Memset, Wmemset, Strerror_s, StrncmpLHS, WcsncmpLHS, + StrncmpRHS, WcsncmpRHS, Strxfrm, Wcsxfrm); + + Finder->addMatcher(callExpr(AnyOfMatchers).bind(FuncExprName), this); + + Finder->addMatcher( + castExpr(has(callExpr(anyOf(Memchr, Wmemchr)).bind(FuncExprName))) + .bind(CastExprName), + this); +} + +void NotNullTerminatedResultCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + + if (FuncExpr->getLocStart().isMacroID()) + return; + + StringRef Name = FuncExpr->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 (!Name.contains("cpy") && !isGivenLengthEQToSrcLength(Result) && + isNoStrlen(Result)) + return; + + if (Name.endswith("chr")) { + memchrFix(Name, Result); + return; + } + + if (Name.contains("cpy") && !isMemcpyRewrite(Result)) + return; + + if (Name.contains("move") && isDestAndSrcEquals(Result)) + return; + + auto Diag = + diag(Result.Nodes.getNodeAs(FuncExprName)->getLocStart(), + "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")) { + destCapacityFix(Result, Diag); + lengthArgInsertIncrease(3, Name, Result, Diag); + } else if (Name.endswith("set")) { + lengthArgRemoveInc(2, Name, Result, Diag); + } +} + +void NotNullTerminatedResultCheck::memcpyFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const bool IsOverflows = destCapacityFix(Result, Diag); + + // If 'memcpy()' cannot be rewritten to any string handler function + if (Result.Nodes.getNodeAs(NotJustCharTyName)) { + + // If it is could be 'memcpy_s()' + if (AreSafeFunctionsAvailable && isKnownDest(Result)) { + renameFunc("memcpy_s", Result, Diag); + insertDestCapacityArg(IsOverflows, Name, Result, Diag); + } + + lengthArgInsertIncrease(2, Name, Result, Diag); + + return; + } + + const bool IsCpy = isGivenLengthEQToSrcLength(Result) || + (!isNoStrlen(Result) && isNotPartialLength(Result)); + const bool IsSafe = AreSafeFunctionsAvailable && IsOverflows && + isKnownDest(Result) && !isProperDestCapacity(Result); + const bool IsDestLengthNotRequired = + IsSafe && getLangOpts().CPlusPlus && + Result.Nodes.getNodeAs(DestArrayTyName); + + SmallString<10> NewFuncName; + NewFuncName = (Name[0] != 'w') ? "str" : "wcs"; + NewFuncName += IsCpy ? "cpy" : "ncpy"; + NewFuncName += IsSafe ? "_s" : ""; + renameFunc(NewFuncName, Result, Diag); + + if (IsSafe && !IsDestLengthNotRequired) + insertDestCapacityArg(IsOverflows, Name, Result, Diag); + + if (IsCpy) + removeArg(2, Result, Diag); + + if (!IsCpy && !IsSafe) + insertNullTerminatorExpr(Name, Result, Diag); +} + +void NotNullTerminatedResultCheck::memcpy_sFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const bool IsOverflows = destCapacityFix(Result, Diag); + + if (Result.Nodes.getNodeAs(NotJustCharTyName)) { + lengthArgInsertIncrease(3, Name, Result, Diag); + return; + } + + const bool RemoveDestLength = + getLangOpts().CPlusPlus && + Result.Nodes.getNodeAs(DestArrayTyName); + const bool IsCpy = !isNoStrlen(Result) && isNotPartialLength(Result); + const bool IsSafe = IsOverflows; + + SmallString<10> NewFuncName; + NewFuncName = (Name[0] != 'w') ? "str" : "wcs"; + NewFuncName += IsCpy ? "cpy" : "ncpy"; + NewFuncName += IsSafe ? "_s" : ""; + renameFunc(NewFuncName, Result, Diag); + + if (!IsSafe || (IsSafe && RemoveDestLength)) + removeArg(1, Result, Diag); + else if (IsOverflows && isKnownDest(Result)) + lengthArgInsertIncrease(1, Name, Result, Diag); + + if (IsCpy) + removeArg(3, Result, Diag); + + if (!IsCpy && !IsSafe) + insertNullTerminatorExpr(Name, Result, Diag); +} + +void NotNullTerminatedResultCheck::memchrFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + const auto *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + if (const auto GivenCL = + dyn_cast_or_null(FuncExpr->getArg(1))) + if (GivenCL->getValue() != 0) + return; + + auto Diag = diag(FuncExpr->getArg(2)->IgnoreParenCasts()->getLocStart(), + "the length is too short for the last %0") + << ((Name[0] != 'w') ? "\'\\0\'" : "L\'\\0\'"); + + if (const auto *CastExpr = Result.Nodes.getNodeAs(CastExprName)) { + const auto CastRemoveFix = FixItHint::CreateRemoval(SourceRange( + CastExpr->getLocStart(), FuncExpr->getLocStart().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) { + const bool IsOverflows = destCapacityFix(Result, Diag); + + if (AreSafeFunctionsAvailable && isKnownDest(Result)) { + renameFunc((Name[0] != 'w') ? "memmove_s" : "wmemmove_s", Result, Diag); + insertDestCapacityArg(IsOverflows, Name, Result, Diag); + } + + lengthArgInsertIncrease(2, Name, Result, Diag); +} + +void NotNullTerminatedResultCheck::strerror_sFix( + const MatchFinder::MatchResult &Result) { + StringRef Name = "strerror_s"; + auto Diag = + diag(Result.Nodes.getNodeAs(FuncExprName)->getLocStart(), + "the result from calling '%0' is not null-terminated and " + "missing the last character of the error message") + << Name; + + destCapacityFix(Result, Diag); + lengthArgInsertIncrease(1, Name, Result, Diag); +} + +void NotNullTerminatedResultCheck::ncmpFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + const auto *FuncExpr = Result.Nodes.getNodeAs(FuncExprName); + const auto *FirstArgExpr = FuncExpr->getArg(0)->IgnoreImpCasts(); + const auto *SecondArgExpr = FuncExpr->getArg(1)->IgnoreImpCasts(); + bool IsLengthTooLong = false; + + if (!isNoStrlen(Result)) { + const auto *LengthExprArgExpr = + Result.Nodes.getNodeAs(StrlenCallName)->getArg(0); + StringRef FirstExprStr = exprToStr(FirstArgExpr, Result); + StringRef SecondExprStr = exprToStr(SecondArgExpr, Result); + StringRef LengthArgStr = exprToStr(LengthExprArgExpr, Result); + IsLengthTooLong = + LengthArgStr == FirstExprStr || LengthArgStr == SecondExprStr; + } else { + const int FirstExprLength = getLength(FirstArgExpr, Result); + const int SecondExprLength = getLength(SecondArgExpr, Result); + const int GivenLength = + getLength(FuncExpr->getArg(2)->IgnoreImpCasts(), Result); + IsLengthTooLong = GivenLength - 1 == FirstExprLength || + GivenLength - 1 == SecondExprLength; + } + + if (!IsLengthTooLong) + return; + + auto Diag = diag(FuncExpr->getArg(2)->IgnoreParenCasts()->getLocStart(), + "comparison length is too long and might lead to a " + "buffer overflow"); + + lengthArgRemoveInc(2, "ncmp", Result, Diag); +} + +void NotNullTerminatedResultCheck::xfrmFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + if (!isDestCapacityOverflows(Result)) + return; + + auto Diag = + diag(Result.Nodes.getNodeAs(FuncExprName)->getLocStart(), + "the result from calling '%0' is not null-terminated") + << Name; + + destCapacityFix(Result, Diag); + lengthArgInsertIncrease(2, Name, Result, Diag); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -73,6 +73,15 @@ Diagnoses comparisons that appear to be incorrectly placed in the argument to the ``TEMP_FAILURE_RETRY`` macro. +- 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 an undefined behaviour when the + string is read. + - New :doc:`bugprone-parent-virtual-call ` check. Index: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst =================================================================== --- /dev/null +++ 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 an undefined behaviour when the string is read. + +The following function calls are checked: + +``memcpy``, ``wmemcpy``, ``memcpy_s``, ``wmemcpy_s``, ``memchr``, ``wmemchr``, +``memmove``, ``wmemmove``, ``memmove_s``, ``wmemmove_s``, ``memset``, +``wmemset``, ``strerror_s``, ``strncmp``, ``wcsncmp``, ``strxfrm``, ``wcsxfrm`` + +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 problematic too. + + .. 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. If it +is neccesary, the buffer size will be increased to hold the null terminator. + + .. code-block:: c + + static char *StringCpy(const std::string &str) { + char *result = reinterpret_cast(malloc(str.size() + 1)); + strcpy(result, str.data()); + return result; + } + +.. _MemcpyTransformation: + +Transformation rules with 'memcpy()' +------------------------------------ + +It is possible to rewrite the ``memcpy()`` and ``memcpy_s()`` calls as the +following four function: ``strcpy()``, ``strncpy()``, ``strcpy_s()``, +``strncpy_s()``, where the latter two is the safe version. Analogly it is +possible to rewrite ``wmemcpy()`` related functions handled in the same way. + +Rewrite to a string handler function is not possible: + +- If the type of the destination array is not just ``char``, that means it + cannot be any string handler function. But if the given length is + ``strlen(source)`` then fix-it adds ``+ 1`` to it, so the result will be + null-terminated. + +Rewrite based on the destination array: + +- If the destination cannot overflow then the new function is should be the + simple ``cpy()``, because it is more efficient. + +- If the destination can overflow and ``AreSafeFunctionsAvailable = 1`` and it + is possible to read the length of the destination array then the new function + could be safe (``cpy_s()``). + +- If the new function is could be safe and the target environment is C++ then + it is not neccesary to pass the length of the destination array. + +and based on the length of the source string: + +- If the given length is ``strlen(source)`` (or equals length) then the new + function is should be the simple ``cpy()``, because it is more efficient than + the safe version. + +- Otherwise we assume that the programmer wanted to copy `n` characters, so the + new function is ``ncpy()``. + +Transformations with 'strlen()' length +-------------------------------------- + +In general, the following transformations are could happen: + +(Note: If a wide-character handler function exists of the following functions +it handled in the same way.) + +Memory handler functions +^^^^^^^^^^^^^^^^^^^^^^^^ + +- ``memcpy``: See in the + :ref:`Transformation rules with 'memcpy()'` section. + +- ``memchr``: + - Usually there is a C-style cast, and it is needed to be removed, because the + new function ``strchr``/``wcschr``'s return type is correct. + - Also the third argument is not needed. + +- ``memmove``: + - If ``_s`` functions are available: New function is + ``memmove_s``/``wmemmove_s``, it has four arguments, + - the new second argument is the first argument's length, and + - the third argument will be moved as the fourth, where ``+ 1`` needed. + + - Else: The third argument gets a ``+ 1`` operation. + +- ``memmove_s function's fourth argument gets a ``+ 1`` operation. + +- ``memset`` function's third argument has to be truncated without the ``+ 1``. + +String handler functions +^^^^^^^^^^^^^^^^^^^^^^^^ + +- ``strerror_s`` functions's second argument get a ``+ 1`` operation. + +- ``strncmp``: + - If the third argument is the first or the second argument's ``length + 1``, + then it has to be truncated without the ``+ 1`` operation. + +- ``strxfrm`` function's third argument gets a ``+ 1`` operation. + +Options +------- + +.. option:: AreSafeFunctionsAvailable + + An integer non-zero value specifying if the target environment implements + ``_s`` suffixed memory and character handler functions which are safer than + older versions (e.g. ``memcpy_s()``). The default value is ``1``. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -37,6 +37,7 @@ bugprone-misplaced-widening-cast bugprone-move-forwarding-reference bugprone-multiple-statement-macro + bugprone-not-null-terminated-result bugprone-parent-virtual-call bugprone-sizeof-container bugprone-sizeof-expression Index: test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c @@ -0,0 +1,123 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 + +typedef unsigned int size_t; +typedef int errno_t; +size_t strlen(const char *); +char *strerror(int); +char *strchr(const char *, int); +errno_t *strncpy_s(char *, const char *, size_t); +errno_t strerror_s(char *, size_t, int); +int strncmp(const char *, const char *, size_t); +size_t strxfrm(char *, const char *, size_t); + +void *memchr(const void *, int, size_t); +void *memset(void *, int, size_t); + +int getLengthWithInc(const char *str) { + int length = strlen(str) + 1; + return 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 for the last '\0' [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_memset_1(const char *src) { + char dest[13]; + int length = getLengthWithInc(src); + memset(dest, '-', length); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memset(dest, '-', length - 1); +} + +void good_memset1(const char *src) { + char dst[13]; + int length = getLengthWithInc(src); + memset(dst, '-', length - 1); +} + +void bad_memset_2(const char *src) { + char dest[13]; + int length = strlen(src); + memset(dest, '-', length + 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memset(dest, '-', length); +} + +void good_memset_2(const char *src) { + char dst[13]; + int length = strlen(src); + memset(dst, '-', length); +} + +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: test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c @@ -0,0 +1,73 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: [{key: bugprone-not-null-terminated-result.AreSafeFunctionsAvailable, \ +// RUN: value: 0}]}" \ +// RUN: -- -std=c11 + +typedef unsigned int size_t; +typedef int errno_t; +size_t strlen(const char *); +void *malloc(size_t); + +char *strcpy(char *, const char *); +void *memcpy(void *, const void *, size_t); + + +//===----------------------------------------------------------------------===// +// 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: memcpy(dest00, src, strlen(src) + 1); +} + +void good_memcpy_not_just_char_dest(const char *src) { + unsigned char dst00[13]; + memcpy(dst00, src, strlen(src) + 1); +} + +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: test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp @@ -0,0 +1,156 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c++11 + +namespace std { +template +struct basic_string { + basic_string(); + const T *data() const; + unsigned long size() const; +}; +typedef basic_string string; +} +typedef unsigned int size_t; +typedef int errno_t; +size_t strlen(const char *); +void *malloc(size_t); +void *realloc(void *, size_t); + +template +errno_t strncpy_s(char (&dest)[size], const char *src, size_t length); +errno_t strncpy_s(char *, size_t, const char *, size_t); + +template +char *strncpy(char (&dest)[size], const char *src, size_t length); +char *strncpy(char *, const char *, size_t); + +template +errno_t strcpy_s(char (&dest)[size], const char *); +errno_t strcpy_s(char *, size_t, const char *); + +template +char *strcpy(char (&dest)[size], const char *); +char *strcpy(char *, const char *); + +errno_t memcpy_s(void *, size_t, const void *, size_t); +void *memcpy(void *, const void *, size_t); + + +//===----------------------------------------------------------------------===// +// 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: memcpy_s(dest00, 14, src, strlen(src) + 1); +} + +void good_memcpy_not_just_char_dest(const char *src) { + unsigned char dst00[14]; + memcpy_s(dst00, 14, src, strlen(src) + 1); +} + +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: char *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: test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c @@ -0,0 +1,107 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 + +typedef unsigned int size_t; +typedef int errno_t; +size_t strlen(const char *); +void *malloc(size_t); +void *realloc(void *, size_t); + +errno_t strncpy_s(char *, size_t, const char *, size_t); +errno_t strcpy_s(char *, size_t, const char *); +char *strcpy(char *, const char *); + +errno_t memcpy_s(void *, size_t, const void *, size_t); +void *memcpy(void *, const void *, size_t); + +#define SRC_LENGTH 3 +#define SRC "foo" + + +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 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); +} + +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); +} + +// FIXME: Should be 'SRC_LENGTH + 1' instead of the constant value. +void bad_memcpy_macro() { + unsigned char dest05[13]; + 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: memcpy_s(dest05, 13, SRC, 4); +} + +void good_memcpy_macro() { + unsigned char dst05[13]; + memcpy_s(dst05, 13, SRC, SRC_LENGTH + 1); +} + Index: test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c @@ -0,0 +1,134 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 + +typedef unsigned int size_t; +typedef int errno_t; +size_t strlen(const char *); +void *malloc(size_t); +void *realloc(void *, size_t); + +errno_t strncpy_s(char *, size_t, const char *, size_t); +errno_t strcpy_s(char *, size_t, const char *); +char *strcpy(char *, const char *); + +errno_t memcpy_s(void *, size_t, const void *, size_t); +void *memcpy(void *, const void *, size_t); + +//===----------------------------------------------------------------------===// +// 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: memcpy_s(dest00, 14, src, strlen(src) + 1); +} + +void good_memcpy_not_just_char_dest(const char *src) { + unsigned char dst00[14]; + memcpy_s(dst00, 14, src, strlen(src) + 1); +} + +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: test/clang-tidy/bugprone-not-null-terminated-result-strlen.c =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-strlen.c @@ -0,0 +1,143 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c11 + +typedef unsigned int size_t; +typedef int errno_t; +size_t strlen(const char *); +char *strerror(int); + +char *strchr(const char *, int); +errno_t strncpy_s(char *, size_t, const char *, size_t); +errno_t strerror_s(char *, size_t, int); +int strncmp(const char *, const char *, size_t); +size_t strxfrm(char *, const char *, size_t); + +void *memchr(const void *, int, size_t); +void *memmove(void *, const void *, size_t); +errno_t memmove_s(void *, size_t, const void *, size_t); +void *memset(void *, int, size_t); + + +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 for the last '\0' [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 for the last '\0' [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_memset(const char *src) { + char dest[13]; + memset(dest, '-', strlen(src) + 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memset(dest, '-', strlen(src)); +} + +void good_memset(const char *src) { + char dst[13]; + memset(dst, '-', strlen(src)); +} + +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: test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp @@ -0,0 +1,128 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c++11 + +typedef unsigned int size_t; +typedef int errno_t; +size_t wcslen(const wchar_t *); + +wchar_t *wcschr(const wchar_t *, int); +errno_t wcsncpy_s(wchar_t *, size_t, const wchar_t *, size_t); +int wcsncmp(const wchar_t *, const wchar_t *, size_t); +size_t wcsxfrm(wchar_t *, const wchar_t *, size_t); + +void *wmemchr(const void *, int, size_t); +void *wmemmove(void *, const void *, size_t); +errno_t wmemmove_s(void *, size_t, const void *, size_t); +void *wmemset(void *, int, size_t); + + +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 for the last L'\0' [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 for the last L'\0' [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); +} + +void bad_wmemset(const wchar_t *src) { + wchar_t dest[13]; + wmemset(dest, L'-', wcslen(src) + 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemset' is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wmemset(dest, L'-', wcslen(src)); +} + +void good_wmemset(const wchar_t *src) { + wchar_t dst[13]; + wmemset(dst, L'-', wcslen(src)); +} + +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: test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp @@ -0,0 +1,133 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \ +// RUN: -- -std=c++11 + +typedef unsigned int size_t; +typedef int errno_t; +size_t wcslen(const wchar_t *); +void *malloc(size_t); +void *realloc(void *, size_t); + +template +errno_t wcsncpy_s(wchar_t (&dest)[size], const wchar_t *src, size_t length); +errno_t wcsncpy_s(wchar_t *, size_t, const wchar_t *, size_t); + +template +wchar_t *wcsncpy(wchar_t (&dest)[size], const wchar_t *src, size_t length); +wchar_t *wcsncpy(wchar_t *, const wchar_t *, size_t); + +template +errno_t wcscpy_s(wchar_t (&dest)[size], const wchar_t *); +errno_t wcscpy_s(wchar_t *, size_t, const wchar_t *); + +template +wchar_t *wcscpy(wchar_t (&dest)[size], const wchar_t *); +wchar_t *wcscpy(wchar_t *, const wchar_t *); + +errno_t wmemcpy_s(wchar_t *, size_t, const wchar_t *, size_t); +wchar_t *wmemcpy(wchar_t *, const wchar_t *, size_t); + + +//===----------------------------------------------------------------------===// +// 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); +} +