[clang-tidy] Detect bugs in bugprone-misplaced-operator-in-strlen-in-alloc even in the case the allocation function is called using a constant function pointer

[baloghadamsoftware]

Summary

Detect bugs even if a function of the malloc() family is called using a constant pointer.

[aaron.ballman]

LGTM

FYI: it's been difficult to perform this review because all of these reviews are touching the same chunk of code for something that's not been committed yet. It would be easier to review if all of these reviews were combined into the review adding the check.

[baloghadamsoftware]

In D39370#910791, @aaron.ballman wrote:

LGTM

FYI: it's been difficult to perform this review because all of these reviews are touching the same chunk of code for something that's not been committed yet. It would be easier to review if all of these reviews were combined into the review adding the check.

I am also working for the Static Analyzer where I received the comment that development should be incremental to avoid huge patches. So I tried to use the same approach here as well.

[aaron.ballman]

In D39370#910803, @baloghadamsoftware wrote:

In D39370#910791, @aaron.ballman wrote:

LGTM

FYI: it's been difficult to perform this review because all of these reviews are touching the same chunk of code for something that's not been committed yet. It would be easier to review if all of these reviews were combined into the review adding the check.

I am also working for the Static Analyzer where I received the comment that development should be incremental to avoid huge patches. So I tried to use the same approach here as well.

Incremental is definitely the way to go, but that's usually to prevent massive code dumps of large-scale functionality. For a single, relatively small check like this, I think it's fine to add all of this into one review because it's all so tightly related.

[baloghadamsoftware]

Updated to match D39121.

[baloghadamsoftware]

Closed by commit rL318913.

clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp

void MisplacedOperatorInStrlenInAllocCheck::registerMatchers(
 
   const auto Alloc0Func =
       functionDecl(anyOf(hasName("::malloc"), hasName("std::malloc"),
                          hasName("::alloca"), hasName("std::alloca")));
   const auto Alloc1Func =
       functionDecl(anyOf(hasName("::calloc"), hasName("std::calloc"),
                          hasName("::realloc"), hasName("std::realloc")));
 
-  Finder->addMatcher(
-      callExpr(callee(Alloc0Func), hasArgument(0, BadArg)).bind("Alloc"), this);
-  Finder->addMatcher(
-      callExpr(callee(Alloc1Func), hasArgument(1, BadArg)).bind("Alloc"), this);
+  const auto Alloc0FuncPtr =
+      varDecl(hasType(isConstQualified()),
+              hasInitializer(ignoringParenImpCasts(
+                  declRefExpr(hasDeclaration(Alloc0Func)))));
+  const auto Alloc1FuncPtr =
+      varDecl(hasType(isConstQualified()),
+              hasInitializer(ignoringParenImpCasts(
+                  declRefExpr(hasDeclaration(Alloc1Func)))));
+
+  Finder->addMatcher(callExpr(callee(decl(anyOf(Alloc0Func, Alloc0FuncPtr))),
+                              hasArgument(0, BadArg))
+                         .bind("Alloc"),
+                     this);
+  Finder->addMatcher(callExpr(callee(decl(anyOf(Alloc1Func, Alloc1FuncPtr))),
+                              hasArgument(1, BadArg))
+                         .bind("Alloc"),
+                     this);
 }
 
 void MisplacedOperatorInStrlenInAllocCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *Alloc = Result.Nodes.getNodeAs<CallExpr>("Alloc");
   const auto *StrLen = Result.Nodes.getNodeAs<CallExpr>("StrLen");
   const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("BinOp");
 

docs/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.rst

 .. title:: clang-tidy - bugprone-misplaced-operator-in-strlen-in-alloc
 
 bugprone-misplaced-operator-in-strlen-in-alloc
 ==============================================
 
 Finds cases where ``1`` is added to the string in the argument to ``strlen()``,
 ``strnlen()``, ``strnlen_s()``, ``wcslen()``, ``wcsnlen()``, and ``wcsnlen_s()``
 instead of the result and the value is used as an argument to a memory
 allocation function (``malloc()``, ``calloc()``, ``realloc()``, ``alloca()``).
-Cases where ``1`` is added both to the parameter and the result of the
-``strlen()``-like function are ignored, as are cases where the whole addition is
-surrounded by extra parentheses.
+The check detects error cases even if one of these functions is called by a
+constant function pointer.  Cases where ``1`` is added both to the parameter
+and the result of the ``strlen()``-like function are ignored, as are cases where
+the whole addition is surrounded by extra parentheses.
 
 Example code:
 
 .. code-block:: c
 
     void bad_malloc(char *str) {
       char *c = (char*) malloc(strlen(str + 1));
     }

test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.c

void intentional2(char *name) {
   // Only give warning for + 1, not + 2
 }
 
 void intentional3(char *name) {
   char *new_name = (char *)malloc(strlen((name + 1)));
   // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen
   // If expression is in extra parentheses, consider it as intentional
 }
+
+void (*(*const alloc_ptr)(size_t)) = malloc;
+
+void bad_indirect_alloc(char *name) {
+  char *new_name = (char *)alloc_ptr(strlen(name + 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  char \*new_name = \(char \*\)alloc_ptr\(}}strlen(name) + 1{{\);$}}
+}