From 829e75a0377403cd94204d74c4f1e94fca68c96b Mon Sep 17 00:00:00 2001 From: Gabor Horvath Date: Fri, 14 Jul 2017 12:15:55 +0000 Subject: [PATCH] [clang-tidy] Add bugprone-suspicious-memset-usage check Created new module bugprone and placed the check in that. Finds memset() calls with potential mistakes in their arguments. Replaces and extends the existing google-runtime-memset-zero-length check. Cases covered: * Fill value is a character '0'. Integer 0 might have been intended. * Fill value is out of char range and gets truncated. * Byte count is zero. Potentially swapped with the fill value argument. Patch by: Reka Nikolett Kovacs Differential Revision: https://reviews.llvm.org/D32700 llvm-svn: 308020 --- clang-tools-extra/clang-tidy/CMakeLists.txt | 1 + .../bugprone/BugproneTidyModule.cpp | 38 ++++++ .../clang-tidy/bugprone/CMakeLists.txt | 16 +++ .../bugprone/SuspiciousMemsetUsageCheck.cpp | 127 ++++++++++++++++++ .../bugprone/SuspiciousMemsetUsageCheck.h | 35 +++++ .../clang-tidy/google/CMakeLists.txt | 1 - .../clang-tidy/google/GoogleTidyModule.cpp | 3 - .../google/MemsetZeroLengthCheck.cpp | 96 ------------- .../clang-tidy/google/MemsetZeroLengthCheck.h | 39 ------ .../clang-tidy/tool/CMakeLists.txt | 1 + .../clang-tidy/tool/ClangTidyMain.cpp | 5 + clang-tools-extra/docs/ReleaseNotes.rst | 10 ++ .../bugprone-suspicious-memset-usage.rst | 54 ++++++++ .../checks/google-runtime-memset.rst | 10 -- .../docs/clang-tidy/checks/list.rst | 2 +- clang-tools-extra/docs/clang-tidy/index.rst | 1 + .../bugprone-suspicious-memset-usage.cpp | 77 +++++++++++ .../google-runtime-memset-zero-length.cpp | 62 --------- 18 files changed, 366 insertions(+), 212 deletions(-) create mode 100644 clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt create mode 100644 clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.h delete mode 100644 clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.cpp delete mode 100644 clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst delete mode 100644 clang-tools-extra/docs/clang-tidy/checks/google-runtime-memset.rst create mode 100644 clang-tools-extra/test/clang-tidy/bugprone-suspicious-memset-usage.cpp delete mode 100644 clang-tools-extra/test/clang-tidy/google-runtime-memset-zero-length.cpp diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt index 57ec851085e4..9d6fc7f035a9 100644 --- a/clang-tools-extra/clang-tidy/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/CMakeLists.txt @@ -28,6 +28,7 @@ add_clang_library(clangTidy add_subdirectory(android) add_subdirectory(boost) +add_subdirectory(bugprone) add_subdirectory(cert) add_subdirectory(cppcoreguidelines) add_subdirectory(google) diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp new file mode 100644 index 000000000000..65353a23c7a4 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -0,0 +1,38 @@ +//===--- BugproneTidyModule.cpp - clang-tidy ------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "../ClangTidy.h" +#include "../ClangTidyModule.h" +#include "../ClangTidyModuleRegistry.h" +#include "SuspiciousMemsetUsageCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +class BugproneModule : public ClangTidyModule { +public: + void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "bugprone-suspicious-memset-usage"); + } +}; + +} // namespace bugprone + +// Register the BugproneTidyModule using this statically initialized variable. +static ClangTidyModuleRegistry::Add + X("bugprone-module", "Adds checks for bugprone code constructs."); + +// This anchor is used to force the linker to link in the generated object file +// and thus register the BugproneModule. +volatile int BugproneModuleAnchorSource = 0; + +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt new file mode 100644 index 000000000000..1f0ec6e04273 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -0,0 +1,16 @@ +set(LLVM_LINK_COMPONENTS support) + +add_clang_library(clangTidyBugproneModule + BugproneTidyModule.cpp + SuspiciousMemsetUsageCheck.cpp + + LINK_LIBS + clangAnalysis + clangAST + clangASTMatchers + clangBasic + clangLex + clangTidy + clangTidyUtils + clangTooling + ) diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp new file mode 100644 index 000000000000..8e11a4351741 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.cpp @@ -0,0 +1,127 @@ +//===--- SuspiciousMemsetUsageCheck.cpp - clang-tidy-----------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "SuspiciousMemsetUsageCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include "clang/Tooling/FixIt.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void SuspiciousMemsetUsageCheck::registerMatchers(MatchFinder *Finder) { + // Note: void *memset(void *buffer, int fill_char, size_t byte_count); + // Look for memset(x, '0', z). Probably memset(x, 0, z) was intended. + Finder->addMatcher( + callExpr( + callee(functionDecl(hasName("::memset"))), + hasArgument(1, characterLiteral(equals(static_cast('0'))) + .bind("char-zero-fill")), + unless( + eachOf(hasArgument(0, anyOf(hasType(pointsTo(isAnyCharacter())), + hasType(arrayType(hasElementType( + isAnyCharacter()))))), + isInTemplateInstantiation()))), + this); + + // Look for memset with an integer literal in its fill_char argument. + // Will check if it gets truncated. + Finder->addMatcher(callExpr(callee(functionDecl(hasName("::memset"))), + hasArgument(1, integerLiteral().bind("num-fill")), + unless(isInTemplateInstantiation())), + this); + + // Look for memset(x, y, 0) as that is most likely an argument swap. + Finder->addMatcher( + callExpr(callee(functionDecl(hasName("::memset"))), + unless(hasArgument(1, anyOf(characterLiteral(equals( + static_cast('0'))), + integerLiteral()))), + unless(isInTemplateInstantiation())) + .bind("call"), + this); +} + +void SuspiciousMemsetUsageCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *CharZeroFill = + Result.Nodes.getNodeAs("char-zero-fill")) { + // Case 1: fill_char of memset() is a character '0'. Probably an + // integer zero was intended. + + SourceRange CharRange = CharZeroFill->getSourceRange(); + auto Diag = + diag(CharZeroFill->getLocStart(), "memset fill value is char '0', " + "potentially mistaken for int 0"); + + // Only suggest a fix if no macros are involved. + if (CharRange.getBegin().isMacroID()) + return; + Diag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(CharRange), "0"); + } + + else if (const auto *NumFill = + Result.Nodes.getNodeAs("num-fill")) { + // Case 2: fill_char of memset() is larger in size than an unsigned char + // so it gets truncated during conversion. + + llvm::APSInt NumValue; + const auto UCharMax = (1 << Result.Context->getCharWidth()) - 1; + if (!NumFill->EvaluateAsInt(NumValue, *Result.Context) || + (NumValue >= 0 && NumValue <= UCharMax)) + return; + + diag(NumFill->getLocStart(), "memset fill value is out of unsigned " + "character range, gets truncated"); + } + + else if (const auto *Call = Result.Nodes.getNodeAs("call")) { + // Case 3: byte_count of memset() is zero. This is most likely an + // argument swap. + + const Expr *FillChar = Call->getArg(1); + const Expr *ByteCount = Call->getArg(2); + + // Return if `byte_count` is not zero at compile time. + llvm::APSInt Value1, Value2; + if (ByteCount->isValueDependent() || + !ByteCount->EvaluateAsInt(Value2, *Result.Context) || Value2 != 0) + return; + + // Return if `fill_char` is known to be zero or negative at compile + // time. In these cases, swapping the args would be a nop, or + // introduce a definite bug. The code is likely correct. + if (!FillChar->isValueDependent() && + FillChar->EvaluateAsInt(Value1, *Result.Context) && + (Value1 == 0 || Value1.isNegative())) + return; + + // `byte_count` is known to be zero at compile time, and `fill_char` is + // either not known or known to be a positive integer. Emit a warning + // and fix-its to swap the arguments. + auto D = diag(Call->getLocStart(), + "memset of size zero, potentially swapped arguments"); + StringRef RHSString = tooling::fixit::getText(*ByteCount, *Result.Context); + StringRef LHSString = tooling::fixit::getText(*FillChar, *Result.Context); + if (LHSString.empty() || RHSString.empty()) + return; + + D << tooling::fixit::createReplacement(*FillChar, RHSString) + << tooling::fixit::createReplacement(*ByteCount, LHSString); + } +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.h b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.h new file mode 100644 index 000000000000..1c0d1bcf3e95 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousMemsetUsageCheck.h @@ -0,0 +1,35 @@ +//===--- SuspiciousMemsetUsageCheck.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_SUSPICIOUS_MEMSET_USAGE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUS_MEMSET_USAGE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds memset calls with potential mistakes in their arguments. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-memset-usage.html +class SuspiciousMemsetUsageCheck : public ClangTidyCheck { +public: + SuspiciousMemsetUsageCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUS_MEMSET_USAGE_H diff --git a/clang-tools-extra/clang-tidy/google/CMakeLists.txt b/clang-tools-extra/clang-tidy/google/CMakeLists.txt index efe3b3cde9c4..ff48b63d03d1 100644 --- a/clang-tools-extra/clang-tidy/google/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/google/CMakeLists.txt @@ -8,7 +8,6 @@ add_clang_library(clangTidyGoogleModule GlobalNamesInHeadersCheck.cpp GoogleTidyModule.cpp IntegerTypesCheck.cpp - MemsetZeroLengthCheck.cpp NonConstReferences.cpp OverloadedUnaryAndCheck.cpp StringReferenceMemberCheck.cpp diff --git a/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp b/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp index e9b4bedf0119..1328463e3563 100644 --- a/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp @@ -20,7 +20,6 @@ #include "ExplicitMakePairCheck.h" #include "GlobalNamesInHeadersCheck.h" #include "IntegerTypesCheck.h" -#include "MemsetZeroLengthCheck.h" #include "NonConstReferences.h" #include "OverloadedUnaryAndCheck.h" #include "StringReferenceMemberCheck.h" @@ -55,8 +54,6 @@ public: "google-runtime-references"); CheckFactories.registerCheck( "google-runtime-member-string-references"); - CheckFactories.registerCheck( - "google-runtime-memset"); CheckFactories.registerCheck( "google-readability-casting"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.cpp b/clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.cpp deleted file mode 100644 index fa87fab204d7..000000000000 --- a/clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.cpp +++ /dev/null @@ -1,96 +0,0 @@ -//===--- MemsetZeroLengthCheck.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 "MemsetZeroLengthCheck.h" -#include "clang/AST/ASTContext.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/ASTMatchers/ASTMatchers.h" -#include "clang/Lex/Lexer.h" - -using namespace clang::ast_matchers; - -namespace clang { -namespace tidy { -namespace google { -namespace runtime { - -void MemsetZeroLengthCheck::registerMatchers( - ast_matchers::MatchFinder *Finder) { - // Look for memset(x, y, 0) as those is most likely an argument swap. - // TODO: Also handle other standard functions that suffer from the same - // problem, e.g. memchr. - Finder->addMatcher(callExpr(callee(functionDecl(hasName("::memset"))), - argumentCountIs(3), - unless(isInTemplateInstantiation())) - .bind("decl"), - this); -} - -/// \brief Get a StringRef representing a SourceRange. -static StringRef getAsString(const MatchFinder::MatchResult &Result, - SourceRange R) { - const SourceManager &SM = *Result.SourceManager; - // Don't even try to resolve macro or include contraptions. Not worth emitting - // a fixit for. - if (R.getBegin().isMacroID() || - !SM.isWrittenInSameFile(R.getBegin(), R.getEnd())) - return StringRef(); - - const char *Begin = SM.getCharacterData(R.getBegin()); - const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken( - R.getEnd(), 0, SM, Result.Context->getLangOpts())); - - return StringRef(Begin, End - Begin); -} - -void MemsetZeroLengthCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Call = Result.Nodes.getNodeAs("decl"); - - // Note, this is: - // void *memset(void *buffer, int fill_char, size_t byte_count); - // Arg1 is fill_char, Arg2 is byte_count. - const Expr *Arg1 = Call->getArg(1); - const Expr *Arg2 = Call->getArg(2); - - // Return if `byte_count` is not zero at compile time. - llvm::APSInt Value1, Value2; - if (Arg2->isValueDependent() || - !Arg2->EvaluateAsInt(Value2, *Result.Context) || Value2 != 0) - return; - - // Return if `fill_char` is known to be zero or negative at compile - // time. In these cases, swapping the args would be a nop, or - // introduce a definite bug. The code is likely correct. - if (!Arg1->isValueDependent() && - Arg1->EvaluateAsInt(Value1, *Result.Context) && - (Value1 == 0 || Value1.isNegative())) - return; - - // `byte_count` is known to be zero at compile time, and `fill_char` is - // either not known or known to be a positive integer. Emit a warning - // and fix-its to swap the arguments. - auto D = diag(Call->getLocStart(), - "memset of size zero, potentially swapped arguments"); - SourceRange LHSRange = Arg1->getSourceRange(); - SourceRange RHSRange = Arg2->getSourceRange(); - StringRef RHSString = getAsString(Result, RHSRange); - StringRef LHSString = getAsString(Result, LHSRange); - if (LHSString.empty() || RHSString.empty()) - return; - - D << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(LHSRange), - RHSString) - << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(RHSRange), - LHSString); -} - -} // namespace runtime -} // namespace google -} // namespace tidy -} // namespace clang diff --git a/clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.h b/clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.h deleted file mode 100644 index 57c7aec746b2..000000000000 --- a/clang-tools-extra/clang-tidy/google/MemsetZeroLengthCheck.h +++ /dev/null @@ -1,39 +0,0 @@ -//===--- MemsetZeroLengthCheck.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_GOOGLE_MEMSETZEROLENGTHCHECK_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSETZEROLENGTHCHECK_H - -#include "../ClangTidy.h" - -namespace clang { -namespace tidy { -namespace google { -namespace runtime { - -/// Finds calls to memset with a literal zero in the length argument. -/// -/// This is most likely unintended and the length and value arguments are -/// swapped. -/// -/// Corresponding cpplint.py check name: 'runtime/memset'. -class MemsetZeroLengthCheck : public ClangTidyCheck { -public: - MemsetZeroLengthCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} - void registerMatchers(ast_matchers::MatchFinder *Finder) override; - void check(const ast_matchers::MatchFinder::MatchResult &Result) override; -}; - -} // namespace runtime -} // namespace google -} // namespace tidy -} // namespace clang - -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSETZEROLENGTHCHECK_H diff --git a/clang-tools-extra/clang-tidy/tool/CMakeLists.txt b/clang-tools-extra/clang-tidy/tool/CMakeLists.txt index 5b952c4f225a..b8c34f367501 100644 --- a/clang-tools-extra/clang-tidy/tool/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/tool/CMakeLists.txt @@ -15,6 +15,7 @@ target_link_libraries(clang-tidy clangTidy clangTidyAndroidModule clangTidyBoostModule + clangTidyBugproneModule clangTidyCERTModule clangTidyCppCoreGuidelinesModule clangTidyGoogleModule diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp index bfc995ac68fb..803dca17122f 100644 --- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp +++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp @@ -462,6 +462,11 @@ extern volatile int BoostModuleAnchorSource; static int LLVM_ATTRIBUTE_UNUSED BoostModuleAnchorDestination = BoostModuleAnchorSource; +// This anchor is used to force the linker to link the BugproneModule. +extern volatile int BugproneModuleAnchorSource; +static int LLVM_ATTRIBUTE_UNUSED BugproneModuleAnchorDestination = + BugproneModuleAnchorSource; + // This anchor is used to force the linker to link the LLVMModule. extern volatile int LLVMModuleAnchorSource; static int LLVM_ATTRIBUTE_UNUSED LLVMModuleAnchorDestination = diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 88d0ea603d5e..c4354745cb4a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -79,6 +79,12 @@ Improvements to clang-tidy Checks if the required file flag ``SOCK_CLOEXEC`` is present in the argument of ``socket()``. +- New `bugprone-suspicious-memset-usage + `_ check + + Finds ``memset()`` calls with potential mistakes in their arguments. + Replaces and extends the ``google-runtime-memset`` check. + - New `cert-dcl21-cpp `_ check @@ -157,6 +163,10 @@ Improvements to clang-tidy - Support clang-formatting of the code around applied fixes (``-format-style`` command-line option). +- New `bugprone` module + + Adds checks that target bugprone code constructs. + - New `hicpp` module Adds checks that implement the `High Integrity C++ Coding Standard `_ and other safety diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst new file mode 100644 index 000000000000..82609d13e4ef --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst @@ -0,0 +1,54 @@ +.. title:: clang-tidy - bugprone-suspicious-memset-usage + +bugprone-suspicious-memset-usage +================================ + +This check finds ``memset()`` calls with potential mistakes in their arguments. +Considering the function as ``void* memset(void* destination, int fill_value, +size_t byte_count)``, the following cases are covered: + +**Case 1: Fill value is a character ``'0'``** + +Filling up a memory area with ASCII code 48 characters is not customary, +possibly integer zeroes were intended instead. +The check offers a replacement of ``'0'`` with ``0``. Memsetting character +pointers with ``'0'`` is allowed. + +**Case 2: Fill value is truncated** + +Memset converts ``fill_value`` to ``unsigned char`` before using it. If +``fill_value`` is out of unsigned character range, it gets truncated +and memory will not contain the desired pattern. + +**Case 3: Byte count is zero** + +Calling memset with a literal zero in its ``byte_count`` argument is likely +to be unintended and swapped with ``fill_value``. The check offers to swap +these two arguments. + +Corresponding cpplint.py check name: ``runtime/memset``. + + +Examples: + +.. code-block:: c++ + + void foo() { + int i[5] = {1, 2, 3, 4, 5}; + int *ip = i; + char c = '1'; + char *cp = &c; + int v = 0; + + // Case 1 + memset(ip, '0', 1); // suspicious + memset(cp, '0', 1); // OK + + // Case 2 + memset(ip, 0xabcd, 1); // fill value gets truncated + memset(ip, 0x00, 1); // OK + + // Case 3 + memset(ip, sizeof(int), v); // zero length, potentially swapped + memset(ip, 0, 1); // OK + } diff --git a/clang-tools-extra/docs/clang-tidy/checks/google-runtime-memset.rst b/clang-tools-extra/docs/clang-tidy/checks/google-runtime-memset.rst deleted file mode 100644 index 3832b6e44856..000000000000 --- a/clang-tools-extra/docs/clang-tidy/checks/google-runtime-memset.rst +++ /dev/null @@ -1,10 +0,0 @@ -.. title:: clang-tidy - google-runtime-memset - -google-runtime-memset -===================== - -Finds calls to ``memset`` with a literal zero in the length argument. - -This is most likely unintended and the length and value arguments are swapped. - -Corresponding cpplint.py check name: `runtime/memset`. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 321573136469..6f091c5a5705 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -9,6 +9,7 @@ Clang-Tidy Checks android-cloexec-open android-cloexec-socket boost-use-to-string + bugprone-suspicious-memset-usage cert-dcl03-c (redirects to misc-static-assert) cert-dcl21-cpp cert-dcl50-cpp @@ -55,7 +56,6 @@ Clang-Tidy Checks google-readability-todo google-runtime-int google-runtime-member-string-references - google-runtime-memset google-runtime-operator google-runtime-references hicpp-explicit-conversions diff --git a/clang-tools-extra/docs/clang-tidy/index.rst b/clang-tools-extra/docs/clang-tidy/index.rst index 4b3576c1b723..69136e6e3e4e 100644 --- a/clang-tools-extra/docs/clang-tidy/index.rst +++ b/clang-tools-extra/docs/clang-tidy/index.rst @@ -57,6 +57,7 @@ Name prefix Description ====================== ========================================================= ``android-`` Checks related to Android. ``boost-`` Checks related to Boost library. +``bugprone-`` Checks that target bugprone code constructs. ``cert-`` Checks related to CERT Secure Coding Guidelines. ``cppcoreguidelines-`` Checks related to C++ Core Guidelines. ``clang-analyzer-`` Clang Static Analyzer checks. diff --git a/clang-tools-extra/test/clang-tidy/bugprone-suspicious-memset-usage.cpp b/clang-tools-extra/test/clang-tidy/bugprone-suspicious-memset-usage.cpp new file mode 100644 index 000000000000..f33ae5ae10a8 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/bugprone-suspicious-memset-usage.cpp @@ -0,0 +1,77 @@ +// RUN: %check_clang_tidy %s bugprone-suspicious-memset-usage %t + +void *memset(void *, int, __SIZE_TYPE__); + +namespace std { + using ::memset; +} + +template +void mtempl(int *ptr) { + memset(ptr, '0', sizeof(T)); +// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage] +// CHECK-FIXES: memset(ptr, 0, sizeof(T)); + memset(ptr, 256, sizeof(T)); +// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: memset fill value is out of unsigned character range, gets truncated [bugprone-suspicious-memset-usage] + memset(0, sizeof(T), 0); +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage] +// CHECK-FIXES: memset(0, 0, sizeof(T)); + memset(0, sizeof(int), 0); +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage] +// CHECK-FIXES: memset(0, 0, sizeof(int)); +} + +void foo(int xsize, int ysize) { + int i[5] = {1, 2, 3, 4, 5}; + char ca[3] = {'a', 'b', 'c'}; + int *p = i; + int l = 5; + char z = '1'; + char *c = &z; + int v = 0; + + memset(p, '0', l); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage] +// CHECK-FIXES: memset(p, 0, l); + + memset(p, 0xabcd, l); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: memset fill value is out of unsigned character range, gets truncated [bugprone-suspicious-memset-usage] + + memset(p, sizeof(int), 0); +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage] +// CHECK-FIXES: memset(p, 0, sizeof(int)); + std::memset(p, sizeof(int), 0x00); +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage] +// CHECK-FIXES: std::memset(p, 0x00, sizeof(int)); + +#define M_CHAR_ZERO memset(p, '0', l); + M_CHAR_ZERO +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset fill value is char '0', potentially mistaken for int 0 [bugprone-suspicious-memset-usage] + +#define M_OUTSIDE_RANGE memset(p, 0xabcd, l); + M_OUTSIDE_RANGE +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset fill value is out of unsigned character range, gets truncated [bugprone-suspicious-memset-usage] + +#define M_ZERO_LENGTH memset(p, sizeof(int), 0); + M_ZERO_LENGTH +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped arguments [bugprone-suspicious-memset-usage] + + memset(p, '2', l); + memset(p, 0, l); + memset(c, '0', 1); + memset(ca, '0', sizeof(ca)); + + memset(p, 0x00, l); + mtempl(p); + + memset(p, sizeof(int), v + 1); + memset(p, 0xcd, 1); + + // Don't warn when the fill char and the length are both known to be + // zero. No bug is possible. + memset(p, 0, v); + + // -1 is clearly not a length by virtue of being negative, so no warning + // despite v == 0. + memset(p, -1, v); +} diff --git a/clang-tools-extra/test/clang-tidy/google-runtime-memset-zero-length.cpp b/clang-tools-extra/test/clang-tidy/google-runtime-memset-zero-length.cpp deleted file mode 100644 index 7599c755b485..000000000000 --- a/clang-tools-extra/test/clang-tidy/google-runtime-memset-zero-length.cpp +++ /dev/null @@ -1,62 +0,0 @@ -// RUN: %check_clang_tidy %s google-runtime-memset %t - -void *memset(void *, int, __SIZE_TYPE__); - -namespace std { - using ::memset; -} - -template -void memtmpl() { - memset(0, sizeof(int), i); - memset(0, sizeof(T), sizeof(T)); - memset(0, sizeof(T), 0); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: memset(0, 0, sizeof(T)); - memset(0, sizeof(int), 0); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: memset(0, 0, sizeof(int)); -} - -void foo(void *a, int xsize, int ysize) { - memset(a, sizeof(int), 0); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: memset(a, 0, sizeof(int)); -#define M memset(a, sizeof(int), 0); - M -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: #define M memset(a, sizeof(int), 0); - ::memset(a, xsize * - ysize, 0); -// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: ::memset(a, 0, xsize * -// CHECK-FIXES-NEXT: ysize); - std::memset(a, sizeof(int), 0x00); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: std::memset(a, 0x00, sizeof(int)); - - const int v = 0; - memset(a, sizeof(int), v); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: memset(a, v, sizeof(int)); - - memset(a, sizeof(int), v + v); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: memset of size zero, potentially swapped argument -// CHECK-FIXES: memset(a, v + v, sizeof(int)); - - memset(a, sizeof(int), v + 1); - - memset(a, -1, sizeof(int)); - memset(a, 0xcd, 1); - - // Don't warn when the fill char and the length are both known to be - // zero. No bug is possible. - memset(a, 0, v); - memset(a, v, 0); - - // -1 is clearly not a length by virtue of being negative, so no warning - // despite v == 0. - memset(a, -1, v); - - memtmpl<0, int>(); -}