[clang-tidy] Add bugprone-undefined-memory-manipulation check.

[rnkovacs]

Summary

Finds calls of memory manipulation functions memset(), memcpy() and memmove() on not TriviallyCopyable objects, as these always result in an undefined behavior.

Moved to module bugprone created by D32700.

Hits on LLVM codebase:

undef-mem-manip-llvm.txt766 BDownload

[alexfh]

As usual, please run the check at least on LLVM+Clang and include a brief summary of results into the description of the patch.

[alexfh]

Another thing is that at this point I'd already create a bugprone module (if nobody has a better name) and place the check there (together with the other memset-related check).

[rnkovacs]

• Moved to module bugprone.
• Added summary of hits on LLVM in the description.

[alexfh]

clang-tidy/bugprone/UndefinedMemoryManipulationCheck.cpp

22

nit: No need for the parentheses.

39

What about memset?

test/clang-tidy/bugprone-undefined-memory-manipulation.cpp

34

Remove stray semicolons after closing braces of function bodies.

[rnkovacs]

Removed redundant parens and stray semicolons.

[rnkovacs]

clang-tidy/bugprone/UndefinedMemoryManipulationCheck.cpp

39

memset has a bit different signature than the other two. It has a fill value as its second argument instead of a source object.

[alexfh]

LG

clang-tidy/bugprone/UndefinedMemoryManipulationCheck.cpp

39

Ah, now I get it. We're inspecting the second argument here.

[alexfh]

FYI, bugprone-undefined-memory-manipulation crashes on some of our code. I guess, this happens with dependent types, but I don't have an isolated repro yet.

[alexfh]

The top of stack trace is:

clang::CXXRecordDecl::isTriviallyCopyable()
(unknown)
clang::tidy::bugprone::(anonymous namespace)::internal::matcher_isNotTriviallyCopyableMatcher::matches()

[rnkovacs]

Dependent types seem to work, but we did manage to produce a crash on incomplete types. I created D35790 for that. I hope it's the same problem you encountered.

clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "SuspiciousMemsetUsageCheck.h"
+#include "UndefinedMemoryManipulationCheck.h"
 
 namespace clang {
 namespace tidy {

   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
+    CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
+        "bugprone-undefined-memory-manipulation");
   }
 };
 

clang-tidy/bugprone/CMakeLists.txt

 add_clang_library(clangTidyBugproneModule
   BugproneTidyModule.cpp
   SuspiciousMemsetUsageCheck.cpp
+  UndefinedMemoryManipulationCheck.cpp
 
   LINK_LIBS
   clangAnalysis

clang-tidy/bugprone/UndefinedMemoryManipulationCheck.h

+//===--- UndefinedMemoryManipulationCheck.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_UNDEFINED_MEMORY_MANIPULATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNDEFINED_MEMORY_MANIPULATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds calls of memory manipulation functions ``memset()``, ``memcpy()`` and
+/// ``memmove()`` on not TriviallyCopyable objects resulting in undefined
+/// behavior.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-undefined-memory-manipulation.html
+class UndefinedMemoryManipulationCheck : public ClangTidyCheck {
+public:
+  UndefinedMemoryManipulationCheck(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_UNDEFINED_MEMORY_MANIPULATION_H

clang-tidy/bugprone/UndefinedMemoryManipulationCheck.cpp

+//===--- UndefinedMemoryManipulationCheck.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 "UndefinedMemoryManipulationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+AST_MATCHER(CXXRecordDecl, isNotTriviallyCopyable) {
+  return !(Node.isTriviallyCopyable());
+}
+} // namespace
+
+void UndefinedMemoryManipulationCheck::registerMatchers(MatchFinder *Finder) {
+  const auto NotTriviallyCopyableObject =
+      hasType(pointsTo(cxxRecordDecl(isNotTriviallyCopyable())));
+
+  // Check whether destination object is not TriviallyCopyable.
+  // Applicable to all three memory manipulation functions.
+  Finder->addMatcher(callExpr(callee(functionDecl(hasAnyName(
+                                  "::memset", "::memcpy", "::memmove"))),
+                              hasArgument(0, NotTriviallyCopyableObject))
+                         .bind("dest"),
+                     this);
+
+  // Check whether source object is not TriviallyCopyable.
+  // Only applicable to memcpy() and memmove().
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasAnyName("::memcpy", "::memmove"))),
+               hasArgument(1, NotTriviallyCopyableObject))
+          .bind("src"),
+      this);
+}
+
+void UndefinedMemoryManipulationCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *Destination = Result.Nodes.getNodeAs<CallExpr>("dest")) {
+    diag(Destination->getLocStart(), "undefined behavior, destination "
+                                     "object is not TriviallyCopyable");
+  }
+  if (const auto *Source = Result.Nodes.getNodeAs<CallExpr>("src")) {
+    diag(Source->getLocStart(), "undefined behavior, source object is not "
+                                "TriviallyCopyable");
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

   Finds ``memset()`` calls with potential mistakes in their arguments.
   Replaces and extends the ``google-runtime-memset`` check.
 
+- New `bugprone-undefined-memory-manipulation
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-undefined-memory-manipulation.html>`_ check
+
+  Finds calls of memory manipulation functions ``memset()``, ``memcpy()`` and
+  ``memmove()`` on not TriviallyCopyable objects resulting in undefined behavior.
+
 - New `cert-dcl21-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl21-cpp.html>`_ check
 

docs/clang-tidy/checks/bugprone-undefined-memory-manipulation.rst

+.. title:: clang-tidy - bugprone-undefined-memory-manipulation
+
+bugprone-undefined-memory-manipulation
+==================================
+
+Finds calls of memory manipulation functions ``memset()``, ``memcpy()`` and
+``memmove()`` on not TriviallyCopyable objects resulting in undefined behavior.

docs/clang-tidy/checks/list.rst

    android-cloexec-open
    boost-use-to-string
    bugprone-suspicious-memset-usage
+   bugprone-undefined-memory-manipulation
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp

test/clang-tidy/bugprone-undefined-memory-manipulation.cpp

+// RUN: %check_clang_tidy %s bugprone-undefined-memory-manipulation %t
+
+void *memset(void *, int, __SIZE_TYPE__);
+void *memcpy(void *, const void *, __SIZE_TYPE__);
+void *memmove(void *, const void *, __SIZE_TYPE__);
+
+namespace std {
+using ::memcpy;
+using ::memmove;
+using ::memset;
+}
+
+// TriviallyCopyable types:
+struct Plain {
+  int n;
+};
+
+enum E {
+  X,
+  Y,
+  Z
+};
+
+struct Base {
+  float b;
+};
+
+struct Derived : Base {
+  bool d;
+};
+
+// not TriviallyCopyable types:
+struct Destruct {
+  ~Destruct() {};
+};
+
+struct Copy {
+  Copy() {};
+  Copy(const Copy &) {};
+};
+
+struct Move {
+  Move() {};
+  Move(Move &&) {};
+};
+
+struct VirtualFunc {
+  virtual void f() {};
+};
+
+struct VirtualBase : virtual Base {
+  int vb;
+};
+
+template <typename T>
+void memset_temp(T *b) {
+  memset(b, 0, sizeof(T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+}
+
+template <typename S, typename T>
+void memcpy_temp(S *a, T *b) {
+  memcpy(a, b, sizeof(T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+}
+
+template <typename S, typename T>
+void memmove_temp(S *a, T *b) {
+  memmove(a, b, sizeof(T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+}
+
+void notTriviallyCopyable() {
+  Plain p; // TriviallyCopyable for variety
+  Destruct d;
+  Copy c;
+  Move m;
+  VirtualFunc vf;
+  VirtualBase vb;
+
+  memset(&vf, 0, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  memset(&d, 0, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  memset(&c, 0, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  std::memset(&m, 0, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  ::memset(&vb, 0, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+
+  memcpy(&p, &vf, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  memcpy(&p, &d, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  memcpy(&c, &p, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  std::memcpy(&m, &p, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  ::memcpy(&vb, &p, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+
+  memmove(&vf, &p, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  memmove(&d, &p, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  memmove(&p, &c, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  std::memmove(&p, &m, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+  ::memmove(&p, &vb, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+
+#define MEMSET memset(&vf, 0, sizeof(int));
+  MEMSET
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+#define MEMCPY memcpy(&d, &p, sizeof(int));
+  MEMCPY
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, destination object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+#define MEMMOVE memmove(&p, &c, sizeof(int));
+  MEMMOVE
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: undefined behavior, source object is not TriviallyCopyable [bugprone-undefined-memory-manipulation]
+
+  memset_temp<VirtualFunc>(&vf);
+  memcpy_temp<Plain, VirtualFunc>(&p, &vf);
+  memmove_temp<Plain, VirtualFunc>(&p, &vf);
+}
+
+void triviallyCopyable() {
+  Plain p;
+  Base base;
+  Derived derived;
+
+  int i = 5;
+  int ia[3] = {1, 2, 3};
+  float f = 3.14;
+  float fa[3] = {1.1, 2.2, 3.3};
+  bool b = false;
+  bool ba[2] = {true, false};
+  E e = X;
+  p.n = 2;
+
+  memset(&p, 0, sizeof(int));
+  memset(&base, 0, sizeof(float));
+  memset(&derived, 0, sizeof(bool));
+  memset(&i, 0, sizeof(int));
+  memset(ia, 0, sizeof(int));
+  memset(&f, 0, sizeof(float));
+  memset(fa, 0, sizeof(float));
+  memset(&b, 0, sizeof(bool));
+  memset(ba, 0, sizeof(bool));
+  memset(&e, 0, sizeof(int));
+  memset(&p.n, 0, sizeof(int));
+
+  memcpy(&p, &p, sizeof(int));
+  memcpy(&base, &base, sizeof(float));
+  memcpy(&derived, &derived, sizeof(bool));
+  memcpy(&i, &i, sizeof(int));
+  memcpy(ia, ia, sizeof(int));
+  memcpy(&f, &f, sizeof(float));
+  memcpy(fa, fa, sizeof(float));
+  memcpy(&b, &b, sizeof(bool));
+  memcpy(ba, ba, sizeof(bool));
+  memcpy(&e, &e, sizeof(int));
+  memcpy(&p.n, &p.n, sizeof(int));
+
+  memmove(&p, &p, sizeof(int));
+  memmove(&base, &base, sizeof(float));
+  memmove(&derived, &derived, sizeof(bool));
+  memmove(&i, &i, sizeof(int));
+  memmove(ia, ia, sizeof(int));
+  memmove(&f, &f, sizeof(float));
+  memmove(fa, fa, sizeof(float));
+  memmove(&b, &b, sizeof(bool));
+  memmove(ba, ba, sizeof(bool));
+  memmove(&e, &e, sizeof(int));
+  memmove(&p.n, &p.n, sizeof(int));
+}