Adding a checker (misc-non-copyable-objects) that detects situations where a non-copyable C type is being dereferenced, such as FILE or pthread_mutex_t. Corresponds to the CERT C++ secure coding rule: https://www.securecoding.cert.org/confluence/display/c/FIO38-C.+Do+not+copy+a+FILE+object

llvm-svn: 248907

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

@@ -13,6 +13,7 @@ add_clang_library(clangTidyMiscModule
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
+  NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

@@ -21,6 +21,7 @@
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
+#include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
@@ -58,8 +59,9 @@ public:
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
-    CheckFactories.registerCheck<SizeofContainerCheck>(
-        "misc-sizeof-container");
+    CheckFactories.registerCheck<NonCopyableObjectsCheck>(
+        "misc-non-copyable-objects");
+    CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(

clang-tools-extra/clang-tidy/misc/NonCopyableObjects.cpp

@@ -0,0 +1,96 @@
+//===--- NonCopyableObjects.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 "NonCopyableObjects.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace {
+// FIXME: it would be good to make a list that is also user-configurable so that
+// users can add their own elements to the list. However, it may require some
+// extra thought since POSIX types and FILE types are usable in different ways.
+bool isPOSIXTypeName(StringRef ClassName) {
+  static const char *TypeNames[] = {
+    "::pthread_cond_t",
+    "::pthread_mutex_t",
+    "pthread_cond_t",
+    "pthread_mutex_t"
+  };
+  return std::binary_search(std::begin(TypeNames), std::end(TypeNames),
+                            ClassName);
+}
+
+bool isFILETypeName(StringRef ClassName) {
+  static const char *TypeNames[] = {
+    "::FILE",
+    "FILE",
+    "std::FILE"
+  };
+  return std::binary_search(std::begin(TypeNames), std::end(TypeNames),
+                            ClassName);
+}
+
+AST_MATCHER(NamedDecl, isFILEType) {
+  return isFILETypeName(Node.getQualifiedNameAsString());
+}
+
+AST_MATCHER(NamedDecl, isPOSIXType) {
+  return isPOSIXTypeName(Node.getQualifiedNameAsString());
+}
+} // namespace
+
+namespace tidy {
+void NonCopyableObjectsCheck::registerMatchers(MatchFinder *Finder) {
+  // There are two ways to get into trouble with objects like FILE *:
+  // dereferencing the pointer type to be a non-pointer type, and declaring
+  // the type as a non-pointer type in the first place. While the declaration
+  // itself could technically be well-formed in the case where the type is not
+  // an opaque type, it's highly suspicious behavior.
+  //
+  // POSIX types are a bit different in that it's reasonable to declare a
+  // non-pointer variable or data member of the type, but it is not reasonable
+  // to dereference a pointer to the type, or declare a parameter of non-pointer
+  // type.
+  auto BadFILEType = hasType(namedDecl(isFILEType()).bind("type_decl"));
+  auto BadPOSIXType = hasType(namedDecl(isPOSIXType()).bind("type_decl"));
+  auto BadEitherType = anyOf(BadFILEType, BadPOSIXType);
+
+  Finder->addMatcher(
+      namedDecl(anyOf(varDecl(BadFILEType), fieldDecl(BadFILEType)))
+          .bind("decl"),
+      this);
+  Finder->addMatcher(parmVarDecl(BadPOSIXType).bind("decl"), this);
+  Finder->addMatcher(
+      expr(unaryOperator(hasOperatorName("*"), BadEitherType)).bind("expr"),
+      this);
+}
+
+void NonCopyableObjectsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *D = Result.Nodes.getNodeAs<NamedDecl>("decl");
+  const auto *BD = Result.Nodes.getNodeAs<NamedDecl>("type_decl");
+  const auto *E = Result.Nodes.getNodeAs<Expr>("expr");
+
+  if (D && BD)
+    diag(D->getLocation(), "'%0' declared as type '%1', which is unsafe to copy"
+         "; did you mean '%1 *'?")
+    << D->getName() << BD->getName();
+  else if (E)
+    diag(E->getExprLoc(),
+         "expression has opaque data structure type '%0'; type should only be "
+         "used as a pointer and not dereferenced")
+        << BD->getName();
+}
+
+} // namespace tidy
+} // namespace clang
+

clang-tools-extra/clang-tidy/misc/NonCopyableObjects.h

@@ -0,0 +1,31 @@
+//===--- NonCopyableObjects.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_MISC_NONCOPYABLEOBJECTS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONCOPYABLEOBJECTS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// The check flags dereferences and non-pointer declarations of objects that
+/// are not meant to be passed by value, such as C FILE objects.
+class NonCopyableObjectsCheck : public ClangTidyCheck {
+public:
+  NonCopyableObjectsCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONCOPYABLEOBJECTS_H

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

@@ -32,6 +32,7 @@ List of clang-tidy Checks
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
+   misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
    misc-swapped-arguments

clang-tools-extra/docs/clang-tidy/checks/misc-non-copyable-objects.rst

@@ -0,0 +1,9 @@
+misc-non-copyable-objects
+=========================
+
+The check flags dereferences and non-pointer declarations of objects that are
+not meant to be passed by value, such as C FILE objects or POSIX
+pthread_mutex_t objects.
+
+This check corresponds to CERT C++ Coding Standard rule `FIO38-C. Do not copy a FILE object
+<https://www.securecoding.cert.org/confluence/display/c/FIO38-C.+Do+not+copy+a+FILE+object>`_.

clang-tools-extra/test/clang-tidy/misc-non-copyable-objects.c

@@ -0,0 +1,43 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-non-copyable-objects %t
+
+typedef struct FILE {} FILE;
+typedef struct pthread_cond_t {} pthread_cond_t;
+typedef int pthread_mutex_t;
+
+// CHECK-MESSAGES: :[[@LINE+1]]:13: warning: 'f' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'? [misc-non-copyable-objects]
+void g(FILE f);
+// CHECK-MESSAGES: :[[@LINE+1]]:24: warning: 'm' declared as type 'pthread_mutex_t', which is unsafe to copy; did you mean 'pthread_mutex_t *'?
+void h(pthread_mutex_t m);
+// CHECK-MESSAGES: :[[@LINE+1]]:23: warning: 'c' declared as type 'pthread_cond_t', which is unsafe to copy; did you mean 'pthread_cond_t *'?
+void i(pthread_cond_t c);
+
+struct S {
+  pthread_cond_t c; // ok
+  // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: 'f' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  FILE f;
+};
+
+void func(FILE *f) {
+  // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: 'f1' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  FILE f1; // match
+  // CHECK-MESSAGES: :[[@LINE+2]]:8: warning: 'f2' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  // CHECK-MESSAGES: :[[@LINE+1]]:13: warning: expression has opaque data structure type 'FILE'; type should only be used as a pointer and not dereferenced
+  FILE f2 = *f;
+  // CHECK-MESSAGES: :[[@LINE+1]]:15: warning: 'f3' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  struct FILE f3;
+  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: expression has opaque data structure type 'FILE'; type should only be used as a pointer and not dereferenced
+  (void)sizeof(*f);
+  (void)sizeof(FILE);
+  // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: expression has opaque data structure type 'FILE'; type should only be used as a pointer and not dereferenced
+  g(*f);
+
+  pthread_mutex_t m; // ok
+  h(m); // ok
+
+  pthread_cond_t c; // ok
+  i(c); // ok
+
+  pthread_mutex_t *m1 = &m; // ok
+  // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: expression has opaque data structure type 'pthread_mutex_t'; type should only be used as a pointer and not dereferenced
+  h(*m1);
+}

clang-tools-extra/test/clang-tidy/misc-non-copyable-objects.cpp

@@ -0,0 +1,26 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-non-copyable-objects %t
+
+namespace std {
+typedef struct FILE {} FILE;
+}
+using namespace std;
+
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: 'f' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'? [misc-non-copyable-objects]
+void g(std::FILE f);
+
+struct S {
+  // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: 'f' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  ::FILE f;
+};
+
+void func(FILE *f) {
+  // CHECK-MESSAGES: :[[@LINE+1]]:13: warning: 'f1' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  std::FILE f1; // match
+  // CHECK-MESSAGES: :[[@LINE+2]]:10: warning: 'f2' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  // CHECK-MESSAGES: :[[@LINE+1]]:15: warning: expression has opaque data structure type 'FILE'; type should only be used as a pointer and not dereferenced
+  ::FILE f2 = *f; // match, match
+  // CHECK-MESSAGES: :[[@LINE+1]]:15: warning: 'f3' declared as type 'FILE', which is unsafe to copy; did you mean 'FILE *'?
+  struct FILE f3; // match
+  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: expression has opaque data structure type 'FILE'; type should only be used as a pointer and not dereferenced
+  (void)sizeof(*f); // match
+}