Index: clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tidy/bugprone/BugproneTidyModule.cpp @@ -28,6 +28,7 @@ #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" +#include "NotNullTerminatedResultCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" #include "StringConstructorCheck.h" @@ -89,6 +90,8 @@ "bugprone-move-forwarding-reference"); CheckFactories.registerCheck( "bugprone-multiple-statement-macro"); + CheckFactories.registerCheck( + "bugprone-not-null-terminated-result"); CheckFactories.registerCheck( "bugprone-sizeof-container"); 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 SizeofContainerCheck.cpp SizeofExpressionCheck.cpp StringConstructorCheck.cpp Index: clang-tidy/bugprone/NotNullTerminatedResultCheck.h =================================================================== --- /dev/null +++ clang-tidy/bugprone/NotNullTerminatedResultCheck.h @@ -0,0 +1,75 @@ +//===--------------- 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`` passed as argument and +/// cause a not null-terminated result +/// +/// 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) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void memAllocFuncFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result); + void memHandlerFuncFix(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, + DiagnosticBuilder &Diag); + void memmoveFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result); + void memmove_sFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result); + void characterFuncFix(StringRef Name, + const ast_matchers::MatchFinder::MatchResult &Result); + 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); + void + lengthArgInsertIncByOne(const int ArgPos, + const ast_matchers::MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag); + void + lengthArgRemoveIncByOne(const ast_matchers::MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag); + void removeArg(const int ArgPos, + const ast_matchers::MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag); + void renameFunc(StringRef NewFuncName, + const ast_matchers::MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag); +}; + +} // 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,435 @@ +//===------------- 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 { + +std::string exprToStr(const Expr *Expr, + const MatchFinder::MatchResult &Result) { + return Lexer::getSourceText( + CharSourceRange::getTokenRange(Expr->getSourceRange()), + *Result.SourceManager, Result.Context->getLangOpts(), 0); +} + +SourceLocation exprLocEnd(const Expr *Expr, + const MatchFinder::MatchResult &Result) { + return Lexer::getLocForEndOfToken(Expr->getLocEnd(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); +} + +bool isInsideParentheses(const Expr *Expr, + const MatchFinder::MatchResult &Result) { + auto NewExpr = const_cast(Expr); + ParenExpr *PE = new (Result.Context) + ParenExpr(NewExpr->getLocStart().getLocWithOffset(-1), + NewExpr->getLocEnd().getLocWithOffset(1), NewExpr); + return Expr == PE->getSubExpr(); +} + +/// Sometimes the 'memmove' function is in use to trim the text until it matches +/// to a specific condition. For example a .CSV file's line would be trimmed +/// from ' "data_name", "data", ' to just 'data' by "memmoving" it char-by-char. +bool isTrimFunc(const MatchFinder::MatchResult &Result) { + const auto *FuncExpr = Result.Nodes.getNodeAs("expr"); + const int ArgPos = FuncExpr->getNumArgs() - 2; + + // This is the following structure: (dest, (src + 1), strlen(src)) + // LHSStr: ~~~ + // RHSStr: ~ + // StrLenArgStr: ~~~ + if (const auto *BinOp = dyn_cast( + FuncExpr->getArg(ArgPos)->IgnoreParenCasts())) { + const auto LHSStr = exprToStr(BinOp->getLHS()->IgnoreParens(), Result); + const auto RHSStr = exprToStr(BinOp->getRHS()->IgnoreParens(), Result); + const auto StrLenArgStr = exprToStr( + Result.Nodes.getNodeAs("length-call")->getArg(0), Result); + + return StrLenArgStr == LHSStr || StrLenArgStr == RHSStr; + } + return false; +} + +void NotNullTerminatedResultCheck::registerMatchers(MatchFinder *Finder) { + // The following function's expression binding is necessary because some of + // the functions create double binding on the same function call. + const auto LengthCall = + callExpr(callee(functionDecl(hasAnyName("strlen", "wcslen"))), + expr().bind("length-call")); + + const auto IsNotPlusOperator = unless(hasOperatorName("+")); + + const auto HasIntOperand = + hasEitherOperand(ignoringParenImpCasts(integerLiteral())); + + const auto HasInc = binaryOperator(hasOperatorName("+"), HasIntOperand); + + // The following 3 case match 'strlen' or 'wcslen' function calls where no + // increase by one operation found. + //- Simple case: strlen(src) or wcslen(src) + const auto SimpleWithoutInc = LengthCall.bind("length-wout-inc-simp"); + + //- Complex case: (strlen(src) * 2) or (wcslen(src) * 2) + const auto ComplexWithoutInc = + binaryOperator(IsNotPlusOperator, hasEitherOperand(LengthCall)) + .bind("length-wout-inc-comp"); + + //- Complex case: (strlen(dest) + strlen(src)) or (wcslen(dest) + wcslen(src)) + const auto ComplexTwoWithoutInc = + allOf(binaryOperator(hasOperatorName("+"), unless(HasIntOperand)), + binaryOperator(hasOperatorName("+"), hasLHS(LengthCall), + hasRHS(LengthCall)) + .bind("length-wout-inc-comp")); + + const auto LengthWithoutInc = + anyOf(SimpleWithoutInc, ComplexWithoutInc, ComplexTwoWithoutInc); + + // The following 2 case match 'strlen' or 'wcslen' function calls where + // increase by one operation found. + //- Simple case: (strlen(src) + 1) or (wcslen(src) + 1) + const auto SimpleWithInc = + allOf(HasInc, + binaryOperator(hasEitherOperand(LengthCall.bind("inner-operator"))) + .bind("outer-operator")); + + //- Complex case: ((strlen(src) / 2) + 1) or ((wcslen(src) / 2) + 1) + const auto ComplexWithInc = allOf( + HasInc, binaryOperator(has(binaryOperator(unless(hasOperatorName("+")), + hasEitherOperand(LengthCall)) + .bind("inner-operator"))) + .bind("outer-operator")); + + const auto LengthWithInc = anyOf(SimpleWithInc, ComplexWithInc); + + enum StrLenKind { WithInc, WithoutInc }; + + const auto Matcher = [=](StringRef Name, const int ArgCount, const int ArgPos, + StrLenKind Kind) { + if (Kind == StrLenKind::WithoutInc) { + return allOf(callee(functionDecl(hasName(Name))), + argumentCountIs(ArgCount), + hasArgument(ArgPos, LengthWithoutInc)); + } else { + return allOf(callee(functionDecl(hasName(Name))), + argumentCountIs(ArgCount), + hasArgument(ArgPos, LengthWithInc)); + } + }; + + // clang-format off + const auto Alloca = Matcher("alloca", 1, 0, StrLenKind::WithoutInc); + const auto Calloc = Matcher("calloc", 2, 0, StrLenKind::WithoutInc); + const auto Malloc = Matcher("malloc", 1, 0, StrLenKind::WithoutInc); + const auto Realloc = Matcher("realloc", 2, 1, StrLenKind::WithoutInc); + const auto Memcpy = Matcher("memcpy", 3, 2, StrLenKind::WithoutInc); + const auto Wmemcpy = Matcher("wmemcpy", 3, 2, StrLenKind::WithoutInc); + const auto Memcpy_s = Matcher("memcpy_s", 4, 3, StrLenKind::WithoutInc); + const auto Wmemcpy_s = Matcher("wmemcpy_s", 4, 3, StrLenKind::WithoutInc); + const auto Memchr = Matcher("memchr", 3, 2, StrLenKind::WithoutInc); + const auto Wmemchr = Matcher("wmemchr", 3, 2, StrLenKind::WithoutInc); + const auto Memmove = Matcher("memmove", 3, 2, StrLenKind::WithoutInc); + const auto Wmemmove = Matcher("wmemmove", 3, 2, StrLenKind::WithoutInc); + const auto Memmove_s = Matcher("memmove_s", 4, 3, StrLenKind::WithoutInc); + const auto Wmemmove_s = Matcher("wmemmove_s", 4, 3, StrLenKind::WithoutInc); + const auto Memset = Matcher("memset", 3, 2, StrLenKind::WithInc); + const auto Wmemset = Matcher("wmemset", 3, 2, StrLenKind::WithInc); + const auto Strerror_s = Matcher("strerror_s", 3, 1, StrLenKind::WithoutInc); + const auto Strncmp = Matcher("strncmp", 3, 2, StrLenKind::WithInc); + const auto Wcsncmp = Matcher("wcsncmp", 3, 2, StrLenKind::WithInc); + const auto Strxfrm = Matcher("strxfrm", 3, 2, StrLenKind::WithoutInc); + const auto Wcsxfrm = Matcher("wcsxfrm", 3, 2, StrLenKind::WithoutInc); + // clang-format on + + const auto AnyOfExpressions = anyOf( + Alloca, Calloc, Malloc, Realloc, Memcpy, Wmemcpy, Memcpy_s, Wmemcpy_s, + Memchr, Wmemchr, Memmove, Wmemmove, Memmove_s, Wmemmove_s, Memset, + Wmemset, Strerror_s, Strncmp, Wcsncmp, Strxfrm, Wcsxfrm); + + Finder->addMatcher(callExpr(AnyOfExpressions).bind("expr"), this); + Finder->addMatcher( + binaryOperator( + has(cStyleCastExpr(has(callExpr(anyOf(Memchr, Wmemchr)).bind("expr"))) + .bind("cast"))), + this); +} + +void NotNullTerminatedResultCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *FuncExpr = Result.Nodes.getNodeAs("expr"); + + if (FuncExpr->getLocStart().isMacroID()) + return; + + if (!FuncExpr->getDirectCallee()) + return; + + const auto Name = FuncExpr->getDirectCallee()->getName(); + + if (Name.contains("alloc")) + memAllocFuncFix(Name, Result); + else if (Name.startswith("mem") || Name.startswith("wmem")) + memHandlerFuncFix(Name, Result); + else if (Name.startswith("str") || Name.startswith("wcs")) + characterFuncFix(Name, Result); +} + +void NotNullTerminatedResultCheck::memAllocFuncFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + auto Diag = + diag(Result.Nodes.getNodeAs("expr")->getLocStart(), + "'%0' function's allocated memory cannot hold the null-terminator") + << Name; + + if (Name == "alloca" || Name == "calloc" || Name == "malloc") + lengthArgInsertIncByOne(0, Result, Diag); + else if (Name == "realloc") + lengthArgInsertIncByOne(1, Result, Diag); +} + +void NotNullTerminatedResultCheck::memHandlerFuncFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + if (Name == "memmove" || Name == "wmemmove") + memmoveFix(Name, Result); + else if (Name == "memmove_s" || Name == "wmemmove_s") + memmove_sFix(Name, Result); + else { + auto Diag = diag(Result.Nodes.getNodeAs("expr")->getLocStart(), + "'%0' function's result is not null-terminated") + << Name; + if (Name == "memcpy" || Name == "wmemcpy") + memcpyFix(Name, Result, Diag); + else if (Name == "memcpy_s" || Name == "wmemcpy_s") + memcpy_sFix(Name, Result, Diag); + else if (Name == "memchr" || Name == "wmemchr") + memchrFix(Name, Result, Diag); + else if (Name == "memset" || Name == "wmemset") + lengthArgRemoveIncByOne(Result, Diag); + } +} + +void NotNullTerminatedResultCheck::memcpyFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + if (getLangOpts().CPlusPlus11) { + const auto NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s"; + renameFunc(NewFuncName, Result, Diag); + } else { + const auto NewFuncName = (Name[0] != 'w') ? "strncpy" : "wcsncpy"; + renameFunc(NewFuncName, Result, Diag); + lengthArgInsertIncByOne(2, Result, Diag); + } +} + +void NotNullTerminatedResultCheck::memcpy_sFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s"; + renameFunc(NewFuncName, Result, Diag); + removeArg(1, Result, Diag); +} + +void NotNullTerminatedResultCheck::memchrFix( + StringRef Name, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + if (const auto *CastExpr = Result.Nodes.getNodeAs("cast")) { + const auto *FuncExpr = Result.Nodes.getNodeAs("expr"); + const auto CastRemoveFix = FixItHint::CreateRemoval(SourceRange( + CastExpr->getLocStart(), FuncExpr->getLocStart().getLocWithOffset(-1))); + Diag << CastRemoveFix; + } + const auto NewFuncName = (Name[0] != 'w') ? "strchr" : "wcschr"; + renameFunc(NewFuncName, Result, Diag); + removeArg(2, Result, Diag); +} + +void NotNullTerminatedResultCheck::memmoveFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + if (isTrimFunc(Result)) + return; + + const auto *FuncExpr = Result.Nodes.getNodeAs("expr"); + auto Diag = diag(FuncExpr->getLocStart(), + "'%0' function's result is not null-terminated") + << Name; + + if (getLangOpts().CPlusPlus11) { + const auto NewFuncName = (Name[0] != 'w') ? "memmove_s" : "wmemmove_s"; + renameFunc(NewFuncName, Result, Diag); + + const auto FirstArg = FuncExpr->getArg(0); + std::string NewSecondArg = " strlen(" + exprToStr(FirstArg, Result) + "),"; + + const auto InsertNewArgFix = FixItHint::CreateInsertion( + exprLocEnd(FirstArg, Result).getLocWithOffset(1), NewSecondArg); + Diag << InsertNewArgFix; + } + lengthArgInsertIncByOne(2, Result, Diag); +} + +void NotNullTerminatedResultCheck::memmove_sFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + if (isTrimFunc(Result)) + return; + + auto Diag = diag(Result.Nodes.getNodeAs("expr")->getLocStart(), + "'%0' function's result is not null-terminated") + << Name; + lengthArgInsertIncByOne(3, Result, Diag); +} + +void NotNullTerminatedResultCheck::characterFuncFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + if (Name == "strerror_s") + strerror_sFix(Result); + else if (Name == "strncmp" || Name == "wcsncmp") + ncmpFix(Name, Result); + else if (Name == "strxfrm" || Name == "wcsxfrm") + xfrmFix(Name, Result); +} + +void NotNullTerminatedResultCheck::strerror_sFix( + const MatchFinder::MatchResult &Result) { + auto Diag = diag(Result.Nodes.getNodeAs("expr")->getLocStart(), + "'strerror_s' function's result is not null-terminated and " + "missing the last character of the error message"); + lengthArgInsertIncByOne(1, Result, Diag); +} + +void NotNullTerminatedResultCheck::ncmpFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + const auto *FuncExpr = Result.Nodes.getNodeAs("expr"); + const auto FirstArgStr = exprToStr(FuncExpr->getArg(0), Result); + const auto SecondArgStr = exprToStr(FuncExpr->getArg(1), Result); + const auto StrLenArgStr = exprToStr( + Result.Nodes.getNodeAs("length-call")->getArg(0), Result); + + if (StrLenArgStr == FirstArgStr || StrLenArgStr == SecondArgStr) { + auto Diag = diag(FuncExpr->getLocStart(), + "'%0' function's comparison's length is too long") + << Name; + lengthArgRemoveIncByOne(Result, Diag); + } +} + +void NotNullTerminatedResultCheck::xfrmFix( + StringRef Name, const MatchFinder::MatchResult &Result) { + auto Diag = diag(Result.Nodes.getNodeAs("expr")->getLocStart(), + "'%0' function's result is not null-terminated") + << Name; + lengthArgInsertIncByOne(2, Result, Diag); +} + +/// FIXME: It is probably a false-positive factory because of the complex cases. +void NotNullTerminatedResultCheck::lengthArgInsertIncByOne( + const int ArgPos, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + bool isComplexCase = false; + const Expr *StrLenExpr = nullptr; + if (const auto *StrLen = Result.Nodes.getNodeAs("length-wout-inc-simp")) + StrLenExpr = StrLen; + else { + StrLenExpr = Result.Nodes.getNodeAs("length-wout-inc-comp"); + isComplexCase = true; + } + + bool NeedInnerParen = false, NeedOuterParen = false; + if (isComplexCase) { + if (const auto *BinOp = + Result.Nodes.getNodeAs("length-wout-inc-comp")) { + NeedInnerParen = BinOp->getOpcode() != BO_Add; + } + } + if (!isInsideParentheses(StrLenExpr, Result)) { + const auto *FuncExpr = Result.Nodes.getNodeAs("expr"); + NeedOuterParen = FuncExpr->getNumArgs() > 1; + } + + std::string LHSPartStr = NeedOuterParen ? "(" : ""; + LHSPartStr += NeedInnerParen ? "(" : ""; + std::string RHSPartStr = NeedInnerParen ? ")" : ""; + RHSPartStr += " + 1"; + RHSPartStr += NeedOuterParen ? ")" : ""; + + const auto InsertFirstParenFix = + FixItHint::CreateInsertion(StrLenExpr->getLocStart(), LHSPartStr); + const auto InsertPlusOneAndSecondParenFix = FixItHint::CreateInsertion( + StrLenExpr->getLocEnd().getLocWithOffset(1), RHSPartStr); + Diag << InsertFirstParenFix << InsertPlusOneAndSecondParenFix; +} + +void NotNullTerminatedResultCheck::lengthArgRemoveIncByOne( + const MatchFinder::MatchResult &Result, DiagnosticBuilder &Diag) { + // This is the following structure: ((strlen(src) * 2) + 1) + // InnerOperatorExpr: ~~~~~~~~~~~~^~~ + // OuterOperatorExpr: ~~~~~~~~~~~~~~~~~~^~~ + const auto InnerOperatorExpr = Result.Nodes.getNodeAs("inner-operator"); + const auto OuterOperatorExpr = Result.Nodes.getNodeAs("outer-operator"); + int OuterOperatorOffset = 0; + + if (isInsideParentheses(OuterOperatorExpr, Result)) + ++OuterOperatorOffset; + + // This is the following structure: ((strlen(src) * 2) + 1) + // LHSRemoveRange: ~~ + // RHSRemoveRange: ~~~~~~ + const auto LHSRemoveRange = SourceRange( + OuterOperatorExpr->getLocStart().getLocWithOffset(-OuterOperatorOffset), + InnerOperatorExpr->getLocStart().getLocWithOffset(-1)); + + const auto RHSRemoveRange = SourceRange( + InnerOperatorExpr->getLocEnd().getLocWithOffset(1), + OuterOperatorExpr->getLocEnd().getLocWithOffset(OuterOperatorOffset)); + + const auto LHSRemoveFix = FixItHint::CreateRemoval(LHSRemoveRange); + const auto RHSRemoveFix = FixItHint::CreateRemoval(RHSRemoveRange); + Diag << LHSRemoveFix << RHSRemoveFix; +} + +void NotNullTerminatedResultCheck::removeArg( + const 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("expr"); + 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; +} + +void NotNullTerminatedResultCheck::renameFunc( + StringRef NewFuncName, const MatchFinder::MatchResult &Result, + DiagnosticBuilder &Diag) { + const auto *FuncExpr = Result.Nodes.getNodeAs("expr"); + const auto FuncNameLength = + FuncExpr->getDirectCallee()->getIdentifier()->getLength(); + const auto FirstArgLoc = FuncExpr->getArg(0)->getExprLoc(); + const auto FuncNameRange = CharSourceRange::getCharRange( + FirstArgLoc.getLocWithOffset(-FuncNameLength - 1), + FirstArgLoc.getLocWithOffset(-1)); + + const auto FuncNameFix = + FixItHint::CreateReplacement(FuncNameRange, NewFuncName); + Diag << FuncNameFix; +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -59,6 +59,12 @@ - New module ``portability``. +- New `bugprone-not-null-terminated-result + `_ check + + Finds function calls where ``strlen`` or ``wcslen`` passed as argument and + cause a not null-terminated result. + - New module ``zircon`` for checks related to Fuchsia's Zircon kernel. - New `bugprone-throw-keyword-missing 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,94 @@ +.. title:: clang-tidy - bugprone-not-null-terminated-result + +bugprone-not-null-terminated-result +========================================= + +This check can be used to find function calls where ``strlen`` or ``wcslen`` +are passed as an argument and cause 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. + +The following function calls are checked: + +``alloca``, ``calloc``, ``malloc``, ``realloc``, ``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``. The proper length is +``strlen(src) + 1`` because the null-terminator need an extra space. The result +is badly not-null-terminated: + +.. code-block:: c + + void bad_memcpy(char *dest, const char *src) { + memcpy(dest, src, strlen(src)); + } + +In addition to issuing warnings, "Fix-it" rewrite all the necessary code +depending on the version number. The upper code would be the following: + +- If C++11 is the target "Fix-it" will rewrite it to the most safe function: + +.. code-block:: c++ + + void good_memcpy_cxx11(char *dest, const char *src) { + strncpy_s(dest, src, strlen(src)); + } + +- Else "Fix-it" will rewrite it to a safer function, that born before C++11: + +.. code-block:: c + + void good_memcpy_not_cxx11(char *dest, const char *src) { + strncpy(dest, src, (strlen(src) + 1)); + } + +In general, the following transformations are could happen: + +**Memory allocation functions** + +- ``alloca`` function's argument gets a ``+ 1`` operation. + +- ``calloc`` function's first argument gets a ``+ 1`` operation. + +- ``malloc`` function's argument gets a ``+ 1`` operation. + +- ``realloc`` function's second argument gets a ``+ 1`` operation. + +**Memory handler functions** + +- ``memcpy``, ``wmemcpy``: + - C++11: New function is ``strncpy_s``/``wcsncpy_s``, where no ``+ 1`` needed. + + - Not C++11: New function is ``strncpy``, where ``+ 1`` needed. + +- ``memchr``, ``wmemchr``: + - 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``, ``wmemmove``: + - C++11: New function is ``memmmove_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. + + - Not C++11: The third argument gets a ``+ 1`` operation. + +- ``memmove_s``/``wmemmove_s``: + - The function's fourth argument gets a ``+ 1`` operation. + +- ``memset``/``wmemset``: + - The function's third argument has to be truncated without the ``+ 1`` part. + +**Character functions** + +- ``strerror_s`` functions's second argument get a ``+ 1`` operation. + +- ``strncmp``/``wcsncmp``: + - 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``/``wcsxfrm`` function's third argument gets a ``+ 1`` operation. + Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -36,6 +36,7 @@ bugprone-misplaced-widening-cast bugprone-move-forwarding-reference bugprone-multiple-statement-macro + bugprone-not-null-terminated-result bugprone-sizeof-container bugprone-sizeof-expression bugprone-string-constructor Index: test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-cxx11.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-cxx11.cpp @@ -0,0 +1,33 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- -- -std=c++98 + +typedef unsigned int size_t; +size_t strlen(const char *); +char *strncpy(char *, const char *, size_t); + +void *memcpy(void *, const void *, size_t); +void *memmove(void *, const void *, size_t); + +void bad_memcpy(char *dest, const char *src) { + memcpy(dest, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncpy(dest, src, (strlen(src) + 1)); +} + +void good_memcpy_not_cxx11(char *dest, const char *src) { + strncpy(dest, src, (strlen(src) + 1)); +} + +void bad_memmove(char *dest, const char *src) { + memmove(dest, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memmove' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memmove(dest, src, (strlen(src) + 1)); +} + +void good_memmove_not_cxx11_1(char *dest, const char *src) { + memmove(dest, src, (strlen(src) + 1)); +} + +void good_memmove_not_cxx11_2(char *dest, const char *src) { + memmove(dest, (src + 1), strlen(src)); +} + Index: test/clang-tidy/bugprone-not-null-terminated-result-strlen-cxx11.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-strlen-cxx11.cpp @@ -0,0 +1,167 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- -- -std=c++11 + +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); + +void *alloca(size_t); +void *calloc(size_t, size_t); +void *malloc(size_t); +void *realloc(void *, size_t); + +void *memcpy(void *, const void *, size_t); +errno_t memcpy_s(void *, size_t, const void *, 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); + +errno_t strerror_s(char *, size_t, int); +int strncmp(const char *, const char *, size_t); +size_t strxfrm(char *, const char *, size_t); + +namespace std { +using ::memcpy; +using ::strlen; +using ::strncpy_s; +} // namespace std + +void bad_alloca(char *dest, const char *src) { + dest = (char *)alloca(strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'alloca' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: alloca(strlen(src) + 1); +} + +void good_alloca(char *dest, const char *src) { + dest = (char *)alloca(strlen(src) + 1); +} + +void bad_calloc(char *dest, const char *src) { + dest = (char *)calloc(strlen(src), sizeof(char)); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'calloc' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: calloc((strlen(src) + 1), sizeof(char)); +} + +void good_calloc(char *dest, const char *src) { + dest = (char *)calloc((strlen(src) + 1), sizeof(char)); +} + +void bad_malloc(char *dest, const char *src) { + dest = (char *)malloc(strlen(src) * 2); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'malloc' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: malloc((strlen(src) * 2) + 1); +} + +void good_malloc(char *dest, const char *src) { + dest = (char *)malloc((strlen(src) * 2) + 1); +} + +void bad_realloc(char *dest, const char *src) { + dest = (char *)realloc(dest, (strlen(dest) + strlen(src))); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'realloc' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: realloc(dest, (strlen(dest) + (strlen(src) + 1))); +} + +void good_realloc(char *dest, const char *src) { + dest = (char *)realloc(dest, (strlen(dest) + (strlen(src) + 1))); +} + +void bad_memcpy(char *dest, const char *src) { + std::memcpy(dest, src, std::strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: std::strncpy_s(dest, src, std::strlen(src)); +} + +void good_memcpy_cxx11(char *dest, const char *src) { + std::strncpy_s(dest, src, std::strlen(src)); +} + +void bad_memcpy_s(char *dest, const char *src) { + memcpy_s(dest, strlen(dest), src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy_s' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncpy_s(dest, src, strlen(src)); +} + +void good_memcpy_s(char *dest, const char *src) { + strncpy_s(dest, src, strlen(src)); +} + +void bad_memchr(char *dest, const char *src) { + dest = (char *)memchr(src, '\0', strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'memchr' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strchr(src, '\0'); +} + +void good_memchr(char *dest, const char *src) { + dest = strchr(src, '\0'); +} + +void bad_memmove(char *dest, const char *src) { + memmove(dest, src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memmove' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memmove_s(dest, strlen(dest), src, (strlen(src) + 1)); +} + +void good_memmove_cxx11(char *dest, const char *src) { + memmove_s(dest, strlen(dest), src, (strlen(src) + 1)); +} + +void bad_memmove_s(char *dest, const char *src) { + memmove_s(dest, strlen(dest), src, strlen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memmove_s' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memmove_s(dest, strlen(dest), src, (strlen(src) + 1)); +} + +void good_memmove_s_1(char *dest, const char *src) { + memmove_s(dest, strlen(dest), src, (strlen(src) + 1)); +} + +void good_memmove_s_2(char *dest, const char *src) { + memmove_s(dest, strlen(dest), (src + 1), strlen(src)); +} + +void bad_memset(char *dest, const char *src) { + memset(dest, '-', (strlen(src) + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memset' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: memset(dest, '-', strlen(src)); +} + +void good_memset(char *dest, const char *src) { + memset(dest, '-', strlen(src)); +} + +void bad_strerror_s(char *dest, int errno) { + strerror_s(dest, strlen(strerror(errno)), errno); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'strerror_s' function's result is not null-terminated and missing the last character of the error message [bugprone-not-null-terminated-result] + // CHECK-FIXES: strerror_s(dest, (strlen(strerror(errno)) + 1), errno); +} + +void good_strerror_s(char *dest, int errno) { + strerror_s(dest, (strlen(strerror(errno)) + 1), errno); +} + +int bad_strncmp(char *str1, const char *str2) { + return strncmp(str1, str2, (strlen(str1) + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'strncmp' function's comparison's length is too long [bugprone-not-null-terminated-result] + // CHECK-FIXES: strncmp(str1, str2, strlen(str1)); +} + +int good_strncmp(const char *str1, const char *str2) { + return strncmp(str1, str2, strlen(str1)); +} + +void bad_strxfrm(char *long_destination_name, const char *long_source_name) { + strxfrm(long_destination_name, long_source_name, + strlen(long_source_name)); + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: 'strxfrm' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: strxfrm(long_destination_name, long_source_name, + // CHECK-FIXES-NEXT: strlen(long_source_name) + 1); +} + +void good_strxfrm(char *long_destination_name, const char *long_source_name) { + strxfrm(long_destination_name, long_source_name, + (strlen(long_source_name) + 1)); +} Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-cxx11.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-cxx11.cpp @@ -0,0 +1,33 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- -- -std=c++98 + +typedef unsigned int size_t; +size_t wcslen(const char *); +char *wcsncpy(char *, const char *, size_t); + +void *wmemcpy(void *, const void *, size_t); +void *wmemmove(void *, const void *, size_t); + +void bad_wmemcpy(char *dest, const char *src) { + wmemcpy(dest, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wmemcpy' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcsncpy(dest, src, (wcslen(src) + 1)); +} + +void good_wmemcpy_not_cxx11(char *dest, const char *src) { + wcsncpy(dest, src, (wcslen(src) + 1)); +} + +void bad_wmemmove(char *dest, const char *src) { + wmemmove(dest, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wmemmove' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wmemmove(dest, src, (wcslen(src) + 1)); +} + +void good_wmemmove_not_cxx11_1(char *dest, const char *src) { + wmemmove(dest, src, (wcslen(src) + 1)); +} + +void good_wmemmove_not_cxx11_2(char *dest, const char *src) { + wmemmove(dest, (src + 1), wcslen(src)); +} + Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-cxx11.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-cxx11.cpp @@ -0,0 +1,153 @@ +// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- -- -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 *, const wchar_t *, size_t); + +void *alloca(size_t); +void *calloc(size_t, size_t); +void *malloc(size_t); +void *realloc(void *, size_t); + +void *wmemcpy(void *, const void *, size_t); +errno_t wmemcpy_s(void *, size_t, const void *, 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); + +int wcsncmp(const wchar_t *, const wchar_t *, size_t); +size_t wcsxfrm(wchar_t *, const wchar_t *, size_t); + +namespace std { +using ::wcsncpy_s; +using ::wmemcpy; +using ::wcslen; +} + +void bad_alloca(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)alloca(wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'alloca' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: alloca(wcslen(src) + 1); +} + +void good_alloca(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)alloca(wcslen(src) + 1); +} + +void bad_calloc(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)calloc(wcslen(src), sizeof(wchar_t)); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'calloc' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: calloc((wcslen(src) + 1), sizeof(wchar_t)); +} + +void good_calloc(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)calloc((wcslen(src) + 1), sizeof(wchar_t)); +} + +void bad_malloc(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)malloc(wcslen(src) * 2); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'malloc' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: malloc((wcslen(src) * 2) + 1); +} + +void good_malloc(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)malloc((wcslen(src) * 2) + 1); +} + +void bad_realloc(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)realloc(dest, (wcslen(dest) + wcslen(src))); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'realloc' function's allocated memory cannot hold the null-terminator [bugprone-not-null-terminated-result] + // CHECK-FIXES: realloc(dest, (wcslen(dest) + (wcslen(src) + 1))); +} + +void good_realloc(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)realloc(dest, (wcslen(dest) + (wcslen(src) + 1))); +} + +void bad_wmemcpy(wchar_t *dest, const wchar_t *src) { + std::wmemcpy(dest, src, std::wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wmemcpy' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: std::wcsncpy_s(dest, src, std::wcslen(src)); +} + +void good_wmemcpy_cxx11(wchar_t *dest, const wchar_t *src) { + std::wcsncpy_s(dest, src, std::wcslen(src)); +} + +void bad_wmemcpy_s(wchar_t *dest, const wchar_t *src) { + wmemcpy_s(dest, wcslen(dest), src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wmemcpy_s' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src)); +} + +void good_wmemcpy_s(wchar_t *dest, const wchar_t *src) { + wcsncpy_s(dest, src, wcslen(src)); +} + +void bad_wmemchr(wchar_t *dest, const wchar_t *src) { + dest = (wchar_t *)wmemchr(src, '\0', wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'wmemchr' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcschr(src, '\0'); +} + +void good_wmemchr(wchar_t *dest, const wchar_t *src) { + dest = wcschr(src, '\0'); +} + +void bad_wmemmove(wchar_t *dest, const wchar_t *src) { + wmemmove(dest, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wmemmove' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1)); +} + +void good_wmemmove_cxx11(wchar_t *dest, const wchar_t *src) { + wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1)); +} + +void bad_wmemmove_s(wchar_t *dest, const wchar_t *src) { + wmemmove_s(dest, wcslen(dest), src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wmemmove_s' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1)); +} + +void good_wmemmove_s_1(wchar_t *dest, const wchar_t *src) { + wmemmove_s(dest, wcslen(dest), src, (wcslen(src) + 1)); +} + +void good_wmemmove_s_2(wchar_t *dest, const wchar_t *src) { + wmemmove_s(dest, wcslen(dest), (src + 1), wcslen(src)); +} + +void bad_wmemset(wchar_t *dest, const wchar_t *src) { + wmemset(dest, '-', (wcslen(src) + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wmemset' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wmemset(dest, '-', wcslen(src)); +} + +void good_wmemset(wchar_t *dest, const wchar_t *src) { + wmemset(dest, '-', wcslen(src)); +} + +int bad_wcsncmp(const wchar_t *str1, const wchar_t *str2) { + return wcsncmp(str1, str2, (wcslen(str1) + 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'wcsncmp' function's comparison's length is too long [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcsncmp(str1, str2, wcslen(str1)); +} + +int good_wcsncmp(const wchar_t *str1, const wchar_t *str2) { + return wcsncmp(str1, str2, wcslen(str1)); +} + +void bad_wcsxfrm(wchar_t *dest, const wchar_t *src) { + wcsxfrm(dest, src, wcslen(src)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'wcsxfrm' function's result is not null-terminated [bugprone-not-null-terminated-result] + // CHECK-FIXES: wcsxfrm(dest, src, (wcslen(src) + 1)); +} + +void good_wcsxfrm(wchar_t *dest, const wchar_t *src) { + wcsxfrm(dest, src, (wcslen(src) + 1)); +} +