[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 ↗

(On Diff #106185)

nit: No need for the parentheses.

39 ↗

(On Diff #106185)

What about memset?

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

34 ↗

(On Diff #106185)

Remove stray semicolons after closing braces of function bodies.

[rnkovacs]

Removed redundant parens and stray semicolons.

[rnkovacs]

clang-tidy/bugprone/UndefinedMemoryManipulationCheck.cpp

39 ↗

(On Diff #106185)

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 ↗

(On Diff #106185)

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-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp

 //===--- 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"
+#include "UndefinedMemoryManipulationCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 class BugproneModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
+    CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
+        "bugprone-undefined-memory-manipulation");
   }
 };
 
 } // 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

clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   BugproneTidyModule.cpp
   SuspiciousMemsetUsageCheck.cpp
+  UndefinedMemoryManipulationCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/trunk/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-tools-extra/trunk/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

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- New `android-cloexec-socket
   ``socket()``.
 
 - New `bugprone-suspicious-memset-usage
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-suspicious-memset-usage.html>`_ check
 
   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
 
   Checks if the overloaded postfix ``operator++/--`` returns a constant object.
 
 - New `cert-dcl58-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl58-cpp.html>`_ check
 

clang-tools-extra/trunk/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.

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    android-cloexec-creat
    android-cloexec-fopen
    android-cloexec-open
    android-cloexec-socket
    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
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c
    cert-err09-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err09-cpp>

clang-tools-extra/trunk/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));
+}