[PATCH] Add checker for objects that should not be value types

[aaron.ballman]

Summary

Some object types are not meant to be used as a value type in all circumstances, but the language (mainly C) does not provide facilities for marking these types as noncopyable. For instance, you should never pass a FILE object by value, but only by pointer. This seems to fall into two category of types:

A FILE type should never be declared by value (whether as a local, a field, or a parameter) and such a pointer should never be dereferenced.
Some POSIX types, like pthread_mutex_t can be declared by value as a variable or field, but should not be declared by value as a parameter, and such a pointer should never be dereferenced.

This patch adds a checker (misc-non-copyable-objects, but a better moniker would not make me sad) to catch code that does this accidentally. This corresponds (at least for treatment of FILE) to: https://www.securecoding.cert.org/confluence/display/c/FIO38-C.+Do+not+copy+a+FILE+object

~Aaron

[aaron.ballman]

Ping?

[alexfh]

Please add a documentation file.

clang-tidy/misc/NonCopyableObjects.cpp

22

How about making these lists configurable or adding a list for custom type names that should be checked in a similar way?

82

I think, error messages should contain some explanation of why is this wrong. Not sure if this can be fit into a reasonable number of words, but we have to try.

[aaron.ballman]

clang-tidy/misc/NonCopyableObjects.cpp

22

Do we have a helper function for making lists like these configurable? If so, I'll gladly use it. If not, perhaps we could make some helper functionality and then implement configurability at that time in a more comprehensive way?

82

Excellent point! How about:

'%0' declared as unsafely-copyable type '%1'; did you mean '%1 *'

or

'%0' declared as type '%1', which is unsafe to copy' did you mean '%1 *'?

[alexfh]

clang-tidy/misc/NonCopyableObjects.cpp

22

I tried to make clang-tidy checks configurable in a more type-safe way (http://reviews.llvm.org/D5602), but never got time to complete this. So no, currently we don't have any facilities to make this kind of configuration easier. Making these lists configurable is also not a precondition to submitting this patch. It was just an idea of an improvement.

82

The latter seems easier to read. Thanks!

[aaron.ballman]

Addresses review comments, also adds a documentation file.

[alexfh]

clang-tidy/misc/NonCopyableObjects.cpp

88

What's a "suspicious type" and why should the user know about this? Should the message explain better what's wrong and what can be done about that?

[aaron.ballman]

clang-tidy/misc/NonCopyableObjects.cpp

88

I was trying to find better wording for this, but it's really hard. This comes from dereferencing a type that should only be used as an opaque pointer type. eg) memcpy(some_buffer, *some_pthread_mutex_t, sizeof(pthread_mutex_t));

Perhaps:

expression has opaque data structure type '%0'; do not rely on internal implementation details

or something to that effect?

[alexfh]

One of the error messages still needs to be made more clear. Otherwise looks good.

Thank you!

clang-tidy/misc/NonCopyableObjects.cpp

88

That's much better, but the "do not rely on internal implementation details" is still not very clear. Maybe add that the type should only be used as a pointer and should never be dereferenced in the user code?

[aaron.ballman]

I settled on "expression has opaque data structure type 'FILE'; type should only be used as a pointer and not dereferenced" but we can tweak if that still isn't quite right.

Committed in r248907.

Thank you for the reviews!

~Aaron

[alexfh]

In D12945#256509, @aaron.ballman wrote:

I settled on "expression has opaque data structure type 'FILE'; type should only be used as a pointer and not dereferenced" but we can tweak if that still isn't quite right.

Seems good. Thank you!

clang-tidy/misc/CMakeLists.txt

   MiscTidyModule.cpp
   MoveConstructorInitCheck.cpp
   NoexceptMoveConstructorCheck.cpp
+  NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
+#include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"

         "misc-move-constructor-init");
     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-tidy/misc/NonCopyableObjects.h

+//===--- 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-tidy/misc/NonCopyableObjects.cpp

+//===--- 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 suspicious type '%0'")
+        << BD->getName();
+}
+
+} // namespace tidy
+} // namespace clang
+

docs/clang-tidy/checks/list.rst

    misc-macro-repeated-side-effects
    misc-move-constructor-init
    misc-noexcept-move-constructor
+   misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
    misc-swapped-arguments

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

+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.

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

+// 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 suspicious type 'FILE'
+  ::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 suspicious type 'FILE'
+  (void)sizeof(*f); // match
+}