[clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

[balazske]

Summary

Add a check to detect usages of realloc where the result is assigned
to the same variable (or field) as passed to the first argument.

[steakhal]

Consider adding a test case demonstrating if the checker recognizes and suppresses the check if there are multiple variables referring to the memory being reallocated.
If the check does not handle that case, we should probably state this limitation explicitly. And marking the corresponding test case with a FIXME.

I was thinking about something like this:

void escape(void *);
void foo(void *p) {
  void *q = p;
  p = realloc(p, 10); // no-warning
  escape(q);
}
void bar(void *p) {
  p = realloc(p, 10); // warn: ...
  void *q = p;
  escape(q);
}

The MallocOverflowCheck does something similar. It uses isBeforeInTranslationUnit().

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

21–22

Commented code?

24

against

[balazske]

I see that there are problems to solve with this patch. If a global variable is used at realloc or the pointer is a function parameter, it may have aliases that are not checkable. In the function code it is possible to detect aliasing. If all these cases are eliminated probably not much is left that is detected by the check, but we can test it.

[martong]

We had a discussion about this with @dkrupp . We think that the p = realloc(p, var) construct in itself is an error-prone style of using realloc. This style does not necessarily give birth to erroneous code (see the error-free escaping example in foo above from @steakhal). However, the form p = realloc(p, var) is an indication that the programmer might missed the non-trivial error handling case, and chances are high. Thus, one should use the p2 = realloc(p1, var) form as a best practice. So, this looks good to me, but please consider this a weak approve and wait for someone else's approve who has more confidence in clang-tidy (@whisperity could you please take a look?).

[whisperity]

I agree with @martong that $ = realloc($, ...) is indicative of something going wrong, even if the developer has saved the original value somewhere. Are we trying to catch this with Tidy specifically for this reason (instead of CSA's stdlib checker, or some taint-related mechanism?). However, @steakhal has some merit in saying that developers might prioritise the original over the copy, even if they made a copy. I think an easy solution from a documentation perspective is to have a test for this at the bottom of the test file. And document that we cannot (for one reason or the other) catch this supposed solution, but if you have made a copy already(!), then you might as well could have done void* q = realloc(p, ...) anyway! Having this documented leaves at least some paths open for us to fix false positives later, if they become a tremendous problem. (I have a gut feeling that they will not, that much.)

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

21–22

(Nit: using namespace clang -> The clang:: should be redundant here.)

88

(We're creating a temporary here, right?)

[whisperity]

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

90–91

I have some reservations about this message, though. It only indirectly states the problem: first, the user needs to know what a memory leak is, and then needs to know how realloc could fail, and then make the mental connection about the overwriting of the pointer. (Also, you're passing a pointer... "memory block" is a more abstract term, requiring more knowledge.)

I think for the sake of clarity, we should be more explicit about this.

taking the returned pointer from 'realloc' overwrites 'XXXPtr' with null
if allocation fails,
which may result in a leak of the original buffer

1st line explains how the issue manifests (explicitly saying that you'll get a nullpointer in a bad place)
2nd line gives the condition, which is a bit more explicit
3rd line explains the real underlying issue

(Getting the null pointer is the "only" bad path here.)

[whisperity]

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

90–91

Also:

diag(...) << PtrInputExpr.getSourceRange() << PtrResultExpr.getSourceRange();

So the connection that the recipient of the assignment and the 1st parameter is the same is more obvious.

[balazske]

Implemented review comments, changed warning text, changed documentation.

[balazske]

added test, added a comment

[balazske]

rebase to current main

[steakhal]

LGTM, no objection here.

clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp

81–85

I believe we should also put a FIXME here.

[balazske]

Added simple check for create of copy of pointer before the realloc call.

[balazske]

I added a simple detection of create a copy of p before p = realloc(p, ...). This can remove the warning at very obvious cases when a copy of p is created (but even if the copy is made inside an if branch for example).

[martong]

In D133119#3827432, @balazske wrote:

I added a simple detection of create a copy of p before p = realloc(p, ...). This can remove the warning at very obvious cases when a copy of p is created (but even if the copy is made inside an if branch for example).

Okay, I am comfortable with this update, even though the recognition of the assignment cannot be fully precise. So, still looks good to me.

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

 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "StringviewNullptrCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousIncludeCheck.h"
 #include "SuspiciousMemoryComparisonCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
+#include "SuspiciousReallocUsageCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
 #include "UncheckedOptionalAccessCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousIncludeCheck>(
         "bugprone-suspicious-include");
     CheckFactories.registerCheck<SuspiciousMemoryComparisonCheck>(
         "bugprone-suspicious-memory-comparison");
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
     CheckFactories.registerCheck<SuspiciousMissingCommaCheck>(
         "bugprone-suspicious-missing-comma");
+    CheckFactories.registerCheck<SuspiciousReallocUsageCheck>(
+        "bugprone-suspicious-realloc-usage");
     CheckFactories.registerCheck<SuspiciousSemicolonCheck>(
         "bugprone-suspicious-semicolon");
     CheckFactories.registerCheck<SuspiciousStringCompareCheck>(
         "bugprone-suspicious-string-compare");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "bugprone-swapped-arguments");
     CheckFactories.registerCheck<TerminatingContinueCheck>(
         "bugprone-terminating-continue");

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

add_clang_library(clangTidyBugproneModule
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   StringviewNullptrCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousIncludeCheck.cpp
   SuspiciousMemoryComparisonCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
+  SuspiciousReallocUsageCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp
   UncheckedOptionalAccessCheck.cpp
   UndefinedMemoryManipulationCheck.cpp

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

+//===--- SuspiciousReallocUsageCheck.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_SUSPICIOUSREALLOCUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SUSPICIOUSREALLOCUSAGECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds usages of ``realloc`` where the return value is assigned to the same
+/// variable as passed to the first argument.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-realloc-usage.html
+class SuspiciousReallocUsageCheck : public ClangTidyCheck {
+public:
+  SuspiciousReallocUsageCheck(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_SUSPICIOUSREALLOCUSAGECHECK_H

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

+//===--- SuspiciousReallocUsageCheck.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 "SuspiciousReallocUsageCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclVisitor.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+using namespace clang;
+
+namespace {
+class IsSamePtrExpr : public StmtVisitor<IsSamePtrExpr, bool> {
+  /// The other expression to compare against.
+  /// This variable is used to pass the data from a \c check function to any of
+  /// the visit functions. Every visit function starts by converting \c OtherE
+  /// to the current type and store it locally, and do not use \c OtherE later.
+  const Expr *OtherE = nullptr;
+
+public:
+  bool VisitDeclRefExpr(const DeclRefExpr *E1) {
+    const auto *E2 = dyn_cast<DeclRefExpr>(OtherE);
+    if (!E2)
+      return false;
+    const Decl *D1 = E1->getDecl()->getCanonicalDecl();
+    return isa<VarDecl, FieldDecl>(D1) &&
+           D1 == E2->getDecl()->getCanonicalDecl();
+  }
+
+  bool VisitMemberExpr(const MemberExpr *E1) {
+    const auto *E2 = dyn_cast<MemberExpr>(OtherE);
+    if (!E2)
+      return false;
+    if (!check(E1->getBase(), E2->getBase()))
+      return false;
+    DeclAccessPair FD = E1->getFoundDecl();
+    return isa<FieldDecl>(FD.getDecl()) && FD == E2->getFoundDecl();
+  }
+
+  bool check(const Expr *E1, const Expr *E2) {
+    E1 = E1->IgnoreParenCasts();
+    E2 = E2->IgnoreParenCasts();
+    OtherE = E2;
+    return Visit(const_cast<Expr *>(E1));
+  }
+};
+} // namespace
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void SuspiciousReallocUsageCheck::registerMatchers(MatchFinder *Finder) {
+  // void *realloc(void *ptr, size_t size);
+  auto ReallocDecl =
+      functionDecl(hasName("::realloc"), parameterCountIs(2),
+                   hasParameter(0, hasType(pointerType(pointee(voidType())))),
+                   hasParameter(1, hasType(isInteger())))
+          .bind("realloc");
+
+  auto ReallocCall =
+      callExpr(callee(ReallocDecl), hasArgument(0, expr().bind("ptr_input")))
+          .bind("call");
+  Finder->addMatcher(binaryOperator(hasOperatorName("="),
+                                    hasLHS(expr().bind("ptr_result")),
+                                    hasRHS(ignoringParenCasts(ReallocCall))),
+                     this);
+}
+
+void SuspiciousReallocUsageCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+  if (!Call)
+    return;
+  const auto *PtrInputExpr = Result.Nodes.getNodeAs<Expr>("ptr_input");
+  const auto *PtrResultExpr = Result.Nodes.getNodeAs<Expr>("ptr_result");
+  if (!PtrInputExpr || !PtrResultExpr)
+    return;
+  const auto *ReallocD = Result.Nodes.getNodeAs<Decl>("realloc");
+  assert(ReallocD && "Value for 'realloc' should exist if 'call' was found.");
+  if (!IsSamePtrExpr{}.check(PtrInputExpr, PtrResultExpr))
+    return;
+  // utils::hasPtrOrReferenceInFunc may provide a starting point to filter out
+  // cases like "void *q = p; p = realloc(p, ...);"
+  StringRef CodeOfAssignedExpr = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(PtrResultExpr->getSourceRange()),
+      ReallocD->getASTContext().getSourceManager(), getLangOpts());
+  diag(Call->getBeginLoc(), "'%0' may be set to null if 'realloc' fails, which "
+                            "may result in a leak of the original buffer")
+      << CodeOfAssignedExpr << PtrInputExpr->getSourceRange()
+      << PtrResultExpr->getSourceRange();
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-suspicious-realloc-usage
+  <clang-tidy/checks/bugprone/suspicious-realloc-usage>` check.
+
+  Finds usages of ``realloc`` where the return value is assigned to the
+  same expression as passed to the first argument.
+
 - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
   <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst

+.. title:: clang-tidy - bugprone-suspicious-realloc-usage
+
+bugprone-suspicious-realloc-usage
+=================================
+
+This check finds usages of ``realloc`` where the return value is assigned to the
+same expression as passed to the first argument:
+``p = realloc(p, size);``
+The problem with this construct is that if ``realloc`` fails it returns a
+null pointer but does not deallocate the original memory. If no other variable
+is pointing to it, the original memory block is not available any more for the
+program to use or free. In either case ``p = realloc(p, size);`` indicates bad
+coding style and can be replaced by ``q = realloc(p, size);``.
+
+The pointer expression can be a variable or a field member of a data structure,
+but can not contain function calls or unresolved types.
+
+Examples:
+
+.. code-block:: c++
+
+  struct A {
+    void *p;
+  };
+
+  A &getA();
+
+  void foo(void *p, A *a, int new_size) {
+    p = realloc(p, new_size); // warning: 'p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer
+    a->p = realloc(a->p, new_size); // warning: 'a->p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer
+    getA().p = realloc(getA().p, new_size); // no warning
+  }

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

.. csv-table::
    `bugprone-string-integer-assignment <bugprone/string-integer-assignment.html>`_, "Yes"
    `bugprone-string-literal-with-embedded-nul <bugprone/string-literal-with-embedded-nul.html>`_,
    `bugprone-stringview-nullptr <bugprone/stringview-nullptr.html>`_, "Yes"
    `bugprone-suspicious-enum-usage <bugprone/suspicious-enum-usage.html>`_,
    `bugprone-suspicious-include <bugprone/suspicious-include.html>`_,
    `bugprone-suspicious-memory-comparison <bugprone/suspicious-memory-comparison.html>`_,
    `bugprone-suspicious-memset-usage <bugprone/suspicious-memset-usage.html>`_, "Yes"
    `bugprone-suspicious-missing-comma <bugprone/suspicious-missing-comma.html>`_,
+   `bugprone-suspicious-realloc-usage <bugprone/suspicious-realloc-usage.html>`_,
    `bugprone-suspicious-semicolon <bugprone/suspicious-semicolon.html>`_, "Yes"
    `bugprone-suspicious-string-compare <bugprone/suspicious-string-compare.html>`_, "Yes"
    `bugprone-swapped-arguments <bugprone/swapped-arguments.html>`_, "Yes"
    `bugprone-terminating-continue <bugprone/terminating-continue.html>`_, "Yes"
    `bugprone-throw-keyword-missing <bugprone/throw-keyword-missing.html>`_,
    `bugprone-too-small-loop-variable <bugprone/too-small-loop-variable.html>`_,
    `bugprone-unchecked-optional-access <bugprone/unchecked-optional-access.html>`_,
    `bugprone-undefined-memory-manipulation <bugprone/undefined-memory-manipulation.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp

+// RUN: %check_clang_tidy %s bugprone-suspicious-realloc-usage %t
+
+void *realloc(void *, __SIZE_TYPE__);
+
+namespace std {
+  using ::realloc;
+}
+
+namespace n1 {
+  void *p;
+}
+
+namespace n2 {
+  void *p;
+}
+
+struct P {
+  void *p;
+  void *q;
+  P *pp;
+  void *&f();
+};
+
+struct P1 {
+  static void *p;
+  static void *q;
+};
+
+template<class>
+struct P2 {
+  static void *p;
+  static void *q;
+};
+
+template<class A, class B>
+void templ(void *p) {
+  A::p = realloc(A::p, 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 'A::p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+  p = realloc(A::p, 10);
+  A::q = realloc(A::p, 10);
+  A::p = realloc(B::p, 10);
+  P2<A>::p = realloc(P2<A>::p, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'P2<A>::p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+  P2<A>::p = realloc(P2<B>::p, 1);
+}
+
+void *&getPtr();
+P &getP();
+
+void warn(void *p, P *p1, int *pi) {
+  p = realloc(p, 111);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+
+  p = std::realloc(p, sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+
+  p1->p = realloc(p1->p, 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 'p1->p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+
+  p1->pp->p = realloc(p1->pp->p, 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'p1->pp->p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+
+  pi = (int*)realloc(pi, 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'pi' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+
+  templ<P1, P2<int>>(p);
+}
+
+void no_warn(void *p, P *p1, P *p2) {
+  void *q = realloc(p, 10);
+  q = realloc(p, 10);
+  p1->q = realloc(p1->p, 10);
+  p2->p = realloc(p1->p, 20);
+  n1::p = realloc(n2::p, 30);
+  p1->pp->p = realloc(p1->p, 10);
+  getPtr() = realloc(getPtr(), 30);
+  getP().p = realloc(getP().p, 20);
+  p1->f() = realloc(p1->f(), 30);
+}
+
+void warn_if_copy_exists(void *p) {
+  void *q = p;
+  p = realloc(p, 111);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage]
+}