[clang-tidy] Add check bugprone-multiple-new-in-one-expression.

Reviewed By: donat.nagy

Differential Revision: https://reviews.llvm.org/D138777

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -36,6 +36,7 @@
 #include "MisplacedPointerArithmeticInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
+#include "MultipleNewInOneExpressionCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "NoEscapeCheck.h"
 #include "NonZeroEnumToBoolConversionCheck.h"
@@ -133,6 +134,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
+    CheckFactories.registerCheck<MultipleNewInOneExpressionCheck>(
+        "bugprone-multiple-new-in-one-expression");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
     CheckFactories.registerCheck<RedundantBranchConditionCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -31,6 +31,7 @@ add_clang_library(clangTidyBugproneModule
   MisplacedPointerArithmeticInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
+  MultipleNewInOneExpressionCheck.cpp
   MultipleStatementMacroCheck.cpp
   NoEscapeCheck.cpp
   NonZeroEnumToBoolConversionCheck.cpp

clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.cpp

@@ -0,0 +1,162 @@
+//===--- MultipleNewInOneExpressionCheck.cpp - clang-tidy------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MultipleNewInOneExpressionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+using namespace clang;
+
+namespace {
+
+// Determine if the result of an expression is "stored" in some way.
+// It is true if the value is stored into a variable or used as initialization
+// or passed to a function or constructor.
+// For this use case compound assignments are not counted as a "store" (the 'E'
+// expression should have pointer type).
+bool isExprValueStored(const Expr *E, ASTContext &C) {
+  E = E->IgnoreParenCasts();
+  // Get first non-paren, non-cast parent.
+  ParentMapContext &PMap = C.getParentMapContext();
+  DynTypedNodeList P = PMap.getParents(*E);
+  if (P.size() != 1)
+    return false;
+  const Expr *ParentE;
+  while ((ParentE = P[0].get<Expr>()) && ParentE->IgnoreParenCasts() == E) {
+    P = PMap.getParents(P[0]);
+    if (P.size() != 1)
+      return false;
+  }
+
+  if (const auto *ParentVarD = P[0].get<VarDecl>())
+    return ParentVarD->getInit()->IgnoreParenCasts() == E;
+
+  if (!ParentE)
+    return false;
+
+  if (const auto *BinOp = dyn_cast<BinaryOperator>(ParentE))
+    return BinOp->getOpcode() == BO_Assign &&
+           BinOp->getRHS()->IgnoreParenCasts() == E;
+
+  return isa<CallExpr, CXXConstructExpr>(ParentE);
+}
+
+} // namespace
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+AST_MATCHER_P(CXXTryStmt, hasHandlerFor,
+              ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
+  for (unsigned NH = Node.getNumHandlers(), I = 0; I < NH; ++I) {
+    const CXXCatchStmt *CatchS = Node.getHandler(I);
+    // Check for generic catch handler (match anything).
+    if (CatchS->getCaughtType().isNull())
+      return true;
+    ast_matchers::internal::BoundNodesTreeBuilder Result(*Builder);
+    if (InnerMatcher.matches(CatchS->getCaughtType(), Finder, &Result)) {
+      *Builder = std::move(Result);
+      return true;
+    }
+  }
+  return false;
+}
+
+AST_MATCHER(CXXNewExpr, mayThrow) {
+  FunctionDecl *OperatorNew = Node.getOperatorNew();
+  if (!OperatorNew)
+    return false;
+  return !OperatorNew->getType()->castAs<FunctionProtoType>()->isNothrow();
+}
+
+void MultipleNewInOneExpressionCheck::registerMatchers(MatchFinder *Finder) {
+  auto BadAllocType =
+      recordType(hasDeclaration(cxxRecordDecl(hasName("::std::bad_alloc"))));
+  auto ExceptionType =
+      recordType(hasDeclaration(cxxRecordDecl(hasName("::std::exception"))));
+  auto BadAllocReferenceType = referenceType(pointee(BadAllocType));
+  auto ExceptionReferenceType = referenceType(pointee(ExceptionType));
+
+  auto CatchBadAllocType =
+      qualType(hasCanonicalType(anyOf(BadAllocType, BadAllocReferenceType,
+                                      ExceptionType, ExceptionReferenceType)));
+  auto BadAllocCatchingTryBlock = cxxTryStmt(hasHandlerFor(CatchBadAllocType));
+
+  auto NewExprMayThrow = cxxNewExpr(mayThrow());
+  auto HasNewExpr1 = expr(anyOf(NewExprMayThrow.bind("new1"),
+                                hasDescendant(NewExprMayThrow.bind("new1"))));
+  auto HasNewExpr2 = expr(anyOf(NewExprMayThrow.bind("new2"),
+                                hasDescendant(NewExprMayThrow.bind("new2"))));
+
+  Finder->addMatcher(
+      callExpr(
+          hasAnyArgument(
+              expr(HasNewExpr1).bind("arg1")),
+          hasAnyArgument(
+              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+          hasAncestor(BadAllocCatchingTryBlock)),
+      this);
+  Finder->addMatcher(
+      cxxConstructExpr(
+          hasAnyArgument(
+              expr(HasNewExpr1).bind("arg1")),
+          hasAnyArgument(
+              expr(HasNewExpr2, unless(equalsBoundNode("arg1"))).bind("arg2")),
+          unless(isListInitialization()),
+          hasAncestor(BadAllocCatchingTryBlock)),
+      this);
+  Finder->addMatcher(binaryOperator(hasLHS(HasNewExpr1), hasRHS(HasNewExpr2),
+                                    unless(hasAnyOperatorName("&&", "||", ",")),
+                                    hasAncestor(BadAllocCatchingTryBlock)),
+                     this);
+  Finder->addMatcher(
+      cxxNewExpr(mayThrow(),
+                 hasDescendant(NewExprMayThrow.bind("new2_in_new1")),
+                 hasAncestor(BadAllocCatchingTryBlock))
+          .bind("new1"),
+      this);
+}
+
+void MultipleNewInOneExpressionCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *NewExpr1 = Result.Nodes.getNodeAs<CXXNewExpr>("new1");
+  const auto *NewExpr2 = Result.Nodes.getNodeAs<CXXNewExpr>("new2");
+  const auto *NewExpr2InNewExpr1 =
+      Result.Nodes.getNodeAs<CXXNewExpr>("new2_in_new1");
+  if (!NewExpr2)
+    NewExpr2 = NewExpr2InNewExpr1;
+  assert(NewExpr1 && NewExpr2 && "Bound nodes not found.");
+
+  // No warning if both allocations are not stored.
+  // The value may be intentionally not stored (no deallocations needed or
+  // self-destructing object).
+  if (!isExprValueStored(NewExpr1, *Result.Context) &&
+      !isExprValueStored(NewExpr2, *Result.Context))
+    return;
+
+  // In C++17 sequencing of a 'new' inside constructor arguments of another
+  // 'new' is fixed. Still a leak can happen if the returned value from the
+  // first 'new' is not saved (yet) and the second fails.
+  if (getLangOpts().CPlusPlus17 && NewExpr2InNewExpr1)
+    diag(NewExpr1->getBeginLoc(),
+         "memory allocation may leak if an other allocation is sequenced after "
+         "it and throws an exception")
+        << NewExpr1->getSourceRange() << NewExpr2->getSourceRange();
+  else
+    diag(NewExpr1->getBeginLoc(),
+         "memory allocation may leak if an other allocation is sequenced after "
+         "it and throws an exception; order of these allocations is undefined")
+        << NewExpr1->getSourceRange() << NewExpr2->getSourceRange();
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/bugprone/MultipleNewInOneExpressionCheck.h

@@ -0,0 +1,35 @@
+//===--- MultipleNewInOneExpressionCheck.h - clang-tidy----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MULTIPLENEWINONEEXPRESSIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MULTIPLENEWINONEEXPRESSIONCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/multiple-new-in-one-expression.html
+class MultipleNewInOneExpressionCheck : public ClangTidyCheck {
+public:
+  MultipleNewInOneExpressionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  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_MULTIPLENEWINONEEXPRESSIONCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -106,6 +106,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-multiple-new-in-one-expression
+  <clang-tidy/checks/bugprone/multiple-new-in-one-expression>` check.
+
+  Finds multiple ``new`` operator calls in a single expression, where the allocated
+  memory by the first ``new`` may leak if the second allocation fails and throws exception.
+
 - New :doc:`bugprone-non-zero-enum-to-bool-conversion
   <clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/multiple-new-in-one-expression.rst

@@ -0,0 +1,99 @@
+.. title:: clang-tidy - bugprone-multiple-new-in-one-expression
+
+bugprone-multiple-new-in-one-expression
+=======================================
+
+Finds multiple ``new`` operator calls in a single expression, where the
+allocated memory by the first ``new`` may leak if the second allocation fails
+and throws exception.
+
+C++ does often not specify the exact order of evaluation of the operands of an
+operator or arguments of a function. Therefore if a first allocation succeeds
+and a second fails, in an exception handler it is not possible to tell which
+allocation has failed and free the memory. Even if the order is fixed the result
+of a first ``new`` may be stored in a temporary location that is not reachable
+at the time when a second allocation fails. It is best to avoid any expression
+that contains more than one ``operator new`` call, if exception handling is
+used to check for allocation errors.
+
+Different rules apply for are the short-circuit operators ``||`` and ``&&`` and
+the ``,`` operator, where evaluation of one side must be completed before the
+other starts. Expressions of a list-initialization (initialization or
+construction using ``{`` and ``}`` characters) are evaluated in fixed order.
+Similarly, condition of a ``?`` operator is evaluated before the branches are
+evaluated.
+
+The check reports warning if two ``new`` calls appear in one expression at
+different sides of an operator, or if ``new`` calls appear in different
+arguments of a function call (that can be an object construction with ``()``
+syntax). These ``new`` calls can be nested at any level.
+For any warning to be emitted the ``new`` calls should be in a code block where
+exception handling is used with catch for ``std::bad_alloc`` or
+``std::exception``. At ``||``, ``&&``, ``,``, ``?`` (condition and one branch)
+operators no warning is emitted. No warning is emitted if both of the memory
+allocations are not assigned to a variable or not passed directly to a function.
+The reason is that in this case the memory may be intentionally not freed or the
+allocated objects can be self-destructing objects.
+
+Examples:
+
+.. code-block:: c++
+
+  struct A {
+    int Var;
+  };
+  struct B {
+    B();
+    B(A *);
+    int Var;
+  };
+  struct C {
+    int *X1;
+    int *X2;
+  };
+
+  void f(A *, B *);
+  int f1(A *);
+  int f1(B *);
+  bool f2(A *);
+
+  void foo() {
+    A *PtrA;
+    B *PtrB;
+    try {
+      // Allocation of 'B'/'A' may fail after memory for 'A'/'B' was allocated.
+      f(new A, new B); // warning: memory allocation may leak if an other allocation is sequenced after it and throws an exception; order of these allocations is undefined
+
+      // List (aggregate) initialization is used.
+      C C1{new int, new int}; // no warning
+
+      // Allocation of 'B'/'A' may fail after memory for 'A'/'B' was allocated but not yet passed to function 'f1'.
+      int X = f1(new A) + f1(new B); // warning: memory allocation may leak if an other allocation is sequenced after it and throws an exception; order of these allocations is undefined
+
+      // Allocation of 'B' may fail after memory for 'A' was allocated.
+      // From C++17 on memory for 'B' is allocated first but still may leak if allocation of 'A' fails.
+      PtrB = new B(new A); // warning: memory allocation may leak if an other allocation is sequenced after it and throws an exception
+
+      // 'new A' and 'new B' may be performed in any order.
+      // 'new B'/'new A' may fail after memory for 'A'/'B' was allocated but not assigned to 'PtrA'/'PtrB'.
+      (PtrA = new A)->Var = (PtrB = new B)->Var; // warning: memory allocation may leak if an other allocation is sequenced after it and throws an exception; order of these allocations is undefined
+
+      // Evaluation of 'f2(new A)' must be finished before 'f1(new B)' starts.
+      // If 'new B' fails the allocated memory for 'A' is supposedly handled correctly because function 'f2' could take the ownership.
+      bool Z = f2(new A) || f1(new B); // no warning
+
+      X = (f2(new A) ? f1(new A) : f1(new B)); // no warning
+
+      // No warning if the result of both allocations is not passed to a function
+      // or stored in a variable.
+      (new A)->Var = (new B)->Var; // no warning
+
+      // No warning if at least one non-throwing allocation is used.
+      f(new(std::nothrow) A, new B); // no warning
+    } catch(std::bad_alloc) {
+    }
+
+    // No warning if the allocation is outside a try block (or no catch handler exists for std::bad_alloc).
+    // (The fact if exceptions can escape from 'foo' is not taken into account.)
+    f(new A, new B); // no warning
+  }

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -102,6 +102,7 @@ Clang-Tidy Checks
    `bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-widening-cast <bugprone/misplaced-widening-cast.html>`_,
    `bugprone-move-forwarding-reference <bugprone/move-forwarding-reference.html>`_, "Yes"
+   `bugprone-multiple-new-in-one-expression <bugprone/multiple-new-in-one-expression.html>`_,
    `bugprone-multiple-statement-macro <bugprone/multiple-statement-macro.html>`_,
    `bugprone-no-escape <bugprone/no-escape.html>`_,
    `bugprone-non-zero-enum-to-bool-conversion <bugprone/non-zero-enum-to-bool-conversion.html>`_,