balazske updated this revision to Diff 463575.
balazske marked 6 inline comments as done.
balazske added a comment.
Implemented review comments, changed warning text, changed documentation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133119/new/
https://reviews.llvm.org/D133119
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-realloc-usage.cpp
@@ -0,0 +1,79 @@
+// 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 foo(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);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -124,6 +124,7 @@
`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"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/suspicious-realloc-usage.rst
@@ -0,0 +1,32 @@
+.. 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
+ }
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -99,6 +99,12 @@
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.
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp
@@ -0,0 +1,102 @@
+//===--- 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;
+ 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
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -54,6 +54,7 @@
SuspiciousMemoryComparisonCheck.cpp
SuspiciousMemsetUsageCheck.cpp
SuspiciousMissingCommaCheck.cpp
+ SuspiciousReallocUsageCheck.cpp
SuspiciousSemicolonCheck.cpp
SuspiciousStringCompareCheck.cpp
SwappedArgumentsCheck.cpp
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ 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 @@
"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>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
