diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- 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 @@ "bugprone-suspicious-memset-usage"); CheckFactories.registerCheck( "bugprone-suspicious-missing-comma"); + CheckFactories.registerCheck( + "bugprone-suspicious-realloc-usage"); CheckFactories.registerCheck( "bugprone-suspicious-semicolon"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/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 diff --git a/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.h new file mode 100644 --- /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/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp new file mode 100644 --- /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 different 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 { + /// 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(OtherE); + if (!E2) + return false; + const Decl *D1 = E1->getDecl()->getCanonicalDecl(); + return isa(D1) && + D1 == E2->getDecl()->getCanonicalDecl(); + } + + bool VisitMemberExpr(const MemberExpr *E1) { + const auto *E2 = dyn_cast(OtherE); + if (!E2) + return false; + if (!check(E1->getBase(), E2->getBase())) + return false; + DeclAccessPair FD = E1->getFoundDecl(); + return isa(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(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 { + const VarDecl *Var; + const DeclRefExpr *VarRef; + SourceManager &SM; + + bool isAccessForVar(const Expr *E) const { + if (const auto *DeclRef = dyn_cast(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(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("call"); + if (!Call) + return; + const auto *PtrInputExpr = Result.Nodes.getNodeAs("ptr_input"); + const auto *PtrResultExpr = Result.Nodes.getNodeAs("ptr_result"); + if (!PtrInputExpr || !PtrResultExpr) + return; + const auto *ReallocD = Result.Nodes.getNodeAs("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(PtrInputExpr->IgnoreParenImpCasts())) + if (const auto *Var = dyn_cast(DeclRef->getDecl())) + if (const auto *Func = + Result.Nodes.getNodeAs("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/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -99,6 +99,12 @@ New checks ^^^^^^^^^^ +- New :doc:`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 ` 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 --- /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 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -124,6 +124,7 @@ `bugprone-suspicious-memory-comparison `_, `bugprone-suspicious-memset-usage `_, "Yes" `bugprone-suspicious-missing-comma `_, + `bugprone-suspicious-realloc-usage `_, `bugprone-suspicious-semicolon `_, "Yes" `bugprone-suspicious-string-compare `_, "Yes" `bugprone-swapped-arguments `_, "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 --- /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 +struct P2 { + static void *p; + static void *q; +}; + +template +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::p = realloc(P2::p, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'P2::p' may be set to null if 'realloc' fails, which may result in a leak of the original buffer [bugprone-suspicious-realloc-usage] + P2::p = realloc(P2::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>(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; +}