[clang-tidy] Add a checker for zero-length memset.

[bkramer]

Summary

If there's memset(x, y, 0) in the code it's most likely a mistake. The
checker suggests a fix-it to swap 'y' and '0'.

I think this has the potential to be promoted into a general clang warning
after some testing in clang-tidy.

[djasper]

clang-tidy/google/MemsetZeroLengthCheck.cpp

32–33

Intuitively, I would have written something like:

unless(hasArgument(1, expr(anyOf(integerLiteral(), characterLiteral())))

Also, I wonder whether we should do this in the callback or inside the callback. In the callback, we could check whether the argument trivially computes to 0 and thereby not only find literal zeros. I am thinking about cases like:

int default_value = 0;
memset(a, 10, default_value);

This is merely a suggestion, though, not sure whether it would lead to false positives.

65

I generally dislike declaration statements defining more than one variable. Here, at lest make it consisten with RHSString and LHSString.

68

Early exit?

if (LHSString.empty() || RHSString.empty())
  return;

clang-tidy/google/MemsetZeroLengthCheck.h

21–22

The sentence doesn't really parse. How about:

This is most likely unintended and the length and value arguments are swapped.

[alexfh]

Other standard functions may suffer from the same problem, e.g. memchr. I think, it makes sense to handle all of them in the same check.

clang-tidy/google/MemsetZeroLengthCheck.cpp

28

Ignore literals in the middle argument.

Why?

[bkramer]

• Make the checker more aggressive by also looking at values that always evaluate to zero.
• Remove the restriction on literals in the middle argument. cpplint did this to reduce false positives from testing code, I don't buy that excuse.
• Cosmetic cleanup.

[djasper]

Looks good.

[bkramer]

Closed by commit rL213155 (authored by d0k).

[alexfh]

clang-tidy/google/MemsetZeroLengthCheck.cpp

61

Maybe we should emit a warning, that this call is a no-op?

65

I'd say Value1 < 0 is also not when we want to swap the arguments.

[bkramer]

clang-tidy/google/MemsetZeroLengthCheck.cpp

61

Fixed.

65

That seems like an incredibly weak heuristic. Zero-sized memsets should not occur in real code :(

clang-tidy/google/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyGoogleModule
   AvoidCStyleCastsCheck.cpp
   ExplicitConstructorCheck.cpp
   ExplicitMakePairCheck.cpp
   GoogleTidyModule.cpp
+  MemsetZeroLengthCheck.cpp
   NamedParameterCheck.cpp
   OverloadedUnaryAndCheck.cpp
   StringReferenceMemberCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/google/GoogleTidyModule.cpp

 //===--- GoogleTidyModule.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 "AvoidCStyleCastsCheck.h"
 #include "ExplicitConstructorCheck.h"
 #include "ExplicitMakePairCheck.h"
+#include "MemsetZeroLengthCheck.h"
 #include "NamedParameterCheck.h"
 #include "OverloadedUnaryAndCheck.h"
 #include "StringReferenceMemberCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
CheckFactories.addCheckFactory(
         new ClangTidyCheckFactory<ExplicitConstructorCheck>());
     CheckFactories.addCheckFactory(
         "google-runtime-operator",
         new ClangTidyCheckFactory<runtime::OverloadedUnaryAndCheck>());
     CheckFactories.addCheckFactory(
         "google-runtime-member-string-references",
         new ClangTidyCheckFactory<runtime::StringReferenceMemberCheck>());
     CheckFactories.addCheckFactory(
+        "google-runtime-memset",
+        new ClangTidyCheckFactory<runtime::MemsetZeroLengthCheck>());
+    CheckFactories.addCheckFactory(
         "google-readability-casting",
         new ClangTidyCheckFactory<readability::AvoidCStyleCastsCheck>());
     CheckFactories.addCheckFactory(
         "google-readability-function",
         new ClangTidyCheckFactory<readability::NamedParameterCheck>());
   }
 };
 

clang-tidy/google/MemsetZeroLengthCheck.h

+//===--- 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_MEMSET_ZERO_LENGTH_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace runtime {
+
+/// \brief Finds calls to memset with a literal zero in the length argument.
+///
+/// This is most likely unintended and the swapped the length and value
+/// arguments.
+///
+/// Corresponding cpplint.py check name: 'runtime/memset'.
+class MemsetZeroLengthCheck : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace runtime
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_MEMSET_ZERO_LENGTH_CHECK_H

clang-tidy/google/MemsetZeroLengthCheck.cpp

+//===--- 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/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace runtime {
+
+void
+MemsetZeroLengthCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
+  auto InTemplateInstantiation = hasAncestor(
+      decl(anyOf(recordDecl(ast_matchers::isTemplateInstantiation()),
+                 functionDecl(ast_matchers::isTemplateInstantiation()))));
+  // Look for memset(x, y, 0) as those is most likely an argument swap. Ignore
+  // literals in the middle argument.
+  Finder->addMatcher(callExpr(callee(functionDecl(hasName("::memset"))),
+                              argumentCountIs(3),
+                              hasArgument(2, integerLiteral(equals(0))),
+                              unless(hasArgument(1, integerLiteral())),
+                              unless(hasArgument(1, characterLiteral())),
+                              unless(InTemplateInstantiation)).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<CallExpr>("decl");
+
+  const Expr *Arg1 = Call->getArg(1);
+  const Expr *Arg2 = Call->getArg(2);
+
+  // 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(),
+              RHSRange = Arg2->getSourceRange();
+  StringRef RHSString = getAsString(Result, RHSRange);
+  StringRef LHSString = getAsString(Result, LHSRange);
+  if (!LHSString.empty() && !RHSString.empty()) {
+    D << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(LHSRange),
+                                      RHSString)
+      << FixItHint::CreateReplacement(CharSourceRange::getTokenRange(RHSRange),
+                                      LHSString);
+  }
+}
+
+} // namespace runtime
+} // namespace tidy
+} // namespace clang

test/clang-tidy/google-memset-zero-length.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s google-runtime-memset %t
+// REQUIRES: shell
+
+void *memset(void *, int, __SIZE_TYPE__);
+
+namespace std {
+  using ::memset;
+}
+
+template <int i>
+void memtmpl() {
+  memset(0, sizeof(int), i);
+  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));
+  memset(a, -1, sizeof(int));
+  memset(a, 0xcd, 0);
+  memset(a, 'a', 0);
+
+  memtmpl<0>();
+}