[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

4 files changed, 216 insertions, 0 deletions

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 00000000000..65353a23c7a
--- /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<SuspiciousMemsetUsageCheck>(
+        "bugprone-suspicious-memset-usage");
+  }
+};
+
+} // namespace bugprone
+
+// Register the BugproneTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<bugprone::BugproneModule>
+    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 00000000000..1f0ec6e0427
--- /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 00000000000..8e11a435174
--- /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<unsigned>('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<unsigned>('0'))),
+                                           integerLiteral()))),
+               unless(isInTemplateInstantiation()))
+          .bind("call"),
+      this);
+}
+
+void SuspiciousMemsetUsageCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *CharZeroFill =
+          Result.Nodes.getNodeAs<CharacterLiteral>("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<IntegerLiteral>("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<CallExpr>("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 00000000000..1c0d1bcf3e9
--- /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