[clang-tidy] Extend bugprone-sizeof-expression check to detect sizeof misuse in pointer arithmetic

[baloghadamsoftware]

Summary

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.

[baloghadamsoftware]

Type int in tests replaced by struct S because it has more "sugar". Check also fixed to handle this case.

[aaron.ballman]

Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?

clang-tidy/bugprone/SizeofExpressionCheck.cpp

217

Missing full stop at the end of the comment.

[baloghadamsoftware]

Dot added to the end of the comment.

[baloghadamsoftware]

In D61422#1488081, @aaron.ballman wrote:

Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?

Neither false, nor true positives found so far. I ran it on several open-source projects.

[aaron.ballman]

In D61422#1489159, @baloghadamsoftware wrote:

In D61422#1488081, @aaron.ballman wrote:

Out of curiosity, have you run this over any large code bases to see what the false positive and true positive rate is?

Neither false, nor true positives found so far. I ran it on several open-source projects.

Hmm, I am a little bit skeptical about the utility of this compared to the expense of making the check marginally slower. However, I don't have any specific objections and so I'm not opposed to the patch. LG!

[baloghadamsoftware]

In D61422#1489451, @aaron.ballman wrote:

Hmm, I am a little bit skeptical about the utility of this compared to the expense of making the check marginally slower. However, I don't have any specific objections and so I'm not opposed to the patch. LG!

I think that the open-source projects I used for testing are mature enough for such amateur errors. However many beginner programmers make such errors.

clang-tidy/bugprone/SizeofExpressionCheck.cpp

 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 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)))))));
 
   // 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(
void SizeofExpressionCheck::registerMatchers(MatchFinder *Finder) {
 
   // Detect strange double-sizeof expression like: sizeof(sizeof(...));
   // Note: The expression 'sizeof(sizeof(0))' is accepted.
   Finder->addMatcher(
       expr(sizeOfExpr(has(ignoringParenImpCasts(expr(
                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) {
   const ASTContext &Ctx = *Result.Context;
 
   if (const auto *E = Result.Nodes.getNodeAs<Expr>("sizeof-constant")) {
     diag(E->getBeginLoc(),
          "suspicious usage of 'sizeof(K)'; did you mean 'K'?");
if (DenominatorSize > CharUnits::Zero() &&
            "suspicious usage of sizeof pointer 'sizeof(P*)/sizeof(Q*)'");
     }
   } else if (const auto *E =
                  Result.Nodes.getNodeAs<Expr>("sizeof-sizeof-expr")) {
     diag(E->getBeginLoc(), "suspicious usage of 'sizeof(sizeof(...))'");
   } 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");
+    }
   }
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

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

int Test5() {
   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 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";
   static const char* ptr[] { "aaa", "bbb", "ccc" };
   int sum = 0;
   if (sizeof(A) < 10)
     sum += sizeof(A);
   sum += sizeof(int);