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

[etienneb]

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;

[alexfh]

The check is really awesome! Thank you for coming up with all the patterns that frequently happen to result from coding errors!

Please update release notes.

A few inline comments as well.

clang-tidy/misc/SizeofExpressionCheck.cpp

22

Seems like you could use the isAnyCharacter matcher.

27

nit: extra 'n' in "relational"

27

I'd put this to ASTMatchers.h together with a test and docs.

40

The !(E = ...) construct is a bit hard to read (needs time to figure out the intention). I'd prefer a simpler alternative.

196

It looks like you've created a tool for what can be done in a simpler way. It should be possible to express this matcher without going through the parent map: sizeOfExpr(hasDescendant(sizeOfExpr(unless(SizeOfZero)))) or something like that.

236

I'm not sure I would understand what "incompatible" means here without reading the code of the check. Please expand the message. Same in the messages below.

258

s/multiplation/multiplication/

[etienneb]

fix alexfh@ comments

[etienneb]

release notes.

[etienneb]

more unittests

[etienneb]

fix typos and doc.

[alexfh]

clang-tidy/misc/SizeofExpressionCheck.cpp

198

What about this comment?

204

nit: missing trailing question mark

210

nit: 'strlen' should be in single quotes for consistency with other messages.

258

nit: I'd say of 'sizeof' by 'sizeof' instead of of two 'sizeof'.

docs/clang-tidy/checks/misc-sizeof-expression.rst

59

nit: the titles need to be consistent - either all have or not have warning:.

I'd also make the first letters capital.

103

s/make/makes/

[etienneb]

alexfh comments.

clang-tidy/misc/SizeofExpressionCheck.cpp

27

There are a few matchers used in different checkers that worth lifting.
It seems there is a place to lift some of them in clang-tidy/utils, and some of them should be lifted to astmatcher directly.
I wonder what should be the rules to lift them.

196

This is 'almost' working!
We cannot pass through all expression.
As an example, callExpr or arrayExpr

sizeof( A[index(sizeof(int))] )

In this example, the checker won't warn.

So, the hasDescendant (or has ancestor) in this case is only allow to follow a subset of Expressions.

The name may be wrong, or unclear. If you have better proposition...

198

unsubmited. (not sure to get when arc is pushing them)

[alexfh]

clang-tidy/misc/SizeofExpressionCheck.cpp

29

There are no firm rules. It mostly depends on how generic/useful in other tools the matcher could be. This one seems pretty generic and it could be used in a couple of other clang-tidy checks at least (e.g. readability/ContainerSizeEmptyCheck.cpp), so it seems to be a good fit for ASTMatchers.h. This can also be done as a separate step, if you prefer.

198

I see the reasoning, but in this case I'd still prefer if this custom matcher looked at the children instead of the parents, since it's a more effective approach in general (at least because it doesn't use the parent map).

We should probably add a variant of hasDescendant (and maybe hasAncestor) with a way to limit which nodes it can look through.

258

"'sizeof' by 'sizeof' expression" is ambiguous, "'sizeof' by 'sizeof' multiplication" would be better.

[etienneb]

alexfh comments.

[alexfh]

clang-tidy/misc/SizeofExpressionCheck.cpp

199

I'm probably wrong about "a more effective approach in general", but for sizeof case it might be more effective, if we assume that sizeof doesn't have large subtrees under it.

A separate concern is that the matchers looking up the tree are somewhat more difficult to understand (at least to me).

Ultimately, I'm not sure what's the best approach here. Maybe find a huge file and measure runtime of both implementations? ;)

[alexfh]

Looks good with one more nit. I'm not sure what to do with hasSizeOfAncestor, we can leave it as is for now.

clang-tidy/misc/SizeofExpressionCheck.cpp

48

The type here doesn't help, I'd use auto.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

100 ↗

(On Diff #53442)

Please put this check after misc-misplaced-widening-cast.

[malcolm.parsons]

docs/clang-tidy/checks/misc-sizeof-expression.rst

17

s/it's/its/

[etienneb]

alexfh@ comment: top-down approach.

clang-tidy/misc/SizeofExpressionCheck.cpp

29

I'm not against at all to lift this.
Now, or in a separate step.

I just leave it here so the patch is still working as-is (if people want to play with it and found invalid cases!).

198

I was thinking it's more effective to look to ancestor than descendants?
There are less ancestors than descendants?

199

I'm uploading the top-down approach.
As I suspected, this is changing nothing on large cases I tried.

I put the limit of 8 level depth, which won't cause any combinatory explosion.
Worse case: 2^8 nodes.

[etienneb]

rebased

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
+  SizeofExpressionCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
+#include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
+    CheckFactories.registerCheck<SizeofExpressionCheck>(
+        "misc-sizeof-expression");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "misc-string-integer-assignment");
     CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>(
         "misc-string-literal-with-embedded-nul");
     CheckFactories.registerCheck<SuspiciousMissingCommaCheck>(
         "misc-suspicious-missing-comma");

clang-tidy/misc/SizeofExpressionCheck.h

+//===--- 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

clang-tidy/misc/SizeofExpressionCheck.cpp

+//===--- 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(QualType, isAnyChar) {
+  return Node->isCharType() || Node->isWideCharType() || Node->isChar16Type() ||
+         Node->isChar32Type();
+}
+
+AST_MATCHER(BinaryOperator, isRelationnalOperator) {
+  return Node.isRelationalOp();
+}
+
+AST_MATCHER_P(IntegerLiteral, isBiggerThan, unsigned, N) {
+  return Node.getValue().getZExtValue() > N;
+}
+
+AST_MATCHER_P(Expr, hasSizeOfAncestor, ast_matchers::internal::Matcher<Expr>,
+              InnerMatcher) {
+  const Expr *E = &Node;
+
+  ASTContext::DynTypedNodeList Parents = Finder->getASTContext().getParents(*E);
+  if (Parents.size() != 1 || !(E = Parents[0].get<Expr>()))
+    return false;
+
+  if (InnerMatcher.matches(*E, Finder, Builder))
+    return true;
+
+  if (isa<CastExpr>(E) || isa<UnaryOperator>(E) || isa<BinaryOperator>(E) ||
+      isa<ParenExpr>(E)) {
+    ast_matchers::internal::Matcher<Expr> M = hasSizeOfAncestor(InnerMatcher);
+    return M.matches(*E, 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(isAnyChar()));
+  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 point 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 sizeof(epxr) <= k) for a suspicious constant 'k'.
+  if (WarnOnSizeOfCompareToConstant) {
+    Finder->addMatcher(
+        binaryOperator(isRelationnalOperator(),
+                       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)),
+                     hasRHS(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, unless(SizeOfZero),
+           hasSizeOfAncestor(SizeOfExpr.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(...)';"
+                             " sizes are incompatible");
+    } else if (ElementSize > CharUnits::Zero() &&
+               DenominatorSize > CharUnits::Zero() &&
+               ElementSize != DenominatorSize) {
+      diag(E->getLocStart(), "suspicious usage of 'sizeof(...)/sizeof(...)';"
+                             " element sizes are incompatible");
+    } 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 multiplation of two 'sizeof'");
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-widening-cast
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-sizeof-container
+   misc-sizeof-expression
    misc-static-assert
    misc-string-integer-assignment
    misc-string-literal-with-embedded-nul
    misc-suspicious-missing-comma
    misc-suspicious-semicolon
    misc-swapped-arguments
    misc-throw-by-value-catch-by-reference
    misc-undelegated-constructor

docs/clang-tidy/checks/misc-sizeof-expression.rst

+.. title:: clang-tidy - misc-sizeof-expression
+
+misc-sizeof-expression
+======================
+
+The check finds usages of ``sizeof`` expressions which are most likely errors.
+
+The ``sizeof`` operator yields the size (in bytes) of its operand, which may be
+an expression or the parenthesized name of a type. Misuse of this operator may
+be leading to errors and possible software vulnerabilities.
+
+
+suspicious usage of 'sizeof(K)'
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+A common mistake is to query the ``sizeof`` of an integer literal. This is
+equivalent to query the size of it's type (probably ``int``). The intend of the
+programmer was probably to simply get the integer and not it's size.
+
+.. code:: c++
+
+  #define BUFLEN 42
+  char buf[BUFLEN];
+  memset(buf, 0, sizeof(BUFLEN));  // sizeof(42) ==> sizeof(int)
+
+
+suspicious usage of 'sizeof(this)'
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+The ``this`` keyword is evaluated to a pointer to an object of a given type.
+The expression ``sizeof(this)`` is returning the size of a pointer. The
+programmer most likely wanted the size of the object and not the size of the
+pointer.
+
+.. code:: c++
+
+  class Point {
+    [...]
+    size_t size() { return sizeof(this); }  // should probably be sizeof(*this)
+    [...]  
+  };
+
+
+suspicious usage of 'sizeof(char*)'
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+There is a subtle difference between declaring a string literal with
+``char* A = ""`` and ``char A[] = ""``. The first case has the type ``char*``
+instead of the aggregate type ``char[]``. Using sizeof on an object declared
+with ``char*`` type is returning the size of a pointer instead of the number of
+characters (bytes) in the string literal.
+
+.. code:: c++
+
+  const char* kMessage = "Hello World!";      // const char kMessage[] = "...";
+  void getMessage(char* buf) {
+    memcpy(buf, kMessage, sizeof(kMessage));  // sizeof(char*)
+  }
+
+
+warning: suspicious usage of 'sizeof(A*)'
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+A common mistake is to compute the sizeof a pointer instead it's pointee. These
+cases may occur because of explicit cast or implicit conversion.
+
+.. code:: c++
+
+  int A[10];
+  memset(A, 0, sizeof(A + 0));
+
+  struct Point point;
+  memset(point, 0, sizeof(&point));
+
+
+suspicious usage of 'sizeof(...)/sizeof(...)'
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Dividing ``sizeof`` expressions is typically used to retrieve the number of
+sub-elements of an aggregate. This check warns on incompatible or suspicious
+cases.
+
+In the following example, the entity has 10-bytes and is incompatible with the
+type ``int`` which has 4 bytes.
+
+.. code:: c++
+
+  char buf[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };  // sizeof(buf) => 10
+  void getMessage(char* dst) {
+    memcpy(dst, buf, sizeof(buf) / sizeof(int));  // sizeof(int) => 4  [incompatible sizes]
+  }
+
+
+In the following example, the expressions ``sizeof(Values)`` is returning the
+size of ``char*``. One who can be easily fooled by it's declaration, but in
+parameter declaraction the size '10' is ignored and the function is receiving a
+``char*``.
+
+.. code:: c++
+
+  char OrderedValues[10] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
+  return CompareArray(char Values[10]) {
+    return memcmp(OrderedValues, Values, sizeof(Values)) == 0;  // sizeof(Values) ==> sizeof(char*) [implicit cast to char*]
+  }
+
+
+suspicious multiplation of two 'sizeof'
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Multiplying sizeof expressions typically make no sense and is probably an logic
+error. In the following example, the programmer used ``*`` instead of ``/``.
+
+.. code:: c++
+
+  const char kMessage[] = "Hello World!";
+  void getMessage(char* buf) {
+    memcpy(buf, kMessage, sizeof(kMessage) * sizeof(char));  //  sizeof(kMessage) / sizeof(char)
+  }
+
+
+This check may trigger on code using the arraysize macro. The following code is
+working correctly but should be simplified by only using the sizeof operator.
+
+.. code:: c++
+
+  extern Object objects[100];
+  void InitializeObjects() {
+    memset(objects, 0, arraysize(objects) * sizeof(Object));  // sizeof(objects)
+  }
+
+
+suspicious usage of 'sizeof(sizeof(...))'
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Getting the ``sizeof`` of a ``sizeof`` makes no sense and is typically an error
+hidden through macros.
+
+.. code:: c++
+
+  #define INT_SZ sizeof(int)
+  int buf[] = { 42 };
+  void getInt(int* dst) {
+    memcpy(dst, buf, sizeof(INT_SZ));  // sizeof(sizeof(int)) is suspicious.
+  }

test/clang-tidy/misc-sizeof-expression.cpp

+// RUN: %check_clang_tidy %s misc-sizeof-expression %t
+
+class C {
+  int size() { return sizeof(this); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: suspicious usage of 'sizeof(this)'
+};
+
+#define LEN 8
+
+int X;
+extern int A[10];
+extern short B[10];
+
+#pragma pack(1)
+struct  S { char a, b, c; };
+
+int Test1(const char* ptr) {
+  int sum = 0;
+  sum += sizeof(LEN);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
+  sum += sizeof(LEN + 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(K)'
+  sum += sizeof(sum, LEN);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(..., ...)'
+  sum += sizeof(sizeof(X));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
+  sum += sizeof(LEN + sizeof(X));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
+  sum += sizeof(LEN + LEN + sizeof(X));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
+  sum += sizeof(LEN + (LEN + sizeof(X)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
+  sum += sizeof(LEN + -sizeof(X));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
+  sum += sizeof(LEN + - + -sizeof(X));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(sizeof(...))'
+  sum += sizeof(char) / sizeof(char);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+  sum += sizeof(A) / sizeof(S);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; sizes are incompatible
+  sum += sizeof(char) / sizeof(int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; sizes are incompatible
+  sum += sizeof(char) / sizeof(A);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; sizes are incompatible
+  sum += sizeof(B[0]) / sizeof(A);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)/sizeof(...)'; sizes are incompatible
+  sum += sizeof(ptr) / sizeof(char);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'
+  sum += sizeof(ptr) / sizeof(ptr[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'
+  sum += sizeof(ptr) / sizeof(char*);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'
+  sum += sizeof(ptr) / sizeof(void*);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'
+  sum += sizeof(ptr) / sizeof(const void volatile*);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'
+  sum += sizeof(ptr) / sizeof(char);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'
+  sum += sizeof(ptr) / sizeof(ptr[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T*)/sizeof(T)'
+  sum += sizeof(int) * sizeof(char);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious multiplation of two 'sizeof'
+  sum += sizeof(ptr) * sizeof(ptr[0]);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious multiplation of two 'sizeof'
+  sum += sizeof(int) * (2 * sizeof(char));
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious multiplation of two 'sizeof'
+  return sum;
+}
+
+typedef char MyChar;
+typedef const MyChar MyConstChar;
+
+int CE0 = sizeof sizeof(char);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: suspicious usage of 'sizeof(sizeof(...))'
+int CE1 = sizeof +sizeof(char);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: suspicious usage of 'sizeof(sizeof(...))'
+int CE2 = sizeof sizeof(const char*);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: suspicious usage of 'sizeof(sizeof(...))'
+int CE3 = sizeof sizeof(const volatile char* const*);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: suspicious usage of 'sizeof(sizeof(...))'
+int CE4 = sizeof sizeof(MyConstChar);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: suspicious usage of 'sizeof(sizeof(...))'
+
+int Test2(MyConstChar* A) {
+  int sum = 0;
+  sum += sizeof(MyConstChar) / sizeof(char);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+  sum += sizeof(MyConstChar) / sizeof(MyChar);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+  sum += sizeof(A[0]) / sizeof(char);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+  return sum;
+}
+
+template <int T>
+int Foo() { int A[T]; return sizeof(T); }
+// CHECK-MESSAGES: :[[@LINE-1]]:30: warning: suspicious usage of 'sizeof(K)'
+template <typename T>
+int Bar() { T A[5]; return sizeof(A[0]) / sizeof(T); }
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: suspicious usage of sizeof pointer 'sizeof(T)/sizeof(T)'
+int Test3() { return Foo<42>() + Bar<char>(); }
+
+static const char* kABC = "abc";
+int Test4(const char A[10]) {
+  int sum = 0;
+  sum += sizeof(kABC);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(char*)'
+  sum += sizeof(A);
+  // ?????
+  return sum;
+}
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+    Array10 arr;
+    Array10* ptr;
+  };
+  typedef const MyStruct TMyStruct;
+
+  static TMyStruct kGlocalMyStruct = {};
+  static TMyStruct volatile * kGlocalMyStructPtr = &kGlocalMyStruct;
+
+  MyStruct S;
+  Array10 A10;
+
+  int sum = 0;
+  sum += sizeof(&S.arr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(&kGlocalMyStruct.arr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(&kGlocalMyStructPtr->arr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(S.arr + 0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(+ S.arr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof((int*)S.arr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+
+  sum += sizeof(S.ptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(kGlocalMyStruct.ptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(kGlocalMyStructPtr->ptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+
+  sum += sizeof(&kGlocalMyStruct);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(&S);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+  sum += sizeof(&A10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate 
+
+  return sum;
+}
+
+int ValidExpressions() {
+  int A[] = {1, 2, 3, 4};
+  static const char str[] = "hello";
+  static const char* ptr[] { "aaa", "bbb", "ccc" };
+  int sum = 0;
+  if (sizeof(A) < 10)
+    sum += sizeof(A);
+  sum += sizeof(int);
+  sum += sizeof(A[sizeof(A) / sizeof(int)]);
+  sum += sizeof(&A[sizeof(A) / sizeof(int)]);
+  sum += sizeof(sizeof(0));  // Special case: sizeof size_t.
+  sum += sizeof(void*);
+  sum += sizeof(void const *);
+  sum += sizeof(void const *) / 4;
+  sum += sizeof(str);
+  sum += sizeof(str) / sizeof(char);
+  sum += sizeof(str) / sizeof(str[0]);
+  sum += sizeof(ptr) / sizeof(ptr[0]);
+  sum += sizeof(ptr) / sizeof(*(ptr));
+  return sum;
+}