[clang-tidy] Extend bugprone-sizeof-expression check to detect sizeof…

… misuse in pointer arithmetic Some programmers tend to forget that subtracting two pointers results in the difference between them in number of elements of the pointee type instead of bytes. This leads to codes such as `size_t size = (p - q) / sizeof(int)` where `p` and `q` are of type `int*`. Or similarily, `if (p - q < buffer_size * sizeof(int)) { ... }`. This patch extends `bugprone-sizeof-expression` to detect such cases. Differential Revision: https://reviews.llvm.org/D61422 llvm-svn: 360032
llvm · May 17, 2019 · adf1b61 · adf1b61
1 parent d59f9b1
commit adf1b61
Show file tree

Hide file tree

Showing 2 changed files with 84 additions and 2 deletions.
diff --git a/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -84,8 +84,11 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto IntegerCallExpr = expr(ignoringParenImpCasts(
       callExpr(anyOf(hasType(isInteger()), hasType(enumType())),
                unless(isInTemplateInstantiation()))));
-  const auto SizeOfExpr =
-      expr(anyOf(sizeOfExpr(has(type())), sizeOfExpr(has(expr()))));
+  const auto SizeOfExpr = expr(anyOf(
+      sizeOfExpr(
+          has(hasUnqualifiedDesugaredType(type().bind("sizeof-arg-type")))),
+      sizeOfExpr(has(expr(hasType(
+          hasUnqualifiedDesugaredType(type().bind("sizeof-arg-type"))))))));
   const auto SizeOfZero = expr(
       sizeOfExpr(has(ignoringParenImpCasts(expr(integerLiteral(equals(0)))))));
 
@@ -209,6 +212,36 @@ void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
                hasSizeOfDescendant(8, expr(SizeOfExpr, unless(SizeOfZero))))))))
           .bind("sizeof-sizeof-expr"),
       this);
+
+  // Detect sizeof in pointer aritmetic like: N * sizeof(S) == P1 - P2 or
+  // (P1 - P2) / sizeof(S) where P1 and P2 are pointers to type S.
+  const auto PtrDiffExpr = binaryOperator(
+      hasOperatorName("-"),
+      hasLHS(expr(hasType(hasUnqualifiedDesugaredType(pointerType(pointee(
+          hasUnqualifiedDesugaredType(type().bind("left-ptr-type")))))))),
+      hasRHS(expr(hasType(hasUnqualifiedDesugaredType(pointerType(pointee(
+          hasUnqualifiedDesugaredType(type().bind("right-ptr-type")))))))));
+
+  Finder->addMatcher(
+      binaryOperator(
+          anyOf(hasOperatorName("=="), hasOperatorName("!="),
+                hasOperatorName("<"), hasOperatorName("<="),
+                hasOperatorName(">"), hasOperatorName(">="),
+                hasOperatorName("+"), hasOperatorName("-")),
+          hasEitherOperand(expr(anyOf(
+              ignoringParenImpCasts(SizeOfExpr),
+              ignoringParenImpCasts(binaryOperator(
+                  hasOperatorName("*"),
+                  hasEitherOperand(ignoringParenImpCasts(SizeOfExpr))))))),
+          hasEitherOperand(ignoringParenImpCasts(PtrDiffExpr)))
+          .bind("sizeof-in-ptr-arithmetic-mul"),
+      this);
+
+  Finder->addMatcher(binaryOperator(hasOperatorName("/"),
+                                    hasLHS(ignoringParenImpCasts(PtrDiffExpr)),
+                                    hasRHS(ignoringParenImpCasts(SizeOfExpr)))
+                         .bind("sizeof-in-ptr-arithmetic-div"),
+                     this);
 }
 
 void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
@@ -275,6 +308,26 @@ void SizeofExpressionCheck::check(const MatchFinder::MatchResult &Result) {
   } else if (const auto *E =
                  Result.Nodes.getNodeAs<Expr>("sizeof-multiply-sizeof")) {
     diag(E->getBeginLoc(), "suspicious 'sizeof' by 'sizeof' multiplication");
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-in-ptr-arithmetic-mul")) {
+    const auto *LPtrTy = Result.Nodes.getNodeAs<Type>("left-ptr-type");
+    const auto *RPtrTy = Result.Nodes.getNodeAs<Type>("right-ptr-type");
+    const auto *SizeofArgTy = Result.Nodes.getNodeAs<Type>("sizeof-arg-type");
+
+    if ((LPtrTy == RPtrTy) && (LPtrTy == SizeofArgTy)) {
+      diag(E->getBeginLoc(), "suspicious usage of 'sizeof(...)' in "
+                              "pointer arithmetic");
+    }
+  } else if (const auto *E =
+                 Result.Nodes.getNodeAs<Expr>("sizeof-in-ptr-arithmetic-div")) {
+    const auto *LPtrTy = Result.Nodes.getNodeAs<Type>("left-ptr-type");
+    const auto *RPtrTy = Result.Nodes.getNodeAs<Type>("right-ptr-type");
+    const auto *SizeofArgTy = Result.Nodes.getNodeAs<Type>("sizeof-arg-type");
+
+    if ((LPtrTy == RPtrTy) && (LPtrTy == SizeofArgTy)) {
+      diag(E->getBeginLoc(), "suspicious usage of 'sizeof(...)' in "
+                              "pointer arithmetic");
+    }
   }
 }
 

diff --git a/clang-tools-extra/test/clang-tidy/bugprone-sizeof-expression.cpp b/clang-tools-extra/test/clang-tidy/bugprone-sizeof-expression.cpp
@@ -231,6 +231,35 @@ int Test5() {
   return sum;
 }
 
+int Test6() {
+  int sum = 0;
+
+  struct S A = AsStruct(), B = AsStruct();
+  struct S *P = &A, *Q = &B;
+  sum += sizeof(struct S) == P - Q;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += 5 * sizeof(S) != P - Q;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += sizeof(S) < P - Q;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += 5 * sizeof(S) <= P - Q;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += 5 * sizeof(*P) >= P - Q;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += Q - P > 3 * sizeof(*P);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += sizeof(S) + (P - Q);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += 5 * sizeof(S) - (P - Q);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += (P - Q) / sizeof(S);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+  sum += (P - Q) / sizeof(*Q);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(...)' in pointer arithmetic
+
+  return sum;
+}
+
 int ValidExpressions() {
   int A[] = {1, 2, 3, 4};
   static const char str[] = "hello";