Clang plugin to check that unstable types are not used in IPC.

tools/clang/plugins/CheckIPCVisitor.cpp

Index: tools/clang/plugins/CheckIPCVisitor.cpp
diff --git a/tools/clang/plugins/CheckIPCVisitor.cpp b/tools/clang/plugins/CheckIPCVisitor.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..8024bfd2f2423d098a2a1c1bb74f2a89e94275e4
--- /dev/null
+++ b/tools/clang/plugins/CheckIPCVisitor.cpp
@@ -0,0 +1,384 @@
+// Copyright (c) 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "CheckIPCVisitor.h"
+
+#include "llvm/ADT/StringSet.h"
+
+using namespace clang;
+
+namespace chrome_checker {
+
+namespace {
+
+const char kWriteParamBadType[] =
+    "[chromium-ipc] IPC::WriteParam() is called on blacklisted type '%0'%1.";
+
+const char kTupleBadType[] =
+    "[chromium-ipc] IPC tuple references banned type '%0'%1.";
+
+const char kWriteParamTemplateArg[] =
+    "[chromium-ipc] IPC::WriteParam() must explicitly specify template "
+    "argument (use WriteParam<T>(...)).";
+
+const char kWriteParamWrongTemplate[] =
+    "[chromium-ipc] IPC::WriteParam() can only be used in IPC::ParamTraits<> "
+    "templates.";
+
+const char kWriteParamBadSignature[] =
+    "[chromium-ipc] IPC::WriteParam() is expected to have two arguments.";
+
+const char kNoteSeeHere[] =
+    "";

[dcheng, 2016/02/23 19:46:46]

Add some content to this string?

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+
+struct CheckDetails {
+  QualType entry_type;
+  QualType exit_type;
+  std::vector<const TypedefType*> typedefs;
+};
+
+bool IsBlacklistedType(ASTContext& context, QualType type) {
+  return context.hasSameUnqualifiedType(type, context.LongTy) ||
+      context.hasSameUnqualifiedType(type, context.UnsignedLongTy);
+}
+
+bool IsBlacklistedTypedef(const TypedefNameDecl* tdef) {
+  static llvm::StringSet<> blacklist({

[dcheng, 2016/02/23 19:46:46]

No static initializers. Just make this an instance var?

[Dmitry Skiba, 2016/02/24 14:39:04]

TBD

+      "intmax_t",
+      "uintmax_t",
+      "intptr_t",
+      "uintptr_t",
+      "wint_t",
+      "size_t",
+      "rsize_t",
+      "ssize_t",
+      "ptrdiff_t",
+      "dev_t",
+      "off_t",
+      "clock_t",
+      "time_t",
+      "suseconds_t"
+  });
+  return blacklist.find(tdef->getName()) != blacklist.end();
+}
+
+QualType UnqualifyType(QualType type) {
+  if (type->isReferenceType()) {
+    type = type->getPointeeType();
+  }
+  type = type.getLocalUnqualifiedType();
+  return type;
+}
+
+// Checks that integer type is not blacklisted.
+bool CheckIntegerType(ASTContext& context,
+                      QualType type,
+                      CheckDetails* details = nullptr) {
+  bool seen_typedef = false;
+  while (true) {
+    if (details) {
+      details->exit_type = type;
+    }
+
+    if (auto tdef = dyn_cast<TypedefType>(type)) {

[dcheng, 2016/02/23 19:46:46]

Please qualify auto with * or & as appropriate (it should be auto* in most
places.

Also, why dyn_cast here and getAs<TypedefType> in other places?

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+      if (IsBlacklistedTypedef(tdef->getDecl())) {
+        return false;
+      }
+      if (details) {
+        details->typedefs.push_back(tdef);
+      }
+      seen_typedef = true;
+    }
+
+    QualType desugared_type =
+        type->getLocallyUnqualifiedSingleStepDesugaredType();

[dcheng, 2016/02/23 19:46:46]

The documentation for this says to prefer getSingleStepDesugaredType(): does
that work here?

[Dmitry Skiba, 2016/02/24 14:39:04]

Previously I was calling that function / stripping away quals, but
getLocallyUnqualifiedSingleStepDesugaredType() achieves that in a single step
(actually, getSingleStepDesugaredType() is calling
getLocallyUnqualifiedSingleStepDesugaredType() and then applying quals back).

+    if (desugared_type == type) {
+      break;
+    }
+
+    type = desugared_type;
+  }
+
+  return seen_typedef || !IsBlacklistedType(context, type);
+}
+
+bool CheckTemplateArgument(ASTContext& context,
+                           const TemplateArgument& arg,
+                           CheckDetails* details = nullptr);
+
+bool CheckType(ASTContext& context,
+               QualType type,
+               CheckDetails* details = nullptr) {
+  type = UnqualifyType(type);
+  if (details && details->entry_type == QualType()) {
+    details->entry_type = type;
+  }
+
+  if (type->isIntegerType()) {
+    return CheckIntegerType(context, type, details);
+  }
+
+  if (auto tdef = type->getAs<TypedefType>()) {
+    if (details) {
+      details->typedefs.push_back(tdef);
+    }
+    return CheckType(context, tdef->desugar(), details);
+  }
+
+  if (auto spec = type->getAs<TemplateSpecializationType>()) {
+    for (unsigned i = 0; i != spec->getNumArgs(); ++i) {

[dcheng, 2016/02/23 19:46:46]

I think you can just do for (TemplateArgument* arg : spec)

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+      if (!CheckTemplateArgument(context, spec->getArg(i), details)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  if (auto record = type->getAs<RecordType>()) {
+    auto spec = dyn_cast<ClassTemplateSpecializationDecl>(record->getDecl());

[dcheng, 2016/02/23 19:46:46]

Fold into 140 for consistency.

[Dmitry Skiba, 2016/02/24 14:39:04]

Done.

+    if (spec) {
+      const TemplateArgumentList& args = spec->getTemplateArgs();
+      for (unsigned i = 0; i != args.size(); ++i) {
+        if (!CheckTemplateArgument(context, args[i], details)) {
+          return false;
+        }
+      }
+    }
+    return true;
+  }
+
+  return true;
+}
+
+bool CheckTemplateArgument(ASTContext& context,
+                           const TemplateArgument& arg,
+                           CheckDetails* details) {
+  return arg.getKind() != TemplateArgument::Type ||
+      CheckType(context, arg.getAsType(), details);
+}
+
+void ReportCheckError(clang::DiagnosticsEngine& diagnostics,
+                      const CheckDetails& details,
+                      SourceLocation loc,
+                      unsigned error, unsigned typedef_note) {

[dcheng, 2016/02/23 19:46:46]

What's the point of typedef_note: aren't we always passing the same thing in?

[Dmitry Skiba, 2016/02/24 14:39:04]

Yes, but it's not known here, since ReportCheckError() is not inside
CheckIPCVisitor. However, it seems that I'll move everything into the class, so
this function will look different.

+  std::string entry_type = details.entry_type.getAsString();
+  std::string exit_type = details.exit_type.getAsString();
+
+  std::string via;
+  if (entry_type != exit_type) {
+    via = " via '" + entry_type + "'";
+  }
+  diagnostics.Report(loc, error) << exit_type << via;
+
+  for (const TypedefType* tdef: details.typedefs) {
+    diagnostics.Report(tdef->getDecl()->getLocation(), typedef_note);
+  }
+}
+
+}  // namespace
+
+CheckIPCVisitor::CheckIPCVisitor(CompilerInstance& compiler)
+  : compiler_(compiler), context_(nullptr) {
+  auto& diagnostics = compiler_.getDiagnostics();
+  error_write_param_bad_type_ = diagnostics.getCustomDiagID(
+      DiagnosticsEngine::Error, kWriteParamBadType);
+  error_tuple_bad_type_ = diagnostics.getCustomDiagID(
+      DiagnosticsEngine::Error, kTupleBadType);
+  error_write_param_template_arg_ = diagnostics.getCustomDiagID(
+      DiagnosticsEngine::Error, kWriteParamTemplateArg);
+  error_write_param_wrong_template_ = diagnostics.getCustomDiagID(
+      DiagnosticsEngine::Error, kWriteParamWrongTemplate);
+  error_write_param_bad_signature_ = diagnostics.getCustomDiagID(
+      DiagnosticsEngine::Error, kWriteParamBadSignature);
+  note_see_here_ = diagnostics.getCustomDiagID(
+      DiagnosticsEngine::Note, kNoteSeeHere);
+}
+
+void CheckIPCVisitor::BeginDecl(Decl* decl) {
+  decl_stack_.push_back(decl);
+}
+
+void CheckIPCVisitor::EndDecl() {
+  decl_stack_.pop_back();
+}
+
+void CheckIPCVisitor::VisitTemplateSpecializationType(
+    TemplateSpecializationType* spec) {
+  ValidateCheckedTuple(spec);
+}
+
+void CheckIPCVisitor::VisitCallExpr(CallExpr* call_expr) {
+  ValidateWriteParam(call_expr);
+}
+
+bool CheckIPCVisitor::ValidateWriteParam(const CallExpr* call_expr) const {
+  if (auto lookup_expr = dyn_cast<UnresolvedLookupExpr>(
+          call_expr->getCallee())) {
+    return ValidateWriteParamInsideTemplate(call_expr, lookup_expr);
+  }
+
+  const FunctionDecl* callee_decl = call_expr->getDirectCallee();
+  if (!callee_decl ||
+      callee_decl->getQualifiedNameAsString() != "IPC::WriteParam") {
+    return true;
+  }
+
+  return ValidateWriteParamSignature(call_expr) &&
+      ValidateWriteParamCaller(call_expr) &&
+      ValidateWriteParamArgument(call_expr->getArg(1));
+}
+
+// Checks that IPC::WriteParam() has expected signature.
+bool CheckIPCVisitor::ValidateWriteParamSignature(
+    const CallExpr* call_expr) const {
+  if (call_expr->getNumArgs() != 2) {
+    compiler_.getDiagnostics().Report(
+        call_expr->getExprLoc(), error_write_param_bad_signature_);
+    return false;
+  }
+  return true;
+}
+
+// Checks that when WriteParam() is used inside a template, it explicitly
+// depends on template argument: WriteParam<T>(...)
+bool CheckIPCVisitor::ValidateWriteParamInsideTemplate(
+    const CallExpr* call_expr,
+    const UnresolvedLookupExpr* lookup_expr) const {
+  if (!IsInsideParamTraits() ||
+      lookup_expr->getName().getAsString() != "WriteParam") {
+    return true;
+  }
+
+  if (!ValidateWriteParamSignature(call_expr)) {
+    return false;
+  }
+
+  if (lookup_expr->getNumTemplateArgs() == 1) {
+    const TemplateArgument& template_arg =
+        lookup_expr->getTemplateArgs()->getArgument();
+    bool explicitly_dependent =
+        template_arg.getKind() == TemplateArgument::Type &&
+        isa<TemplateTypeParmType>(template_arg.getAsType());
+    if (explicitly_dependent) {
+      return true;
+    }
+  }
+
+  compiler_.getDiagnostics().Report(
+      call_expr->getExprLoc(), error_write_param_template_arg_);
+  return false;
+}
+
+// Checks that IPC::WriteParam() argument type is allowed.
+// See CheckType() above for specifics.
+bool CheckIPCVisitor::ValidateWriteParamArgument(const Expr* arg_expr) const {
+  auto parent_fn_decl = GetParentDecl<FunctionDecl>();
+  if (parent_fn_decl &&
+      parent_fn_decl->getTemplatedKind() != FunctionDecl::TK_NonTemplate) {
+    // Skip all specializations - we checked templates earlier in
+    // ValidateWriteParamInsideTemplate().
+    return true;
+  }
+
+  const Expr* arg_type_expr = arg_expr;
+  if (auto tmp_expr = dyn_cast<MaterializeTemporaryExpr>(arg_type_expr)) {
+    arg_type_expr = tmp_expr->GetTemporaryExpr();
+  }
+
+  QualType arg_type;
+  if (auto cast_expr = dyn_cast<ExplicitCastExpr>(arg_type_expr)) {
+    arg_type = cast_expr->getTypeAsWritten();
+  } else {
+    if (auto cast_expr = dyn_cast<ImplicitCastExpr>(arg_type_expr)) {
+      arg_type_expr = cast_expr->getSubExpr();
+    }
+    arg_type = arg_type_expr->getType();
+  }
+
+  if (CheckType(*context_, arg_type)) {
+    return true;
+  }
+
+  CheckDetails details;
+  CheckType(*context_, arg_type, &details);

[dcheng, 2016/02/23 19:46:46]

Why not just pass details in to begin with?

[Dmitry Skiba, 2016/02/24 14:39:04]

It's just an optimization. I expect CheckType() to fail most of the time, and
collecting details involves pushing_back to vector, which I want to avoid.

[dcheng, 2016/02/24 22:57:42]

I'd just use llvm::SmallVector<> and always pass in a CheckDetails. CheckDetails
is stack allocated in this code, and SmallVector has room for a couple inline
elements, so we can avoid spilling to the heap most of the time.

+
+  ReportCheckError(compiler_.getDiagnostics(),
+                   details,
+                   arg_expr->getExprLoc(),
+                   error_write_param_bad_type_, note_see_here_);
+
+  return false;
+}
+
+// Checks that when WriteParam() is called from a template, it's
+// in fact IPC::ParamTraits<> template.
+bool CheckIPCVisitor::ValidateWriteParamCaller(
+    const CallExpr* call_expr) const {
+  auto parent_fn_decl = GetParentDecl<FunctionDecl>();
+  if (!parent_fn_decl ||
+      parent_fn_decl->getTemplatedKind() == FunctionDecl::TK_NonTemplate) {
+    // Skip if we're not in template.
+    return true;
+  }
+
+  // If we're in a template of any kind, check that it's ParamTraits
+  if (IsInsideParamTraits()) {
+    return true;
+  }
+
+  compiler_.getDiagnostics().Report(
+      call_expr->getExprLoc(), error_write_param_wrong_template_);
+  return false;
+}
+
+// Checks that IPC::CheckedTuple<> is specialized with allowed types.
+// See CheckType() above for specifics.
+bool CheckIPCVisitor::ValidateCheckedTuple(
+    const TemplateSpecializationType* spec) const {
+  TemplateDecl* decl = spec->getTemplateName().getAsTemplateDecl();
+  if (!decl || decl->getQualifiedNameAsString() != "IPC::CheckedTuple") {
+    return true;
+  }
+
+  bool valid = true;
+  for (unsigned i = 0; i != spec->getNumArgs(); ++i) {
+    const TemplateArgument& arg = spec->getArg(i);
+    if (CheckTemplateArgument(*context_, arg)) {
+      continue;
+    }
+
+    valid = false;
+
+    CheckDetails details;
+    CheckTemplateArgument(*context_, arg, &details);
+
+    auto parent_decl = GetParentDecl<Decl>();
+    SourceLocation error_loc = parent_decl ?
+        parent_decl->getLocStart() : SourceLocation();
+    ReportCheckError(compiler_.getDiagnostics(),
+                     details,
+                     error_loc,
+                     error_tuple_bad_type_, note_see_here_);
+  }
+
+  return valid;
+}
+
+bool CheckIPCVisitor::IsInsideParamTraits() const {
+  auto parent_spec_decl = GetParentDecl<ClassTemplateSpecializationDecl>();
+  return parent_spec_decl &&
+      parent_spec_decl->getQualifiedNameAsString() == "IPC::ParamTraits";
+}
+
+template <typename T>
+const T* CheckIPCVisitor::GetParentDecl() const {
+  for (auto i = decl_stack_.rbegin(); i != decl_stack_.rend(); ++i) {
+    if (auto parent = dyn_cast_or_null<T>(*i)) {
+      return parent;
+    }
+  }
+  return nullptr;
+}
+
+}  // namespace chrome_checker