[clang-tidy] Add new checker for suspicious sizeof expressions

Summary:
This check is finding suspicious cases of sizeof expression.

Sizeof expression is returning the size (in bytes) of a type or an
expression. Programmers often abuse or misuse this expression.

This checker is adding common set of patterns to detect some
of these bad constructs.


Some examples found by this checker:

R/packages/ifultools/ifultools/src/fra_neig.c
```
        /* free buffer memory */
        (void) mutil_free( dist_buff, sizeof( ctr * sizeof( double ) ) );
        (void) mutil_free( nidx_buff, sizeof( ctr * sizeof( sint32 ) ) );
```


graphviz/v2_20_2/lib/common/utils.c
```
static Dtdisc_t mapDisc = {
    offsetof(item, p),
    sizeof(2 * sizeof(void *)),
    offsetof(item, link),
    (Dtmake_f) newItem,
    (Dtfree_f) freeItem,
    (Dtcompar_f) cmpItem,
    NIL(Dthash_f),
    NIL(Dtmemory_f),
    NIL(Dtevent_f)
};
```


mDNSResponder/mDNSShared/dnsextd.c
```
	context = ( TCPContext* ) malloc( sizeof( TCPContext ) );
	require_action( context, exit, err = mStatus_NoMemoryErr; LogErr( "AcceptTCPConnection", "malloc" ) );
	mDNSPlatformMemZero( context, sizeof( sizeof( TCPContext ) ) );
	context->d		 = self;
```

Reviewers: alexfh

Subscribers: malcolm.parsons, Eugene.Zelenko, cfe-commits

Differential Revision: http://reviews.llvm.org/D19014

llvm-svn: 266451

4 files changed, 309 insertions, 0 deletions

diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index ae0b0dfdb78..e3ab929e677 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -23,6 +23,7 @@ add_clang_library(clangTidyMiscModule
   NonCopyableObjects.cpp
   PointerAndIntegralOperationCheck.cpp
   SizeofContainerCheck.cpp
+  SizeofExpressionCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index bbc0eb714e8..f4393c27351 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -31,6 +31,7 @@
 #include "NonCopyableObjects.h"
 #include "PointerAndIntegralOperationCheck.h"
 #include "SizeofContainerCheck.h"
+#include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
@@ -92,6 +93,8 @@ public:
     CheckFactories.registerCheck<PointerAndIntegralOperationCheck>(
         "misc-pointer-and-integral-operation");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
+    CheckFactories.registerCheck<SizeofExpressionCheck>(
+        "misc-sizeof-expression");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
diff --git a/clang-tools-extra/clang-tidy/misc/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/SizeofExpressionCheck.cpp
new file mode 100644
index 00000000000..c0a223d17a7
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/SizeofExpressionCheck.cpp
@@ -0,0 +1,265 @@
+//===--- SizeofExpressionCheck.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 "SizeofExpressionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+AST_MATCHER(BinaryOperator, isRelationalOperator) {
+  return Node.isRelationalOp();
+}
+
+AST_MATCHER_P(IntegerLiteral, isBiggerThan, unsigned, N) {
+  return Node.getValue().getZExtValue() > N;
+}
+
+AST_MATCHER_P2(Expr, hasSizeOfDescendant, int, Depth,
+               ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  if (Depth < 0)
+    return false;
+
+  const Expr *E = Node.IgnoreParenImpCasts();
+  if (InnerMatcher.matches(*E, Finder, Builder))
+    return true;
+
+  if (const auto *CE = dyn_cast<CastExpr>(E)) {
+    const auto M = hasSizeOfDescendant(Depth - 1, InnerMatcher);
+    return M.matches(*CE->getSubExpr(), Finder, Builder);
+  } else if (const auto *UE = dyn_cast<UnaryOperator>(E)) {
+    const auto M = hasSizeOfDescendant(Depth - 1, InnerMatcher);
+    return M.matches(*UE->getSubExpr(), Finder, Builder);
+  } else if (const auto *BE = dyn_cast<BinaryOperator>(E)) {
+    const auto LHS = hasSizeOfDescendant(Depth - 1, InnerMatcher);
+    const auto RHS = hasSizeOfDescendant(Depth - 1, InnerMatcher);
+    return LHS.matches(*BE->getLHS(), Finder, Builder) ||
+           RHS.matches(*BE->getRHS(), Finder, Builder);
+  }
+
+  return false;
+}
+
+CharUnits getSizeOfType(const ASTContext &Ctx, const Type *Ty) {
+  if (!Ty || Ty->isIncompleteType() || Ty->isDependentType() ||
+      isa<DependentSizedArrayType>(Ty) || !Ty->isConstantSizeType())
+    return CharUnits::Zero();
+  return Ctx.getTypeSizeInChars(Ty);
+}
+
+} // namespace
+
+SizeofExpressionCheck::SizeofExpressionCheck(StringRef Name,
+                                             ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      WarnOnSizeOfConstant(Options.get("WarnOnSizeOfConstant", 1) != 0),
+      WarnOnSizeOfThis(Options.get("WarnOnSizeOfThis", 1) != 0),
+      WarnOnSizeOfCompareToConstant(
+          Options.get("WarnOnSizeOfCompareToConstant", 1) != 0) {}
+
+void SizeofExpressionCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WarnOnSizeOfConstant", WarnOnSizeOfConstant);
+  Options.store(Opts, "WarnOnSizeOfThis", WarnOnSizeOfThis);
+  Options.store(Opts, "WarnOnSizeOfCompareToConstant",
+                WarnOnSizeOfCompareToConstant);
+}
+
+void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IntegerExpr = ignoringParenImpCasts(integerLiteral());
+  const auto ConstantExpr = expr(ignoringParenImpCasts(
+      anyOf(integerLiteral(), unaryOperator(hasUnaryOperand(IntegerExpr)),
+            binaryOperator(hasLHS(IntegerExpr), hasRHS(IntegerExpr)))));
+  const auto SizeOfExpr =
+      expr(anyOf(sizeOfExpr(has(type())), sizeOfExpr(has(expr()))));
+  const auto SizeOfZero = expr(
+      sizeOfExpr(has(expr(ignoringParenImpCasts(integerLiteral(equals(0)))))));
+
+  // Detect expression like: sizeof(ARRAYLEN);
+  // Note: The expression 'sizeof(sizeof(0))' is a portable trick used to know
+  //       the sizeof size_t.
+  if (WarnOnSizeOfConstant) {
+    Finder->addMatcher(expr(sizeOfExpr(has(ConstantExpr)), unless(SizeOfZero))
+                           .bind("sizeof-constant"),
+                       this);
+  }
+
+  // Detect expression like: sizeof(this);
+  if (WarnOnSizeOfThis) {
+    Finder->addMatcher(
+        expr(sizeOfExpr(has(expr(ignoringParenImpCasts(cxxThisExpr())))))
+            .bind("sizeof-this"),
+        this);
+  }
+
+  // Detect sizeof(kPtr) where kPtr is 'const char* kPtr = "abc"';
+  const auto CharPtrType = pointerType(pointee(isAnyCharacter()));
+  const auto ConstStrLiteralDecl =
+      varDecl(isDefinition(), hasType(qualType(hasCanonicalType(CharPtrType))),
+              hasInitializer(ignoringParenImpCasts(stringLiteral())));
+  Finder->addMatcher(
+      expr(sizeOfExpr(has(expr(hasType(qualType(hasCanonicalType(CharPtrType))),
+                               ignoringParenImpCasts(declRefExpr(
+                                   hasDeclaration(ConstStrLiteralDecl)))))))
+          .bind("sizeof-charp"),
+      this);
+
+  // Detect sizeof(ptr) where ptr points to an aggregate (i.e. sizeof(&S)).
+  const auto ArrayExpr = expr(ignoringParenImpCasts(
+      expr(hasType(qualType(hasCanonicalType(arrayType()))))));
+  const auto ArrayCastExpr = expr(anyOf(
+      unaryOperator(hasUnaryOperand(ArrayExpr), unless(hasOperatorName("*"))),
+      binaryOperator(hasEitherOperand(ArrayExpr)),
+      castExpr(hasSourceExpression(ArrayExpr))));
+  const auto PointerToArrayExpr = expr(ignoringParenImpCasts(expr(
+      hasType(qualType(hasCanonicalType(pointerType(pointee(arrayType()))))))));
+
+  const auto StructAddrOfExpr =
+      unaryOperator(hasOperatorName("&"),
+                    hasUnaryOperand(ignoringParenImpCasts(expr(
+                        hasType(qualType(hasCanonicalType(recordType())))))));
+
+  Finder->addMatcher(
+      expr(sizeOfExpr(has(expr(ignoringParenImpCasts(
+               anyOf(ArrayCastExpr, PointerToArrayExpr, StructAddrOfExpr))))))
+          .bind("sizeof-pointer-to-aggregate"),
+      this);
+
+  // Detect expression like: sizeof(epxr) <= k for a suspicious constant 'k'.
+  if (WarnOnSizeOfCompareToConstant) {
+    Finder->addMatcher(
+        binaryOperator(isRelationalOperator(),
+                       hasEitherOperand(ignoringParenImpCasts(SizeOfExpr)),
+                       hasEitherOperand(ignoringParenImpCasts(
+                           anyOf(integerLiteral(equals(0)),
+                                 integerLiteral(isBiggerThan(0x80000))))))
+            .bind("sizeof-compare-constant"),
+        this);
+  }
+
+  // Detect expression like: sizeof(expr, expr); most likely an error.
+  Finder->addMatcher(expr(sizeOfExpr(has(expr(ignoringParenImpCasts(
+                              binaryOperator(hasOperatorName(",")))))))
+                         .bind("sizeof-comma-expr"),
+                     this);
+
+  // Detect sizeof(...) /sizeof(...));
+  const auto ElemType =
+      arrayType(hasElementType(recordType().bind("elem-type")));
+  const auto ElemPtrType = pointerType(pointee(type().bind("elem-ptr-type")));
+  const auto NumType = qualType(hasCanonicalType(
+      type(anyOf(ElemType, ElemPtrType, type())).bind("num-type")));
+  const auto DenomType = qualType(hasCanonicalType(type().bind("denom-type")));
+
+  Finder->addMatcher(
+      binaryOperator(hasOperatorName("/"),
+                     hasLHS(expr(ignoringParenImpCasts(
+                         anyOf(sizeOfExpr(has(NumType)),
+                               sizeOfExpr(has(expr(hasType(NumType)))))))),
+                     hasRHS(expr(ignoringParenImpCasts(
+                         anyOf(sizeOfExpr(has(DenomType)),
+                               sizeOfExpr(has(expr(hasType(DenomType)))))))))
+          .bind("sizeof-divide-expr"),
+      this);
+
+  // Detect expression like: sizeof(...) * sizeof(...)); most likely an error.
+  Finder->addMatcher(binaryOperator(hasOperatorName("*"),
+                                    hasLHS(ignoringParenImpCasts(SizeOfExpr)),
+                                    hasRHS(ignoringParenImpCasts(SizeOfExpr)))
+                         .bind("sizeof-multiply-sizeof"),
+                     this);
+
+  Finder->addMatcher(
+      binaryOperator(hasOperatorName("*"),
+                     hasEitherOperand(ignoringParenImpCasts(SizeOfExpr)),
+                     hasEitherOperand(ignoringParenImpCasts(binaryOperator(
+                         hasOperatorName("*"),
+                         hasEitherOperand(ignoringParenImpCasts(SizeOfExpr))))))
+          .bind("sizeof-multiply-sizeof"),
+      this);
+
+  // Detect strange double-sizeof expression like: sizeof(sizeof(...));
+  // Note: The expression 'sizeof(sizeof(0))' is accepted.
+  Finder->addMatcher(expr(sizeOfExpr(has(expr(hasSizeOfDescendant(
+                              8, expr(SizeOfExpr, unless(SizeOfZero)))))))
+                         .bind("sizeof-sizeof-expr"),
+                     this);
+}
+
+void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
+  const ASTContext &Ctx = *Result.Context;
+
+  if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-constant")) {
+    diag(E->getLocStart(),
+         "suspicious usage of 'sizeof(K)'; did you mean 'K'?");
+  } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-this")) {
+    diag(E->getLocStart(),
+         "suspicious usage of 'sizeof(this)'; did you mean 'sizeof(*this)'");
+  } else if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-charp")) {
+    diag(E->getLocStart(),
+         "suspicious usage of 'sizeof(char*)'; do you mean 'strlen'?");
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-pointer-to-aggregate")) {
+    diag(E->getLocStart(),
+         "suspicious usage of 'sizeof(A*)'; pointer to aggregate");
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-compare-constant")) {
+    diag(E->getLocStart(),
+         "suspicious comparison of 'sizeof(expr)' to a constant");
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-comma-expr")) {
+    diag(E->getLocStart(), "suspicious usage of 'sizeof(..., ...)'");
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-divide-expr")) {
+    const auto *NumTy = Result.Nodes.getNodeAs<Type>("num-type");
+    const auto *DenomTy = Result.Nodes.getNodeAs<Type>("denom-type");
+    const auto *ElementTy = Result.Nodes.getNodeAs<Type>("elem-type");
+    const auto *PointedTy = Result.Nodes.getNodeAs<Type>("elem-ptr-type");
+
+    CharUnits NumeratorSize = getSizeOfType(Ctx, NumTy);
+    CharUnits DenominatorSize = getSizeOfType(Ctx, DenomTy);
+    CharUnits ElementSize = getSizeOfType(Ctx, ElementTy);
+
+    if (DenominatorSize > CharUnits::Zero() &&
+        !NumeratorSize.isMultipleOf(DenominatorSize)) {
+      diag(E->getLocStart(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
+                             " numerator is not a multiple of denominator");
+    } else if (ElementSize > CharUnits::Zero() &&
+               DenominatorSize > CharUnits::Zero() &&
+               ElementSize != DenominatorSize) {
+      diag(E->getLocStart(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
+                             " numerator is not a multiple of denominator");
+    } else if (NumTy && DenomTy && NumTy == DenomTy) {
+      diag(E->getLocStart(),
+           "suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'");
+    } else if (PointedTy && DenomTy && PointedTy == DenomTy) {
+      diag(E->getLocStart(),
+           "suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'");
+    } else if (NumTy && DenomTy && NumTy->isPointerType() &&
+               DenomTy->isPointerType()) {
+      diag(E->getLocStart(),
+           "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'");
+    }
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-sizeof-expr")) {
+    diag(E->getLocStart(), "suspicious usage of 'sizeof(sizeof(...))'");
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-multiply-sizeof")) {
+    diag(E->getLocStart(), "suspicious 'sizeof' by 'sizeof' multiplication");
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/misc/SizeofExpressionCheck.h b/clang-tools-extra/clang-tidy/misc/SizeofExpressionCheck.h
new file mode 100644
index 00000000000..82388d142f5
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/SizeofExpressionCheck.h
@@ -0,0 +1,40 @@
+//===--- SizeofExpressionCheck.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_SIZEOF_EXPRESSION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SIZEOF_EXPRESSION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Find suspicious usages of sizeof expression.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-sizeof-expression.html
+class SizeofExpressionCheck : public ClangTidyCheck {
+public:
+  SizeofExpressionCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool WarnOnSizeOfConstant;
+  const bool WarnOnSizeOfThis;
+  const bool WarnOnSizeOfCompareToConstant;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SIZEOF_EXPRESSION_H