[clang-tidy] Add support for operator new[] in check bugprone-misplaced-operator-in-strlen-in-alloc

[baloghadamsoftware]

Summary

The check now recognizes error cases like new char[strlen(s + 1)] and suggests a fix in the format new char[strlen(s) + 1].

[JonasToth]

Should the release notes be modified as well?

[JonasToth]

clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp

62

Break after if condition. You could use the ternary operator, too.

Maybe a safeguard assertion if Alloc is nullptr after the second assignemt. This would uncover a potential bug.

[baloghadamsoftware]

Reformatted according to the comments.

[baloghadamsoftware]

In D39367#909117, @JonasToth wrote:

Should the release notes be modified as well?

I think so. I consider the inclusion of operator new[] important.

[aaron.ballman]

clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp

64–65

The backticks should be single quotes instead, I think.

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

39

Please add tests showing that the correct behavior does not diagnose. Also, please add a test showing that this works with overloaded operator new[]().

[baloghadamsoftware]

Backsticks changed to single quotes, new tests added.

[aaron.ballman]

clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp

64–65

Missed the backticks around CXXNewExpr

[baloghadamsoftware]

Oops! Sorry!

[aaron.ballman]

LGTM

[baloghadamsoftware]

Updated to match D39121.

[baloghadamsoftware]

Closed by commit rL318912.

clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp

const auto BadArg = anyOf(
       BadUse);
 
   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);
+  Finder->addMatcher(
+      cxxNewExpr(isArray(), hasArraySize(BadArg)).bind("Alloc"), this);
 }
 
 void MisplacedOperatorInStrlenInAllocCheck::check(
     const MatchFinder::MatchResult &Result) {
-  const auto *Alloc = Result.Nodes.getNodeAs<CallExpr>("Alloc");
+  const Expr *Alloc = Result.Nodes.getNodeAs<CallExpr>("Alloc");
+  if (!Alloc)
+    Alloc = Result.Nodes.getNodeAs<CXXNewExpr>("Alloc");
+  assert(Alloc && "Matched node bound by 'Alloc' shoud be either 'CallExpr'"
+         " or 'CXXNewExpr'");
+
   const auto *StrLen = Result.Nodes.getNodeAs<CallExpr>("StrLen");
   const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("BinOp");
 
   const StringRef StrLenText = Lexer::getSourceText(
       CharSourceRange::getTokenRange(StrLen->getSourceRange()),
       *Result.SourceManager, getLangOpts());
-  const StringRef StrLenBegin = StrLenText.substr(0, StrLenText.find('(') + 1);
   const StringRef Arg0Text = Lexer::getSourceText(
       CharSourceRange::getTokenRange(StrLen->getArg(0)->getSourceRange()),
       *Result.SourceManager, getLangOpts());
+  const StringRef StrLenBegin = StrLenText.substr(0, StrLenText.find(Arg0Text));
   const StringRef StrLenEnd = StrLenText.substr(
       StrLenText.find(Arg0Text) + Arg0Text.size(), StrLenText.size());
 
   const StringRef LHSText = Lexer::getSourceText(
       CharSourceRange::getTokenRange(BinOp->getLHS()->getSourceRange()),
       *Result.SourceManager, getLangOpts());
   const StringRef RHSText = Lexer::getSourceText(
       CharSourceRange::getTokenRange(BinOp->getRHS()->getSourceRange()),

docs/ReleaseNotes.rst

 
 - New `bugprone-misplaced-operator-in-strlen-in-alloc
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.html>`_ check
 
   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()``).
+  ``alloca()``) or the ``new[]`` operator in `C++`.
 
 - Renamed checks to use correct term "implicit conversion" instead of "implicit
   cast" and modified messages and option names accordingly:
 
     * **performance-implicit-cast-in-loop** was renamed to
       `performance-implicit-conversion-in-loop
       <http://clang.llvm.org/extra/clang-tidy/checks/performance-implicit-conversion-in-loop.html>`_
     * **readability-implicit-bool-cast** was renamed to

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.
+allocation function (``malloc()``, ``calloc()``, ``realloc()``, ``alloca()``) or
+the ``new[]`` operator in `C++`. 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:
+`C` example code:
 
 .. code-block:: c
 
     void bad_malloc(char *str) {
       char *c = (char*) malloc(strlen(str + 1));
     }
 
 
 The suggested fix is to add ``1`` to the return value of ``strlen()`` and not
 to its argument. In the example above the fix would be
 
 .. code-block:: c
 
       char *c = (char*) malloc(strlen(str) + 1);
 
 
+`C++` example code:
+
+.. code-block:: c++
+
+    void bad_new(char *str) {
+      char *c = new char[strlen(str + 1)];
+    }
+
+
+As in the `C` code with the ``malloc()`` function, the suggested fix is to
+add ``1`` to the return value of ``strlen()`` and not to its argument. In the
+example above the fix would be
+
+.. code-block:: c++
+
+      char *c = new char[strlen(str) + 1];
+
+
 Example for silencing the diagnostic:
 
 .. 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.cpp

void ignore_non_std_malloc_std_strlen(char *name) {
   // Ignore functions of the malloc family in custom namespaces
 }
 
 void ignore_std_malloc_non_std_strlen(char *name) {
   char *new_name = (char *)std::malloc(non_std::strlen(name + 1));
   // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: addition operator is applied to the argument of strlen
   // Ignore functions of the strlen family in custom namespaces
 }
+
+void bad_new_strlen(char *name) {
+  char *new_name = new char[std::strlen(name + 1)];
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  char \*new_name = new char\[}}std::strlen(name) + 1{{\];$}}
+}
+
+void good_new_strlen(char *name) {
+  char *new_name = new char[std::strlen(name) + 1];
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:20: warning: addition operator is applied to the argument of strlen
+}
+
+class C {
+  char c;
+public:
+  static void *operator new[](std::size_t count) {
+    return ::operator new(count);
+  }
+};
+
+void bad_custom_new_strlen(char *name) {
+  C *new_name = new C[std::strlen(name + 1)];
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  C \*new_name = new C\[}}std::strlen(name) + 1{{\];$}}
+}
+