[clang-tools-extra] 6d9eb53 - [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 4 00:15:49 PDT 2022
Author: Balázs Kéri
Date: 2022-10-04T09:14:46+02:00
New Revision: 6d9eb533291377979882ac9674431eddd1248445
URL: https://github.com/llvm/llvm-project/commit/6d9eb533291377979882ac9674431eddd1248445
DIFF: https://github.com/llvm/llvm-project/commit/6d9eb533291377979882ac9674431eddd1248445.diff
LOG: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.
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.
Reviewed By: steakhal, martong
Differential Revision: https://reviews.llvm.org/D133119
Added:
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h
clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp
Modified:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 961480a23f8f1..34386a0cb0154 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -58,6 +58,7 @@
#include "SuspiciousMemoryComparisonCheck.h"
#include "SuspiciousMemsetUsageCheck.h"
#include "SuspiciousMissingCommaCheck.h"
+#include "SuspiciousReallocUsageCheck.h"
#include "SuspiciousSemicolonCheck.h"
#include "SuspiciousStringCompareCheck.h"
#include "SwappedArgumentsCheck.h"
@@ -173,6 +174,8 @@ class BugproneModule : public ClangTidyModule {
"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>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 1c7b0c2519f77..34c019357dab5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -54,6 +54,7 @@ add_clang_library(clangTidyBugproneModule
SuspiciousMemoryComparisonCheck.cpp
SuspiciousMemsetUsageCheck.cpp
SuspiciousMissingCommaCheck.cpp
+ SuspiciousReallocUsageCheck.cpp
SuspiciousSemicolonCheck.cpp
SuspiciousStringCompareCheck.cpp
SwappedArgumentsCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
new file mode 100644
index 0000000000000..bb975bc893d0d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
@@ -0,0 +1,163 @@
+//===--- 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 "../utils/Aliasing.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 {
+/// Check if two
diff erent expression nodes denote the same
+/// "pointer expression". The "pointer expression" can consist of member
+/// expressions and declaration references only (like \c a->b->c), otherwise the
+/// check is always false.
+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));
+ }
+};
+
+/// Check if there is an assignment or initialization that references a variable
+/// \c Var (at right-hand side) and is before \c VarRef in the source code.
+/// Only simple assignments like \code a = b \endcode are found.
+class FindAssignToVarBefore
+ : public ConstStmtVisitor<FindAssignToVarBefore, bool> {
+ const VarDecl *Var;
+ const DeclRefExpr *VarRef;
+ SourceManager &SM;
+
+ bool isAccessForVar(const Expr *E) const {
+ if (const auto *DeclRef = dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
+ return DeclRef->getDecl() &&
+ DeclRef->getDecl()->getCanonicalDecl() == Var &&
+ SM.isBeforeInTranslationUnit(E->getBeginLoc(),
+ VarRef->getBeginLoc());
+ return false;
+ }
+
+public:
+ FindAssignToVarBefore(const VarDecl *Var, const DeclRefExpr *VarRef,
+ SourceManager &SM)
+ : Var(Var->getCanonicalDecl()), VarRef(VarRef), SM(SM) {}
+
+ bool VisitDeclStmt(const DeclStmt *S) {
+ for (const Decl *D : S->getDeclGroup())
+ if (const auto *LeftVar = dyn_cast<VarDecl>(D))
+ if (LeftVar->hasInit())
+ return isAccessForVar(LeftVar->getInit());
+ return false;
+ }
+ bool VisitBinaryOperator(const BinaryOperator *S) {
+ if (S->getOpcode() == BO_Assign)
+ return isAccessForVar(S->getRHS());
+ return false;
+ }
+ bool VisitStmt(const Stmt *S) {
+ for (const Stmt *Child : S->children())
+ if (Visit(Child))
+ return true;
+ return false;
+ }
+};
+
+} // 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")),
+ hasAncestor(functionDecl().bind("parent_function")))
+ .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.");
+ SourceManager &SM = ReallocD->getASTContext().getSourceManager();
+
+ if (!IsSamePtrExpr{}.check(PtrInputExpr, PtrResultExpr))
+ return;
+
+ if (const auto *DeclRef =
+ dyn_cast<DeclRefExpr>(PtrInputExpr->IgnoreParenImpCasts()))
+ if (const auto *Var = dyn_cast<VarDecl>(DeclRef->getDecl()))
+ if (const auto *Func =
+ Result.Nodes.getNodeAs<FunctionDecl>("parent_function"))
+ if (FindAssignToVarBefore{Var, DeclRef, SM}.Visit(Func->getBody()))
+ return;
+
+ StringRef CodeOfAssignedExpr = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(PtrResultExpr->getSourceRange()), SM,
+ 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
diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h
new file mode 100644
index 0000000000000..150a7637c8859
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h
@@ -0,0 +1,36 @@
+//===--- 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
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8e65ed214a21d..199f46ae14334 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -99,6 +99,12 @@ 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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst
new file mode 100644
index 0000000000000..67e416b711b64
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst
@@ -0,0 +1,44 @@
+.. 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 (used at ``realloc``) can be a variable or a field member
+of a data structure, but can not contain function calls or unresolved types.
+
+In obvious cases when the pointer used at realloc is assigned to another
+variable before the ``realloc`` call, no warning is emitted. This happens only
+if a simple expression in form of ``q = p`` or ``void *q = p`` is found in the
+same function where ``p = realloc(p, ...)`` is found. The assignment has to be
+before the call to realloc (but otherwise at any place) in the same function.
+This suppression works only if ``p`` is a single variable.
+
+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
+ }
+
+ void foo1(void *p, int new_size) {
+ void *p1 = p;
+ p = realloc(p, new_size); // no warning
+ }
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index b777c57290c05..7b588bd1edada 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -124,6 +124,7 @@ Clang-Tidy Checks
`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"
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp
new file mode 100644
index 0000000000000..6e3c7f4174845
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp
@@ -0,0 +1,102 @@
+// 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 no_warn_if_copy_exists_before1(void *p) {
+ void *q = p;
+ p = realloc(p, 111);
+}
+
+void no_warn_if_copy_exists_before2(void *p, void *q) {
+ q = p;
+ p = realloc(p, 111);
+}
+
+void *g_p;
+
+void no_warn_if_copy_exists_before3() {
+ void *q = g_p;
+ g_p = realloc(g_p, 111);
+}
+
+void warn_if_copy_exists_after(void *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]
+ void *q = p;
+}
More information about the cfe-commits
mailing list