[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

37

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/BugproneTidyModule.cpp

 //===--- BugproneTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "IntegerDivisionCheck.h"
+#include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 class BugproneModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
+    CheckFactories.registerCheck<MisplacedOperatorInStrlenInAllocCheck>(
+        "bugprone-misplaced-operator-in-strlen-in-alloc");
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
   }
 };
 
 } // namespace bugprone

clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   BugproneTidyModule.cpp
   IntegerDivisionCheck.cpp
+  MisplacedOperatorInStrlenInAllocCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.h

+//===--- MisplacedOperatorInStrlenInAllocCheck.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_BUGPRONE_MISPLACED_OPERATOR_IN_STRLEN_IN_ALLOC_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISPLACED_OPERATOR_IN_STRLEN_IN_ALLOC_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds cases where ``1`` is added to the string in the parameter of a
+/// function in the ``strlen()`` family instead of to the result and use its
+/// return value as an argument of a memory allocation function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.html
+class MisplacedOperatorInStrlenInAllocCheck : public ClangTidyCheck {
+public:
+  MisplacedOperatorInStrlenInAllocCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MISPLACED_OPERATOR_IN_STRLEN_IN_ALLOC_H

clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp

+//===--- MisplacedOperatorInStrlenInAllocCheck.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 "MisplacedOperatorInStrlenInAllocCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void MisplacedOperatorInStrlenInAllocCheck::registerMatchers(
+    MatchFinder *Finder) {
+  const auto StrLenFunc = functionDecl(anyOf(
+      hasName("::strlen"), hasName("::std::strlen"), hasName("::strnlen"),
+      hasName("::std::strnlen"), hasName("::strnlen_s"),
+      hasName("::std::strnlen_s"), hasName("::wcslen"),
+      hasName("::std::wcslen"), hasName("::wcsnlen"), hasName("::std::wcsnlen"),
+      hasName("::wcsnlen_s"), hasName("std::wcsnlen_s")));
+
+  const auto BadUse =
+      callExpr(callee(StrLenFunc),
+               hasAnyArgument(ignoringImpCasts(
+                   binaryOperator(allOf(hasOperatorName("+"),
+                                        hasRHS(ignoringParenImpCasts(
+                                            integerLiteral(equals(1))))))
+                       .bind("BinOp"))))
+          .bind("StrLen");
+
+  const auto BadArg = anyOf(
+      allOf(hasDescendant(BadUse),
+            unless(binaryOperator(allOf(
+                hasOperatorName("+"), hasLHS(BadUse),
+                hasRHS(ignoringParenImpCasts(integerLiteral(equals(1)))))))),
+      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 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 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()),
+      *Result.SourceManager, getLangOpts());
+
+  auto Hint = FixItHint::CreateReplacement(
+      StrLen->getSourceRange(),
+      (StrLenBegin + LHSText + StrLenEnd + " + " + RHSText).str());
+
+  diag(Alloc->getLocStart(),
+       "Addition operator is applied to the argument of "
+       "%0 instead of its result; surround the addition subexpression with "
+       "parentheses to silence this warning")
+      << StrLen->getDirectCallee()->getName() << Hint;
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- 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 parameter of
+  ``strlen()``, ``strnlen()``, ``strnlen_s()``, ``wcslen()``, ``wcsnlen()`` and
+  ``wcsnlen_s()`` functions instead of to the result and use its return value as
+  an argument of a memory allocation function (``malloc()``, ``calloc()``,
+  ``realloc()``, ``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
       `readability-implicit-bool-conversion

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 parameter of ``strlen()``,
+``strnlen()``, ``strnlen_s()``, ``wcslen()``, ``wcsnlen()`` and ``wcsnlen_s()``
+functions instead of to the result and use its return value as an argument of a
+memory 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.
+
+`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 bug report:
+
+.. code-block:: c
+
+    void bad_malloc(char *str) {
+      char *c = (char*) malloc(strlen((str + 1)));
+    }

docs/clang-tidy/checks/list.rst

.. toctree::
    android-cloexec-fopen
    android-cloexec-inotify-init
    android-cloexec-inotify-init1
    android-cloexec-memfd-create
    android-cloexec-open
    android-cloexec-socket
    boost-use-to-string
    bugprone-integer-division
+   bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-suspicious-memset-usage
    bugprone-undefined-memory-manipulation
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>

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

+// RUN: %check_clang_tidy %s bugprone-misplaced-operator-in-strlen-in-alloc %t
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *realloc(void *, size_t);
+
+size_t strlen(const char*);
+size_t strnlen(const char*, size_t);
+size_t strnlen_s(const char*, size_t);
+
+typedef unsigned wchar_t;
+
+size_t wcslen(const wchar_t*);
+size_t wcsnlen(const wchar_t*, size_t);
+size_t wcsnlen_s(const wchar_t*, size_t);
+
+void bad_malloc(char *name) {
+  char *new_name = (char*) malloc(strlen(name + 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: Addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  char \*new_name = \(char\*\) malloc\(}}strlen(name) + 1{{\);$}}
+  new_name = (char*) malloc(strnlen(name + 1, 10));
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Addition operator is applied to the argument of strnlen
+  // CHECK-FIXES: {{^  new_name = \(char\*\) malloc\(}}strnlen(name, 10) + 1{{\);$}}
+  new_name = (char*) malloc(strnlen_s(name + 1, 10));
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Addition operator is applied to the argument of strnlen_s
+  // CHECK-FIXES: {{^  new_name = \(char\*\) malloc\(}}strnlen_s(name, 10) + 1{{\);$}}
+}
+
+void bad_malloc_wide(wchar_t *name) {
+  wchar_t *new_name = (wchar_t*) malloc(wcslen(name + 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: Addition operator is applied to the argument of wcslen
+  // CHECK-FIXES: {{^  wchar_t \*new_name = \(wchar_t\*\) malloc\(}}wcslen(name) + 1{{\);$}}
+  new_name = (wchar_t*) malloc(wcsnlen(name + 1, 10));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: Addition operator is applied to the argument of wcsnlen
+  // CHECK-FIXES: {{^  new_name = \(wchar_t\*\) malloc\(}}wcsnlen(name, 10) + 1{{\);$}}
+  new_name = (wchar_t*) malloc(wcsnlen_s(name + 1, 10));
+  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: Addition operator is applied to the argument of wcsnlen_s
+  // CHECK-FIXES: {{^  new_name = \(wchar_t\*\) malloc\(}}wcsnlen_s(name, 10) + 1{{\);$}}
+}
+
+void bad_alloca(char *name) {
+  char *new_name = (char*) alloca(strlen(name + 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: Addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  char \*new_name = \(char\*\) alloca\(}}strlen(name) + 1{{\);$}}
+}
+
+void bad_calloc(char *name) {
+  char *new_names = (char*) calloc(2, strlen(name + 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: Addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  char \*new_names = \(char\*\) calloc\(2, }}strlen(name) + 1{{\);$}}
+}
+
+void bad_realloc(char * old_name, char *name) {
+  char *new_name = (char*) realloc(old_name, strlen(name + 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: Addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  char \*new_name = \(char\*\) realloc\(old_name, }}strlen(name) + 1{{\);$}}
+}
+
+void intentional1(char *name) {
+  char *new_name = (char*) malloc(strlen(name + 1) + 1);
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: Addition operator is applied to the argument of strlen
+  // We have + 1 outside as well so we assume this is intentional
+}
+
+void intentional2(char *name) {
+  char *new_name = (char*) malloc(strlen(name + 2));
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:28: warning: Addition operator is applied to the argument of strlen
+  // 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
+}
+

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

+// RUN: %check_clang_tidy %s bugprone-misplaced-operator-in-strlen-in-alloc %t
+
+namespace std {
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+
+size_t strlen(const char*);
+}
+
+namespace non_std {
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+
+size_t strlen(const char*);
+}
+
+void bad_std_malloc_std_strlen(char *name) {
+  char *new_name = (char*) std::malloc(std::strlen(name + 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: Addition operator is applied to the argument of strlen
+  // CHECK-FIXES: {{^  char \*new_name = \(char\*\) std::malloc\(}}std::strlen(name) + 1{{\);$}}
+}
+
+void ignore_non_std_malloc_std_strlen(char *name) {
+  char *new_name = (char*) non_std::malloc(std::strlen(name + 1));
+  // 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));
+  // 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{{\];$}}
+}
+